Hello Mike and all,
On Thu, 15 Jan 2015 17:31:32 +0000
Mike Crowe <[email protected]> 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.
On Thu, Mar 21, 2019 at 10:47 AM Alexander Sverdlin
<[email protected]> wrote:
>
> Hello Mike and all,
>
> On Thu, 15 Jan 2015 17:31:32 +0000
> Mike Crowe <[email protected]> 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: [email protected]"
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
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 <[email protected]> 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.
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.
On Thursday 21 March 2019 at 11:33:33 +0100, Arnd Bergmann wrote:
> On Thu, Mar 21, 2019 at 10:47 AM Alexander Sverdlin
> <[email protected]> wrote:
> >
> > Hello Mike and all,
> >
> > On Thu, 15 Jan 2015 17:31:32 +0000
> > Mike Crowe <[email protected]> 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: [email protected]"
> 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.