[PATCH] kernel/printk: add kmsg SEEK_CUR handling

[PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Bruno Meneguele]

Userspace libraries, e.g. glibc's dprintf(), expect the default return value
for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
current state of kernel code was returning the generic case of an -EINVAL.
Hence, userspace programs were not behaving as expected or documented.

With this patch we add SEEK_CUR case returning the expected value and also a
simple mention of it in kernel's documentation for those relying on that for
guidance.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
 Documentation/ABI/testing/dev-kmsg | 2 ++
 kernel/printk/printk.c             | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/Documentation/ABI/testing/dev-kmsg b/Documentation/ABI/testing/dev-kmsg
index f307506eb54c..8533d28e6fda 100644
--- a/Documentation/ABI/testing/dev-kmsg
+++ b/Documentation/ABI/testing/dev-kmsg
@@ -56,6 +56,8 @@ Description:	The /dev/kmsg character device node provides userspace access
 		  seek after the last record available at the time
 		  the last SYSLOG_ACTION_CLEAR was issued.
 
+		While SEEK_CUR sets -ESPIPE (invalid seek) to errno.
+
 		The output format consists of a prefix carrying the syslog
 		prefix including priority and facility, the 64 bit message
 		sequence number and the monotonic timestamp in microseconds,
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ad4606234545..d02606723d2d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -963,6 +963,10 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
 		user->idx = log_next_idx;
 		user->seq = log_next_seq;
 		break;
+	case SEEK_CUR:
+		/* return the default errno for invalid seek */
+		ret = -ESPIPE;
+		break;
 	default:
 		ret = -EINVAL;
 	}
-- 
2.24.1

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Greg KH]

On Thu, Mar 12, 2020 at 09:35:33PM -0300, Bruno Meneguele wrote:
> Userspace libraries, e.g. glibc's dprintf(), expect the default return value
> for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
> current state of kernel code was returning the generic case of an -EINVAL.
> Hence, userspace programs were not behaving as expected or documented.
> 
> With this patch we add SEEK_CUR case returning the expected value and also a
> simple mention of it in kernel's documentation for those relying on that for
> guidance.
> 
> Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
> ---
>  Documentation/ABI/testing/dev-kmsg | 2 ++
>  kernel/printk/printk.c             | 4 ++++
>  2 files changed, 6 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Sergey Senozhatsky]

On (20/03/12 21:35), Bruno Meneguele wrote:
> 
> Userspace libraries, e.g. glibc's dprintf(), expect the default return value
> for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
> current state of kernel code was returning the generic case of an -EINVAL.
> Hence, userspace programs were not behaving as expected or documented.
> 

Hmm. I don't think I see ESPIPE in documentation [0], [1], [2]

[0] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html
[1] http://man7.org/linux/man-pages/man3/dprintf.3p.html
[2] http://man7.org/linux/man-pages/man3/fprintf.3p.html

	-ss

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

On Fri, Mar 13, 2020 at 08:22:54AM +0100, Greg KH wrote:
> On Thu, Mar 12, 2020 at 09:35:33PM -0300, Bruno Meneguele wrote:
> > Userspace libraries, e.g. glibc's dprintf(), expect the default return value
> > for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
> > current state of kernel code was returning the generic case of an -EINVAL.
> > Hence, userspace programs were not behaving as expected or documented.
> > 
> > With this patch we add SEEK_CUR case returning the expected value and also a
> > simple mention of it in kernel's documentation for those relying on that for
> > guidance.
> > 
> > Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
> > ---
> >  Documentation/ABI/testing/dev-kmsg | 2 ++
> >  kernel/printk/printk.c             | 4 ++++
> >  2 files changed, 6 insertions(+)
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>
> 

ouch, yes of course. Sorry for the noise.  
Will repost it once the concerns with the patch are solved.

Thanks Greg.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

On Fri, Mar 13, 2020 at 04:34:25PM +0900, Sergey Senozhatsky wrote:
> On (20/03/12 21:35), Bruno Meneguele wrote:
> > 
> > Userspace libraries, e.g. glibc's dprintf(), expect the default return value
> > for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
> > current state of kernel code was returning the generic case of an -EINVAL.
> > Hence, userspace programs were not behaving as expected or documented.
> > 
> 
> Hmm. I don't think I see ESPIPE in documentation [0], [1], [2]
> 
> [0] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html
> [1] http://man7.org/linux/man-pages/man3/dprintf.3p.html
> [2] http://man7.org/linux/man-pages/man3/fprintf.3p.html
> 
> 	-ss
> 

Ok, I poorly expressed the notion of "documentantion". The userspace
doesn't tell about returning -ESPIPE, but to the functions work properly
they watch for -ESPIPE returning from the syscall. For instance, gblic
dprintf() implementation:

dprintf:
  __vdprintf_internal:
    _IO_new_file_attach:

  if (_IO_SEEKOFF (fp, (off64_t)0, _IO_seek_cur, _IOS_INPUT|_IOS_OUTPUT)
      == _IO_pos_BAD && errno != ESPIPE)
    return NULL;

With that, if the seek fails, but return anything other than ESPIPE the
dprintf() will also fail returning -EINVAL to dprintf() caller. While if
ESPIPE is returned, it's "ignored" and the call still works. The way we
have today make kmsg an exception case among the rest of the system
files where you can open with dprintf.

One of the things I could agree with is removing the SEEK call from
dprintf, since fprintf basically follows the same steps, but doesn't
seek anything.  But at the same time, IMO it makes sense to make kmsg
interface complaint with the errno return values.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[David Laight]

From: Bruno Meneguele
> Sent: 13 March 2020 11:02
> On Fri, Mar 13, 2020 at 04:34:25PM +0900, Sergey Senozhatsky wrote:
> > On (20/03/12 21:35), Bruno Meneguele wrote:
> > >
> > > Userspace libraries, e.g. glibc's dprintf(), expect the default return value
> > > for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
> > > current state of kernel code was returning the generic case of an -EINVAL.
> > > Hence, userspace programs were not behaving as expected or documented.
> > >
> >
> > Hmm. I don't think I see ESPIPE in documentation [0], [1], [2]
> >
> > [0] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html
> > [1] http://man7.org/linux/man-pages/man3/dprintf.3p.html
> > [2] http://man7.org/linux/man-pages/man3/fprintf.3p.html
> >
> > 	-ss
> >
> 
> Ok, I poorly expressed the notion of "documentantion". The userspace
> doesn't tell about returning -ESPIPE, but to the functions work properly
> they watch for -ESPIPE returning from the syscall. For instance, gblic
> dprintf() implementation:
> 
> dprintf:
>   __vdprintf_internal:
>     _IO_new_file_attach:
> 
>   if (_IO_SEEKOFF (fp, (off64_t)0, _IO_seek_cur, _IOS_INPUT|_IOS_OUTPUT)
>       == _IO_pos_BAD && errno != ESPIPE)
>     return NULL;

Someone explain why it is doing an explicit seek to the current position?
The only reason to do that is to get the current offset.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1909 bytes --]

On Fri, Mar 13, 2020 at 11:06:42AM +0000, David Laight wrote:
> From: Bruno Meneguele
> > Sent: 13 March 2020 11:02
> > On Fri, Mar 13, 2020 at 04:34:25PM +0900, Sergey Senozhatsky wrote:
> > > On (20/03/12 21:35), Bruno Meneguele wrote:
> > > >
> > > > Userspace libraries, e.g. glibc's dprintf(), expect the default return value
> > > > for invalid seek situations: -ESPIPE, but when the IO was over /dev/kmsg the
> > > > current state of kernel code was returning the generic case of an -EINVAL.
> > > > Hence, userspace programs were not behaving as expected or documented.
> > > >
> > >
> > > Hmm. I don't think I see ESPIPE in documentation [0], [1], [2]
> > >
> > > [0] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html
> > > [1] http://man7.org/linux/man-pages/man3/dprintf.3p.html
> > > [2] http://man7.org/linux/man-pages/man3/fprintf.3p.html
> > >
> > > 	-ss
> > >
> > 
> > Ok, I poorly expressed the notion of "documentantion". The userspace
> > doesn't tell about returning -ESPIPE, but to the functions work properly
> > they watch for -ESPIPE returning from the syscall. For instance, gblic
> > dprintf() implementation:
> > 
> > dprintf:
> >   __vdprintf_internal:
> >     _IO_new_file_attach:
> > 
> >   if (_IO_SEEKOFF (fp, (off64_t)0, _IO_seek_cur, _IOS_INPUT|_IOS_OUTPUT)
> >       == _IO_pos_BAD && errno != ESPIPE)
> >     return NULL;
> 
> Someone explain why it is doing an explicit seek to the current position?
> The only reason to do that is to get the current offset.
> 
> 	David
> 

dprintf gets a fd as input and convert it to a FILE structure, with that
it can't garantuee the previous state of that fd: was it already
manipulated? Thus they check the current position to make sure it's not
junk.

But that's me guessing things about a code from 1996 :).

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Sergey Senozhatsky]

On (20/03/13 08:02), Bruno Meneguele wrote:
> Ok, I poorly expressed the notion of "documentantion". The userspace
> doesn't tell about returning -ESPIPE, but to the functions work properly
> they watch for -ESPIPE returning from the syscall. For instance, gblic
> dprintf() implementation:
> 
> dprintf:
>   __vdprintf_internal:
>     _IO_new_file_attach:
> 
>   if (_IO_SEEKOFF (fp, (off64_t)0, _IO_seek_cur, _IOS_INPUT|_IOS_OUTPUT)
>       == _IO_pos_BAD && errno != ESPIPE)
>     return NULL;
> 
> With that, if the seek fails, but return anything other than ESPIPE the
> dprintf() will also fail returning -EINVAL to dprintf() caller. While if
> ESPIPE is returned, it's "ignored" and the call still works. The way we
> have today make kmsg an exception case among the rest of the system
> files where you can open with dprintf.
> 
> One of the things I could agree with is removing the SEEK call from
> dprintf, since fprintf basically follows the same steps, but doesn't
> seek anything.  But at the same time, IMO it makes sense to make kmsg
> interface complaint with the errno return values.

The code in questions is very old. So let's add the missing bit to the
kernel. At the same time, we probably can have a slightly more detailed
documentation / code comment.

	-ss

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On Tue, Mar 17, 2020 at 11:03:26AM +0900, Sergey Senozhatsky wrote:
> On (20/03/13 08:02), Bruno Meneguele wrote:
> > Ok, I poorly expressed the notion of "documentantion". The userspace
> > doesn't tell about returning -ESPIPE, but to the functions work properly
> > they watch for -ESPIPE returning from the syscall. For instance, gblic
> > dprintf() implementation:
> > 
> > dprintf:
> >   __vdprintf_internal:
> >     _IO_new_file_attach:
> > 
> >   if (_IO_SEEKOFF (fp, (off64_t)0, _IO_seek_cur, _IOS_INPUT|_IOS_OUTPUT)
> >       == _IO_pos_BAD && errno != ESPIPE)
> >     return NULL;
> > 
> > With that, if the seek fails, but return anything other than ESPIPE the
> > dprintf() will also fail returning -EINVAL to dprintf() caller. While if
> > ESPIPE is returned, it's "ignored" and the call still works. The way we
> > have today make kmsg an exception case among the rest of the system
> > files where you can open with dprintf.
> > 
> > One of the things I could agree with is removing the SEEK call from
> > dprintf, since fprintf basically follows the same steps, but doesn't
> > seek anything.  But at the same time, IMO it makes sense to make kmsg
> > interface complaint with the errno return values.
> 
> The code in questions is very old. So let's add the missing bit to the
> kernel. At the same time, we probably can have a slightly more detailed
> documentation / code comment.
> 
> 	-ss
> 

Sure thing.

I'm going to send a v2 of this patch with more details still today or
tomorrow.

Thanks!

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kernel/printk: add kmsg SEEK_CUR handling

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

On Tue, Mar 17, 2020 at 05:54:00AM -0300, Bruno Meneguele wrote:
> On Tue, Mar 17, 2020 at 11:03:26AM +0900, Sergey Senozhatsky wrote:
> > On (20/03/13 08:02), Bruno Meneguele wrote:
> > > Ok, I poorly expressed the notion of "documentantion". The userspace
> > > doesn't tell about returning -ESPIPE, but to the functions work properly
> > > they watch for -ESPIPE returning from the syscall. For instance, gblic
> > > dprintf() implementation:
> > > 
> > > dprintf:
> > >   __vdprintf_internal:
> > >     _IO_new_file_attach:
> > > 
> > >   if (_IO_SEEKOFF (fp, (off64_t)0, _IO_seek_cur, _IOS_INPUT|_IOS_OUTPUT)
> > >       == _IO_pos_BAD && errno != ESPIPE)
> > >     return NULL;
> > > 
> > > With that, if the seek fails, but return anything other than ESPIPE the
> > > dprintf() will also fail returning -EINVAL to dprintf() caller. While if
> > > ESPIPE is returned, it's "ignored" and the call still works. The way we
> > > have today make kmsg an exception case among the rest of the system
> > > files where you can open with dprintf.
> > > 
> > > One of the things I could agree with is removing the SEEK call from
> > > dprintf, since fprintf basically follows the same steps, but doesn't
> > > seek anything.  But at the same time, IMO it makes sense to make kmsg
> > > interface complaint with the errno return values.
> > 
> > The code in questions is very old. So let's add the missing bit to the
> > kernel. At the same time, we probably can have a slightly more detailed
> > documentation / code comment.
> > 
> > 	-ss
> > 
> 
> Sure thing.
> 
> I'm going to send a v2 of this patch with more details still today or
> tomorrow.
> 
> Thanks!
> 

Just fyi, v2 was posted:
https://lkml.org/lkml/2020/3/17/321

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]