kmsg: lseek errors confuse glibc's dprintf

kmsg: lseek errors confuse glibc's dprintf

[Mike Crowe]

glibc's dprintf implementation does not work correctly with /dev/kmsg file
descriptors because glibc treats receiving EBADF and EINVAL from lseek when
trying to determine the current file position as errors. See
https://sourceware.org/bugzilla/show_bug.cgi?id=17830

>From what I can tell prior to Kay Sievers printk record commit
e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
with such a file descriptor would not return an error.

Prior to Kay's change, Arnd Bergmann's commit
6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
preserve the successful return code rather than returning (the perhaps more
logical) -ESPIPE.

glibc is happy with either a successful return or -ESPIPE.

For maximum compatibility it seems that success should be returned but
given Kay's new seek interface perhaps this isn't helpful.

This patch ensures that such a seek succeeds:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 02d6b6d..b3ff6f0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
 	loff_t ret = 0;
 
 	if (!user)
-		return -EBADF;
+		return (whence == SEEK_CUR) ? 0 : -EBADF;
 	if (offset)
 		return -ESPIPE;
 
@@ -718,6 +718,11 @@ 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:
+		/* For compatibility with userspace requesting the
+		 * current file position. */
+		ret = 0;
+		break;
 	default:
 		ret = -EINVAL;
 	}

(although it could be argued that the !user case should return -ESPIPE
rather than EBADF since the file descriptor _is_ valid.)

and this patch causes a failure that glibc is prepared to accept:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 02d6b6d..f6b0c93 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
 	loff_t ret = 0;
 
 	if (!user)
-		return -EBADF;
+		return -ESPIPE;
 	if (offset)
 		return -ESPIPE;
 
@@ -718,6 +718,11 @@ 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:
+		/* For compatibility with userspace expecting SEEK_CUR
+		 * to not yield EINVAL. */
+		ret = -ESPIPE;
+		break;
 	default:
 		ret = -EINVAL;
 	}

Either makes dprintf work, but is either the right solution?

Thanks.

Mike.

Re: kmsg: lseek errors confuse glibc's dprintf

[Andrew Morton]

On Thu, 15 Jan 2015 17:31:32 +0000 Mike Crowe <mac@mcrowe.com> wrote:

> glibc's dprintf implementation does not work correctly with /dev/kmsg file
> descriptors because glibc treats receiving EBADF and EINVAL from lseek when
> trying to determine the current file position as errors. See
> https://sourceware.org/bugzilla/show_bug.cgi?id=17830
> 
> >From what I can tell prior to Kay Sievers printk record commit
> e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
> with such a file descriptor would not return an error.
> 
> Prior to Kay's change, Arnd Bergmann's commit
> 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
> preserve the successful return code rather than returning (the perhaps more
> logical) -ESPIPE.
> 
> glibc is happy with either a successful return or -ESPIPE.
> 
> For maximum compatibility it seems that success should be returned but
> given Kay's new seek interface perhaps this isn't helpful.
> 
> This patch ensures that such a seek succeeds:
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 02d6b6d..b3ff6f0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
>  	loff_t ret = 0;
>  
>  	if (!user)
> -		return -EBADF;
> +		return (whence == SEEK_CUR) ? 0 : -EBADF;

What's actually going on here?  What is the significance of
file->private_data==NULL and why does this code treat it as an error?

>  	if (offset)
>  		return -ESPIPE;
>  
> @@ -718,6 +718,11 @@ 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:
> +		/* For compatibility with userspace requesting the
> +		 * current file position. */
> +		ret = 0;
> +		break;

Can we actually do something useful here?  Return some value which can
be fed back into SEEK_SET to restore the file position?

>  	default:
>  		ret = -EINVAL;
>  	}

Re: kmsg: lseek errors confuse glibc's dprintf

[Mike Crowe]

On Friday 23 January 2015 at 15:09:39 -0800, Andrew Morton wrote:
> On Thu, 15 Jan 2015 17:31:32 +0000 Mike Crowe <mac@mcrowe.com> wrote:
> 
> > glibc's dprintf implementation does not work correctly with /dev/kmsg file
> > descriptors because glibc treats receiving EBADF and EINVAL from lseek when
> > trying to determine the current file position as errors. See
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17830
> > 
> > From what I can tell prior to Kay Sievers printk record commit
> > e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
> > with such a file descriptor would not return an error.
> > 
> > Prior to Kay's change, Arnd Bergmann's commit
> > 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
> > preserve the successful return code rather than returning (the perhaps more
> > logical) -ESPIPE.
> > 
> > glibc is happy with either a successful return or -ESPIPE.
> > 
> > For maximum compatibility it seems that success should be returned but
> > given Kay's new seek interface perhaps this isn't helpful.
> > 
> > This patch ensures that such a seek succeeds:
> > 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 02d6b6d..b3ff6f0 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
> >  	loff_t ret = 0;
> >  
> >  	if (!user)
> > -		return -EBADF;
> > +		return (whence == SEEK_CUR) ? 0 : -EBADF;
> 
> What's actually going on here?  What is the significance of
> file->private_data==NULL and why does this code treat it as an error?

Kay is presumably the expert on this but my understanding is that opening
/dev/kmsg for writing only is supposed to be as lightweight as possible -
there's not even a context structure so file->private_data is NULL (see
devmksg_open.) In this mode seeking is not supported. I believe that EBADF
is not a particularly helpful error in this case but didn't want to
complicate the patch with a separate change.

When /dev/kmsg is opened for reading seeking is supported but the seek
behaviour is tailored to (I assume) systemd journal's usage.

> >  	if (offset)
> >  		return -ESPIPE;
> >  
> > @@ -718,6 +718,11 @@ 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:
> > +		/* For compatibility with userspace requesting the
> > +		 * current file position. */
> > +		ret = 0;
> > +		break;
> 
> Can we actually do something useful here?  Return some value which can
> be fed back into SEEK_SET to restore the file position?

Perhaps that would be useful for someone. It would certainly be more
logical than just returning zero.

> 
> >  	default:
> >  		ret = -EINVAL;
> >  	}

Mike.

Re: kmsg: lseek errors confuse glibc's dprintf

[Alexander Sverdlin]

Hello Mike and all,

On Thu, 15 Jan 2015 17:31:32 +0000
Mike Crowe <mac@mcrowe.com> wrote:

> glibc's dprintf implementation does not work correctly with /dev/kmsg file
> descriptors because glibc treats receiving EBADF and EINVAL from lseek when
> trying to determine the current file position as errors. See
> https://sourceware.org/bugzilla/show_bug.cgi?id=17830

we need to conclude on this issue. This is a real bug which is ignored
for 4 years now. Mike, would you like to re-send a formal patch?
I can do it as well, preserving a link to your original patch/report.
In case you'd like to post it yourself, I can be a tester and/or
provide a reproducer.

> >>From what I can tell prior to Kay Sievers printk record commit
> e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
> with such a file descriptor would not return an error.
> 
> Prior to Kay's change, Arnd Bergmann's commit
> 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
> preserve the successful return code rather than returning (the perhaps more
> logical) -ESPIPE.
> 
> glibc is happy with either a successful return or -ESPIPE.
> 
> For maximum compatibility it seems that success should be returned but
> given Kay's new seek interface perhaps this isn't helpful.
> 
> This patch ensures that such a seek succeeds:
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 02d6b6d..b3ff6f0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
>  	loff_t ret = 0;
>  
>  	if (!user)
> -		return -EBADF;
> +		return (whence == SEEK_CUR) ? 0 : -EBADF;
>  	if (offset)
>  		return -ESPIPE;
>  
> @@ -718,6 +718,11 @@ 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:
> +		/* For compatibility with userspace requesting the
> +		 * current file position. */
> +		ret = 0;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> 
> (although it could be argued that the !user case should return -ESPIPE
> rather than EBADF since the file descriptor _is_ valid.)
> 
> and this patch causes a failure that glibc is prepared to accept:
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 02d6b6d..f6b0c93 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
>  	loff_t ret = 0;
>  
>  	if (!user)
> -		return -EBADF;
> +		return -ESPIPE;
>  	if (offset)
>  		return -ESPIPE;
>  
> @@ -718,6 +718,11 @@ 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:
> +		/* For compatibility with userspace expecting SEEK_CUR
> +		 * to not yield EINVAL. */
> +		ret = -ESPIPE;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> 
> Either makes dprintf work, but is either the right solution?
> 
> Thanks.
> 
> Mike.


-- 
Alexander Sverdlin.

Re: kmsg: lseek errors confuse glibc's dprintf

[Arnd Bergmann]

On Thu, Mar 21, 2019 at 10:47 AM Alexander Sverdlin
<alexander.sverdlin@gmail.com> wrote:
>
> Hello Mike and all,
>
> On Thu, 15 Jan 2015 17:31:32 +0000
> Mike Crowe <mac@mcrowe.com> wrote:
>
> > glibc's dprintf implementation does not work correctly with /dev/kmsg file
> > descriptors because glibc treats receiving EBADF and EINVAL from lseek when
> > trying to determine the current file position as errors. See
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17830
>
> we need to conclude on this issue. This is a real bug which is ignored
> for 4 years now. Mike, would you like to re-send a formal patch?
> I can do it as well, preserving a link to your original patch/report.
> In case you'd like to post it yourself, I can be a tester and/or
> provide a reproducer.

The patch needs to be rebased because of the changed file
location. I would also suggest adding a "Cc: stable@kernel.org"
tag so it will get backported into stable kernels.

> > >>From what I can tell prior to Kay Sievers printk record commit
> > e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
> > with such a file descriptor would not return an error.
> >
> > Prior to Kay's change, Arnd Bergmann's commit
> > 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
> > preserve the successful return code rather than returning (the perhaps more
> > logical) -ESPIPE.
> >
> > glibc is happy with either a successful return or -ESPIPE.
> >
> > For maximum compatibility it seems that success should be returned but
> > given Kay's new seek interface perhaps this isn't helpful.
> >
> > This patch ensures that such a seek succeeds:
> >
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 02d6b6d..b3ff6f0 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
> >       loff_t ret = 0;
> >
> >       if (!user)
> > -             return -EBADF;
> > +             return (whence == SEEK_CUR) ? 0 : -EBADF;
> >       if (offset)
> >               return -ESPIPE;
> >
> > @@ -718,6 +718,11 @@ 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:
> > +             /* For compatibility with userspace requesting the
> > +              * current file position. */
> > +             ret = 0;
> > +             break;
> >       default:
> >               ret = -EINVAL;
> >       }
> >
> > (although it could be argued that the !user case should return -ESPIPE
> > rather than EBADF since the file descriptor _is_ valid.)

I don't think the !user case can ever be hit, I would just leave that
to return -BADF and not touch it.

> > @@ -718,6 +718,11 @@ 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:
> > +             /* For compatibility with userspace expecting SEEK_CUR
> > +              * to not yield EINVAL. */
> > +             ret = -ESPIPE;
> > +             break;
> >       default:
> >               ret = -EINVAL;
> >       }
> >
> > Either makes dprintf work, but is either the right solution?

I'd vote for -ESPIPE, for consistency with the offset!=0 case, but
etiher one is fine with me here.

       Arnd

Re: kmsg: lseek errors confuse glibc's dprintf

[Mike Crowe]

On Thursday 21 March 2019 at 11:33:33 +0100, Arnd Bergmann wrote:
> On Thu, Mar 21, 2019 at 10:47 AM Alexander Sverdlin
> <alexander.sverdlin@gmail.com> wrote:
> >
> > Hello Mike and all,
> >
> > On Thu, 15 Jan 2015 17:31:32 +0000
> > Mike Crowe <mac@mcrowe.com> wrote:
> >
> > > glibc's dprintf implementation does not work correctly with /dev/kmsg file
> > > descriptors because glibc treats receiving EBADF and EINVAL from lseek when
> > > trying to determine the current file position as errors. See
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=17830
> >
> > we need to conclude on this issue. This is a real bug which is ignored
> > for 4 years now. Mike, would you like to re-send a formal patch?
> > I can do it as well, preserving a link to your original patch/report.
> > In case you'd like to post it yourself, I can be a tester and/or
> > provide a reproducer.
> 
> The patch needs to be rebased because of the changed file
> location. I would also suggest adding a "Cc: stable@kernel.org"
> tag so it will get backported into stable kernels.
> 
> > > >>From what I can tell prior to Kay Sievers printk record commit
> > > e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
> > > with such a file descriptor would not return an error.
> > >
> > > Prior to Kay's change, Arnd Bergmann's commit
> > > 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
> > > preserve the successful return code rather than returning (the perhaps more
> > > logical) -ESPIPE.
> > >
> > > glibc is happy with either a successful return or -ESPIPE.
> > >
> > > For maximum compatibility it seems that success should be returned but
> > > given Kay's new seek interface perhaps this isn't helpful.
> > >
> > > This patch ensures that such a seek succeeds:
> > >
> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > > index 02d6b6d..b3ff6f0 100644
> > > --- a/kernel/printk/printk.c
> > > +++ b/kernel/printk/printk.c
> > > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
> > >       loff_t ret = 0;
> > >
> > >       if (!user)
> > > -             return -EBADF;
> > > +             return (whence == SEEK_CUR) ? 0 : -EBADF;
> > >       if (offset)
> > >               return -ESPIPE;
> > >
> > > @@ -718,6 +718,11 @@ 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:
> > > +             /* For compatibility with userspace requesting the
> > > +              * current file position. */
> > > +             ret = 0;
> > > +             break;
> > >       default:
> > >               ret = -EINVAL;
> > >       }
> > >
> > > (although it could be argued that the !user case should return -ESPIPE
> > > rather than EBADF since the file descriptor _is_ valid.)
> 
> I don't think the !user case can ever be hit, I would just leave that
> to return -BADF and not touch it.

I originally wrote the patch for linux-3.8(!) and back then a write-only
kmsg instance had no private data. It looks like this changed with
750afe7babd117daabebf4855da18e4418ea845e in v4.8, so user should now always
be valid.

> > > @@ -718,6 +718,11 @@ 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:
> > > +             /* For compatibility with userspace expecting SEEK_CUR
> > > +              * to not yield EINVAL. */
> > > +             ret = -ESPIPE;
> > > +             break;
> > >       default:
> > >               ret = -EINVAL;
> > >       }
> > >
> > > Either makes dprintf work, but is either the right solution?
> 
> I'd vote for -ESPIPE, for consistency with the offset!=0 case, but
> etiher one is fine with me here.

It seems that this is the version we've successfully been running in our
kernel since then, so that's good.

Thanks.

Mike.

Re: kmsg: lseek errors confuse glibc's dprintf

[Mike Crowe]

On Thursday 21 March 2019 at 10:47:26 +0100, Alexander Sverdlin wrote:
> Hello Mike and all,
> 
> On Thu, 15 Jan 2015 17:31:32 +0000
> Mike Crowe <mac@mcrowe.com> wrote:
> 
> > glibc's dprintf implementation does not work correctly with /dev/kmsg file
> > descriptors because glibc treats receiving EBADF and EINVAL from lseek when
> > trying to determine the current file position as errors. See
> > https://sourceware.org/bugzilla/show_bug.cgi?id=17830
> 
> we need to conclude on this issue. This is a real bug which is ignored
> for 4 years now. Mike, would you like to re-send a formal patch?
> I can do it as well, preserving a link to your original patch/report.
> In case you'd like to post it yourself, I can be a tester and/or
> provide a reproducer.

Hi Alexander,

I'm sorry for dropping the ball on this. I think I was travelling at the
time. :( Thank you for picking it up.

I don't mind rebasing my patch once I know which of my two solutions is
preferred. It seems that the -ESPIPE one is the current favourite.

Thanks.

> > >>From what I can tell prior to Kay Sievers printk record commit
> > e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
> > with such a file descriptor would not return an error.
> > 
> > Prior to Kay's change, Arnd Bergmann's commit
> > 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
> > preserve the successful return code rather than returning (the perhaps more
> > logical) -ESPIPE.
> > 
> > glibc is happy with either a successful return or -ESPIPE.
> > 
> > For maximum compatibility it seems that success should be returned but
> > given Kay's new seek interface perhaps this isn't helpful.
> > 
> > This patch ensures that such a seek succeeds:
> > 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 02d6b6d..b3ff6f0 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
> >  	loff_t ret = 0;
> >  
> >  	if (!user)
> > -		return -EBADF;
> > +		return (whence == SEEK_CUR) ? 0 : -EBADF;
> >  	if (offset)
> >  		return -ESPIPE;
> >  
> > @@ -718,6 +718,11 @@ 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:
> > +		/* For compatibility with userspace requesting the
> > +		 * current file position. */
> > +		ret = 0;
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > 
> > (although it could be argued that the !user case should return -ESPIPE
> > rather than EBADF since the file descriptor _is_ valid.)
> > 
> > and this patch causes a failure that glibc is prepared to accept:
> > 
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 02d6b6d..f6b0c93 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
> >  	loff_t ret = 0;
> >  
> >  	if (!user)
> > -		return -EBADF;
> > +		return -ESPIPE;
> >  	if (offset)
> >  		return -ESPIPE;
> >  
> > @@ -718,6 +718,11 @@ 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:
> > +		/* For compatibility with userspace expecting SEEK_CUR
> > +		 * to not yield EINVAL. */
> > +		ret = -ESPIPE;
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > 
> > Either makes dprintf work, but is either the right solution?

Mike.

Re: Re: kmsg: lseek errors confuse glibc's dprintf

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi!

On 21/03/2019 13:38, Mike Crowe wrote:
[...]
> I don't mind rebasing my patch once I know which of my two solutions is
> preferred. It seems that the -ESPIPE one is the current favourite.

I think:
1. Since Linux v4.8 (!user) case in not possible any more if the device
   file was opened in write-only mode, so you are probably safe to omit this
   chunk at all.

2. Nothing comes to my mind, what one could return as "current position"
   from the lseek() call on /dev/kmsg. So without good idea what to return,
   supporting offset == 0 (the only thing we ever can support on SEEK_CUR)
   makes no sense to me.
   Documentation/ABI/testing/dev-kmsg makes no promise on SEEK_CUR either,
   therefore I'd say, -ESPIPE unconditionally is the right choice.

>>>> >From what I can tell prior to Kay Sievers printk record commit
>>> e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR)
>>> with such a file descriptor would not return an error.
>>>
>>> Prior to Kay's change, Arnd Bergmann's commit
>>> 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to
>>> preserve the successful return code rather than returning (the perhaps more
>>> logical) -ESPIPE.
>>>
>>> glibc is happy with either a successful return or -ESPIPE.
>>>
>>> For maximum compatibility it seems that success should be returned but
>>> given Kay's new seek interface perhaps this isn't helpful.
>>>
>>> This patch ensures that such a seek succeeds:
>>>
>>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>>> index 02d6b6d..b3ff6f0 100644
>>> --- a/kernel/printk/printk.c
>>> +++ b/kernel/printk/printk.c
>>> @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
>>>  	loff_t ret = 0;
>>>  
>>>  	if (!user)
>>> -		return -EBADF;
>>> +		return (whence == SEEK_CUR) ? 0 : -EBADF;
>>>  	if (offset)
>>>  		return -ESPIPE;
>>>  
>>> @@ -718,6 +718,11 @@ 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:
>>> +		/* For compatibility with userspace requesting the
>>> +		 * current file position. */
>>> +		ret = 0;
>>> +		break;
>>>  	default:
>>>  		ret = -EINVAL;
>>>  	}
>>>
>>> (although it could be argued that the !user case should return -ESPIPE
>>> rather than EBADF since the file descriptor _is_ valid.)
>>>
>>> and this patch causes a failure that glibc is prepared to accept:
>>>
>>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>>> index 02d6b6d..f6b0c93 100644
>>> --- a/kernel/printk/printk.c
>>> +++ b/kernel/printk/printk.c
>>> @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t offset, int whence)
>>>  	loff_t ret = 0;
>>>  
>>>  	if (!user)
>>> -		return -EBADF;
>>> +		return -ESPIPE;
>>>  	if (offset)
>>>  		return -ESPIPE;
>>>  
>>> @@ -718,6 +718,11 @@ 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:
>>> +		/* For compatibility with userspace expecting SEEK_CUR
>>> +		 * to not yield EINVAL. */
>>> +		ret = -ESPIPE;
>>> +		break;
>>>  	default:
>>>  		ret = -EINVAL;
>>>  	}
>>>
>>> Either makes dprintf work, but is either the right solution?
> 
> Mike.
> 

-- 
Best regards,
Alexander Sverdlin.