All of lore.kernel.org
 help / color / mirror / Atom feed
* [BUG -next] sysfs change breaks userspace
@ 2013-10-31 11:43 Heiko Carstens
  2013-10-31 17:25 ` Tejun Heo
  0 siblings, 1 reply; 10+ messages in thread
From: Heiko Carstens @ 2013-10-31 11:43 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Greg Kroah-Hartman, Kay Sievers, linux-next

Hi Tejun,

I just tried out linux-next and my network doesn't come up anymore.
Userspace fails like this:

network[2211]: Bringing up interface eth0:  sysfs read broadcast value: Invalid argument

I bisected that down to:

commit 13c589d5b0ac654d9da7e490a2dd548e6b86b4a5
Author: Tejun Heo <tj@kernel.org>
Date:   Tue Oct 1 17:42:02 2013 -0400

    sysfs: use seq_file when reading regular files
    
    sysfs read path implements its own buffering scheme between userland
    and kernel callbacks, which essentially is a degenerate duplicate of
    seq_file.  This patch replaces the custom read buffering
    implementation in sysfs with seq_file.
    
    While the amount of code reduction is small, this reduces low level
    hairiness and enables future development of a new versatile API based
    on seq_file so that sysfs features can be shared with other
    subsystems.
    
    As write path was already converted to not use sysfs_open_file->page,
    this patch makes ->page and ->count unused and removes them.
    
    Userland behavior remains the same except for some extreme corner
    cases - e.g. sysfs will now regenerate the content each time a file is
    read after a non-contiguous seek whereas the original code would keep
    using the same content.  While this is a userland visible behavior
    change, it is extremely unlikely to be noticeable and brings sysfs
    behavior closer to that of procfs.
    
    Signed-off-by: Tejun Heo <tj@kernel.org>
    Cc: Kay Sievers <kay@vrfy.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG -next] sysfs change breaks userspace
  2013-10-31 11:43 [BUG -next] sysfs change breaks userspace Heiko Carstens
@ 2013-10-31 17:25 ` Tejun Heo
  2013-11-01 14:13   ` Heiko Carstens
  0 siblings, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2013-10-31 17:25 UTC (permalink / raw)
  To: Heiko Carstens; +Cc: Greg Kroah-Hartman, Kay Sievers, linux-next

Hello, Heiko.

On Thu, Oct 31, 2013 at 12:43:58PM +0100, Heiko Carstens wrote:
> Hi Tejun,
> 
> I just tried out linux-next and my network doesn't come up anymore.
> Userspace fails like this:
> 
> network[2211]: Bringing up interface eth0:  sysfs read broadcast value: Invalid argument
> 
> I bisected that down to:
> 
> commit 13c589d5b0ac654d9da7e490a2dd548e6b86b4a5
> Author: Tejun Heo <tj@kernel.org>
> Date:   Tue Oct 1 17:42:02 2013 -0400
> 
>     sysfs: use seq_file when reading regular files

Heh, intereting.  The content doesn't change over multiple show
invocations, so the behavior shouldn't change at all for the attribute
and seq_file handles seeking and partial reads correctly.  No idea
what could go wrong there.  It probably was reading
/sys/devices/BLAHBLAH/net/NETIF/broadcast file.  Can you please locate
the file and do "ls -l" and "cat" on it?  If that looks normal, can
you please strace the network interface config program / script /
whatever?  BTW, what are you running on the system?

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG -next] sysfs change breaks userspace
  2013-10-31 17:25 ` Tejun Heo
@ 2013-11-01 14:13   ` Heiko Carstens
  2013-11-01 14:35     ` Tejun Heo
  0 siblings, 1 reply; 10+ messages in thread
From: Heiko Carstens @ 2013-11-01 14:13 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Greg Kroah-Hartman, Kay Sievers, linux-next

On Thu, Oct 31, 2013 at 01:25:06PM -0400, Tejun Heo wrote:
> Hello, Heiko.
> 
> On Thu, Oct 31, 2013 at 12:43:58PM +0100, Heiko Carstens wrote:
> > Hi Tejun,
> > 
> > I just tried out linux-next and my network doesn't come up anymore.
> > Userspace fails like this:
> > 
> > network[2211]: Bringing up interface eth0:  sysfs read broadcast value: Invalid argument
> > 
> > I bisected that down to:
> > 
> > commit 13c589d5b0ac654d9da7e490a2dd548e6b86b4a5
> > Author: Tejun Heo <tj@kernel.org>
> > Date:   Tue Oct 1 17:42:02 2013 -0400
> > 
> >     sysfs: use seq_file when reading regular files
> 
> Heh, intereting.  The content doesn't change over multiple show
> invocations, so the behavior shouldn't change at all for the attribute
> and seq_file handles seeking and partial reads correctly.  No idea
> what could go wrong there.  It probably was reading
> /sys/devices/BLAHBLAH/net/NETIF/broadcast file.  Can you please locate
> the file and do "ls -l" and "cat" on it?  If that looks normal, can
> you please strace the network interface config program / script /
> whatever?  BTW, what are you running on the system?

Ok, here we go:

before your patch it was like this:

[pid  2888] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
[pid  2888] lseek(5, 0, SEEK_END)       = 4096
[pid  2888] lseek(5, 0, SEEK_SET)       = 0
[pid  2888] read(5, "ff:ff:ff:ff:ff:ff\n", 4096) = 18
[pid  2888] close(5)                    = 0

With your patch applied I get this:

[pid  2450] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
[pid  2450] lseek(5, 0, SEEK_END)       = -1 EINVAL (Invalid argument)
[pid  2450] lseek(5, 0, SEEK_SET)       = 0
[pid  2450] read(5, 0x557421e8, 4294967295) = -1 EINVAL (Invalid argument)
[pid  2450] close(5)                    = 0

So the problem is that lseek with SEEK_END doesn't work.
Afterwards the process tried to use the return value of lseek as number of
bytes to be read, which doesn't work ;)

This is a Fedora 17 like system on s390. It's a bit special since the kernel
is 64 bit and whole user space is 32 bit.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG -next] sysfs change breaks userspace
  2013-11-01 14:13   ` Heiko Carstens
@ 2013-11-01 14:35     ` Tejun Heo
  2013-11-01 15:04       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2013-11-01 14:35 UTC (permalink / raw)
  To: Heiko Carstens; +Cc: Greg Kroah-Hartman, Kay Sievers, linux-next

Hello, Heiko.

On Fri, Nov 01, 2013 at 03:13:48PM +0100, Heiko Carstens wrote:
> before your patch it was like this:
> 
> [pid  2888] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
> [pid  2888] lseek(5, 0, SEEK_END)       = 4096
> [pid  2888] lseek(5, 0, SEEK_SET)       = 0
> [pid  2888] read(5, "ff:ff:ff:ff:ff:ff\n", 4096) = 18
> [pid  2888] close(5)                    = 0
> 
> With your patch applied I get this:
> 
> [pid  2450] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
> [pid  2450] lseek(5, 0, SEEK_END)       = -1 EINVAL (Invalid argument)
> [pid  2450] lseek(5, 0, SEEK_SET)       = 0
> [pid  2450] read(5, 0x557421e8, 4294967295) = -1 EINVAL (Invalid argument)
> [pid  2450] close(5)                    = 0
> 
> So the problem is that lseek with SEEK_END doesn't work.
> Afterwards the process tried to use the return value of lseek as number of
> bytes to be read, which doesn't work ;)
> 
> This is a Fedora 17 like system on s390. It's a bit special since the kernel
> is 64 bit and whole user space is 32 bit.

LOL, that's creative.  I guess we'll have to give up using
seq_lseek().  Can you please test the following?

Thanks.

diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 382db3c..79b5da2 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -800,7 +800,7 @@ EXPORT_SYMBOL_GPL(sysfs_notify);
 const struct file_operations sysfs_file_operations = {
 	.read		= seq_read,
 	.write		= sysfs_write_file,
-	.llseek		= seq_lseek,
+	.llseek		= generic_file_llseek,
 	.open		= sysfs_open_file,
 	.release	= sysfs_release,
 	.poll		= sysfs_poll,

-- 
tejun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG -next] sysfs change breaks userspace
  2013-11-01 14:35     ` Tejun Heo
@ 2013-11-01 15:04       ` Greg Kroah-Hartman
  2013-11-01 15:08         ` Tejun Heo
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-11-01 15:04 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Heiko Carstens, Kay Sievers, linux-next

On Fri, Nov 01, 2013 at 10:35:42AM -0400, Tejun Heo wrote:
> Hello, Heiko.
> 
> On Fri, Nov 01, 2013 at 03:13:48PM +0100, Heiko Carstens wrote:
> > before your patch it was like this:
> > 
> > [pid  2888] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
> > [pid  2888] lseek(5, 0, SEEK_END)       = 4096
> > [pid  2888] lseek(5, 0, SEEK_SET)       = 0
> > [pid  2888] read(5, "ff:ff:ff:ff:ff:ff\n", 4096) = 18
> > [pid  2888] close(5)                    = 0
> > 
> > With your patch applied I get this:
> > 
> > [pid  2450] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
> > [pid  2450] lseek(5, 0, SEEK_END)       = -1 EINVAL (Invalid argument)
> > [pid  2450] lseek(5, 0, SEEK_SET)       = 0
> > [pid  2450] read(5, 0x557421e8, 4294967295) = -1 EINVAL (Invalid argument)
> > [pid  2450] close(5)                    = 0
> > 
> > So the problem is that lseek with SEEK_END doesn't work.
> > Afterwards the process tried to use the return value of lseek as number of
> > bytes to be read, which doesn't work ;)
> > 
> > This is a Fedora 17 like system on s390. It's a bit special since the kernel
> > is 64 bit and whole user space is 32 bit.
> 
> LOL, that's creative.  I guess we'll have to give up using
> seq_lseek().  Can you please test the following?
> 
> Thanks.
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index 382db3c..79b5da2 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -800,7 +800,7 @@ EXPORT_SYMBOL_GPL(sysfs_notify);
>  const struct file_operations sysfs_file_operations = {
>  	.read		= seq_read,
>  	.write		= sysfs_write_file,
> -	.llseek		= seq_lseek,
> +	.llseek		= generic_file_llseek,

Does that mean that seq_lseek can't handle SEEK_END?  Shouldn't we fix
that instead?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG -next] sysfs change breaks userspace
  2013-11-01 15:04       ` Greg Kroah-Hartman
@ 2013-11-01 15:08         ` Tejun Heo
  2013-11-01 16:14           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2013-11-01 15:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Heiko Carstens, Kay Sievers, linux-next

Hello, Greg.

On Fri, Nov 01, 2013 at 08:04:48AM -0700, Greg Kroah-Hartman wrote:
> > @@ -800,7 +800,7 @@ EXPORT_SYMBOL_GPL(sysfs_notify);
> >  const struct file_operations sysfs_file_operations = {
> >  	.read		= seq_read,
> >  	.write		= sysfs_write_file,
> > -	.llseek		= seq_lseek,
> > +	.llseek		= generic_file_llseek,
> 
> Does that mean that seq_lseek can't handle SEEK_END?  Shouldn't we fix
> that instead?

As the files are dynamic, seq_file has no way of knowing whether the
file is gonna end at all.  It's perfectly valid to have an infinite
file.  The only benefits of using seq_lseek() instead of
generic_file_llseek() are

* Early failure.  For whatever reason, traversing to certain file
  position should fail, seq_lseek() will report such failure on
  seek(2) instead of the following read/write operations.

* EOF detection.  While SEEK_END is not supported, SEEK_SET/CUR +
  large offset can be used to detect eof - eof at the time of the last
  seek anyway as the file size may change dynamically.

I don't think either would matter for sysfs or for prospect kernfs
users and sticking with genefic_file_llseek() should be fine.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [BUG -next] sysfs change breaks userspace
  2013-11-01 15:08         ` Tejun Heo
@ 2013-11-01 16:14           ` Greg Kroah-Hartman
  2013-11-01 17:16             ` [PATCH driver-core-next] sysfs: use generic_file_llseek() for sysfs_file_operations Tejun Heo
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-11-01 16:14 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Heiko Carstens, Kay Sievers, linux-next

On Fri, Nov 01, 2013 at 11:08:00AM -0400, Tejun Heo wrote:
> Hello, Greg.
> 
> On Fri, Nov 01, 2013 at 08:04:48AM -0700, Greg Kroah-Hartman wrote:
> > > @@ -800,7 +800,7 @@ EXPORT_SYMBOL_GPL(sysfs_notify);
> > >  const struct file_operations sysfs_file_operations = {
> > >  	.read		= seq_read,
> > >  	.write		= sysfs_write_file,
> > > -	.llseek		= seq_lseek,
> > > +	.llseek		= generic_file_llseek,
> > 
> > Does that mean that seq_lseek can't handle SEEK_END?  Shouldn't we fix
> > that instead?
> 
> As the files are dynamic, seq_file has no way of knowing whether the
> file is gonna end at all.  It's perfectly valid to have an infinite
> file.  The only benefits of using seq_lseek() instead of
> generic_file_llseek() are
> 
> * Early failure.  For whatever reason, traversing to certain file
>   position should fail, seq_lseek() will report such failure on
>   seek(2) instead of the following read/write operations.
> 
> * EOF detection.  While SEEK_END is not supported, SEEK_SET/CUR +
>   large offset can be used to detect eof - eof at the time of the last
>   seek anyway as the file size may change dynamically.
> 
> I don't think either would matter for sysfs or for prospect kernfs
> users and sticking with genefic_file_llseek() should be fine.

Ok, you are right, that makes sense as sysfs files do have an end,
although it seems odd that userspace would care to try to find it and
not just do a seek to the start of the file.

Fun userspace oddities, just when you thought you had seen it all...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH driver-core-next] sysfs: use generic_file_llseek() for sysfs_file_operations
  2013-11-01 16:14           ` Greg Kroah-Hartman
@ 2013-11-01 17:16             ` Tejun Heo
  2013-11-01 17:19               ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Tejun Heo @ 2013-11-01 17:16 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Heiko Carstens, Kay Sievers, linux-next, linux-kernel

Hey, Greg.

Here's proper patch with description and SOB.  I'll be traveling from
tomorrow so I might not be responsive for some days.  Can you please
apply it once Heiko confirms it fixes the issue?

Thanks!

------- 8< -------
13c589d5b0ac6 ("sysfs: use seq_file when reading regular files")
converted regular sysfs files to use seq_file.  The commit substituted
generic_file_llseek() with seq_lseek() for llseek implementation.

Before the change, all regular sysfs files were allowed to seek to any
position in [0, PAGE_SIZE] as the file size is always PAGE_SIZE and
generic_file_llseek() allows any seeking inside the range under file
size; however, seq_lseek()'s behavior is different.  It traverses the
output by repeatedly invoking ->show() until it reaches the target
offset or traversal indicates EOF.  As seq_files are fully dynamic and
may not end at all, it doesn't support seeking from the end
(SEEK_END).

Apparently, there are userland tools which uses SEEK_END to discover
the buffer size to use and the switch to seq_lseek() disturbs them as
SEEK_END fails with -EINVAL.

The only benefits of using seq_lseek() instead of
generic_file_llseek() are

* Early failure.  If traversing to certain file position should fail,
  seq_lseek() will report such failures on lseek(2) instead of the
  following read/write operations.

* EOF detection.  While SEEK_END is not supported, SEEK_SET/CUR +
  large offset can be used to detect eof - eof at the time of the seek
  anyway as the file size may change dynamically.

Both aren't necessary for sysfs or prospect kernfs users.  Revert to
genefic_file_llseek() and preserve the original behavior.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Link: https://lkml.kernel.org/r/20131031114358.GA5551@osiris
---
 fs/sysfs/file.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -800,7 +800,7 @@ EXPORT_SYMBOL_GPL(sysfs_notify);
 const struct file_operations sysfs_file_operations = {
 	.read		= seq_read,
 	.write		= sysfs_write_file,
-	.llseek		= seq_lseek,
+	.llseek		= generic_file_llseek,
 	.open		= sysfs_open_file,
 	.release	= sysfs_release,
 	.poll		= sysfs_poll,

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH driver-core-next] sysfs: use generic_file_llseek() for sysfs_file_operations
  2013-11-01 17:16             ` [PATCH driver-core-next] sysfs: use generic_file_llseek() for sysfs_file_operations Tejun Heo
@ 2013-11-01 17:19               ` Greg Kroah-Hartman
  2013-11-01 18:40                 ` Heiko Carstens
  0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2013-11-01 17:19 UTC (permalink / raw)
  To: Tejun Heo; +Cc: Heiko Carstens, Kay Sievers, linux-next, linux-kernel

On Fri, Nov 01, 2013 at 01:16:53PM -0400, Tejun Heo wrote:
> Hey, Greg.
> 
> Here's proper patch with description and SOB.  I'll be traveling from
> tomorrow so I might not be responsive for some days.  Can you please
> apply it once Heiko confirms it fixes the issue?

Will do.

Heiko, let me know if this fixes your problem or not, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH driver-core-next] sysfs: use generic_file_llseek() for sysfs_file_operations
  2013-11-01 17:19               ` Greg Kroah-Hartman
@ 2013-11-01 18:40                 ` Heiko Carstens
  0 siblings, 0 replies; 10+ messages in thread
From: Heiko Carstens @ 2013-11-01 18:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Tejun Heo, Kay Sievers, linux-next, linux-kernel

On Fri, Nov 01, 2013 at 10:19:13AM -0700, Greg Kroah-Hartman wrote:
> On Fri, Nov 01, 2013 at 01:16:53PM -0400, Tejun Heo wrote:
> > Hey, Greg.
> > 
> > Here's proper patch with description and SOB.  I'll be traveling from
> > tomorrow so I might not be responsive for some days.  Can you please
> > apply it once Heiko confirms it fixes the issue?
> 
> Will do.
> 
> Heiko, let me know if this fixes your problem or not, thanks.

Yes it does fix the problem. strace output looks like before:

[pid  2495] open("/sys/class/net/eth0/broadcast", O_RDONLY) = 5
[pid  2495] lseek(5, 0, SEEK_END)       = 4096
[pid  2495] lseek(5, 0, SEEK_SET)       = 0
[pid  2495] read(5, "ff:ff:ff:ff:ff:ff\n", 4096) = 18
[pid  2495] close(5)                    = 0

Tested-by: Heiko Carstens <heiko.carstens@de.ibm.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-11-01 18:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-31 11:43 [BUG -next] sysfs change breaks userspace Heiko Carstens
2013-10-31 17:25 ` Tejun Heo
2013-11-01 14:13   ` Heiko Carstens
2013-11-01 14:35     ` Tejun Heo
2013-11-01 15:04       ` Greg Kroah-Hartman
2013-11-01 15:08         ` Tejun Heo
2013-11-01 16:14           ` Greg Kroah-Hartman
2013-11-01 17:16             ` [PATCH driver-core-next] sysfs: use generic_file_llseek() for sysfs_file_operations Tejun Heo
2013-11-01 17:19               ` Greg Kroah-Hartman
2013-11-01 18:40                 ` Heiko Carstens

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.