Re: kmsg: lseek errors confuse glibc's dprintf

[Mike Crowe <mac@mcrowe.com>]

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.