* [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 related [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).