* writing to sysfs appears to hang @ 2002-11-15 23:00 Paul Larson 2002-11-16 0:47 ` Mike Anderson 0 siblings, 1 reply; 6+ messages in thread From: Paul Larson @ 2002-11-15 23:00 UTC (permalink / raw) To: lkml [-- Attachment #1: Type: text/plain, Size: 372 bytes --] I've been playing with sysfs and notices something odd. If I do this: echo 1 > /sys/devices/sys/name the process appears to be hung. ^c won't return control to me. If I log in on another console though, I can't find it running in the process list. All I can do is kill the login process. No kernel errors when I do this, just the hung terminal. -Paul Larson [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 240 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: writing to sysfs appears to hang 2002-11-15 23:00 writing to sysfs appears to hang Paul Larson @ 2002-11-16 0:47 ` Mike Anderson 2002-11-19 17:02 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Mike Anderson @ 2002-11-16 0:47 UTC (permalink / raw) To: Paul Larson; +Cc: lkml Paul Larson [plars@linuxtestproject.org] wrote: > I've been playing with sysfs and notices something odd. If I do this: > echo 1 > /sys/devices/sys/name > the process appears to be hung. ^c won't return control to me. If I > log in on another console though, I can't find it running in the process > list. All I can do is kill the login process. No kernel errors when I > do this, just the hung terminal. > > -Paul Larson I repeated your example and in a quick look at the backtrace the echo is in a loop calling down into sysfs_write_file/dev_attr_store. I think the problem is that if a device does not have a attribute store function the return value from dev_attr_store is incorrect. -andmike -- Michael Anderson andmike@us.ibm.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: writing to sysfs appears to hang 2002-11-16 0:47 ` Mike Anderson @ 2002-11-19 17:02 ` Jens Axboe 2002-11-20 17:24 ` Paul Larson 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2002-11-19 17:02 UTC (permalink / raw) To: Paul Larson, lkml On Fri, Nov 15 2002, Mike Anderson wrote: > Paul Larson [plars@linuxtestproject.org] wrote: > > I've been playing with sysfs and notices something odd. If I do this: > > echo 1 > /sys/devices/sys/name > > the process appears to be hung. ^c won't return control to me. If I > > log in on another console though, I can't find it running in the process > > list. All I can do is kill the login process. No kernel errors when I > > do this, just the hung terminal. > > > > -Paul Larson > > I repeated your example and in a quick look at the backtrace > the echo is in a loop calling down into sysfs_write_file/dev_attr_store. > > I think the problem is that if a device does not have a attribute store > function the return value from dev_attr_store is incorrect. This has been in the deadline-rbtree patches for some time (uses writes to sysfs, too). ===== fs/sysfs/inode.c 1.59 vs edited ===== --- 1.59/fs/sysfs/inode.c Wed Oct 30 21:27:35 2002 +++ edited/fs/sysfs/inode.c Fri Nov 8 14:33:59 2002 @@ -243,7 +243,7 @@ if (kobj && kobj->subsys) ops = kobj->subsys->sysfs_ops; if (!ops || !ops->store) - return 0; + return -EINVAL; page = (char *)__get_free_page(GFP_KERNEL); if (!page) -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: writing to sysfs appears to hang 2002-11-19 17:02 ` Jens Axboe @ 2002-11-20 17:24 ` Paul Larson 2002-11-20 19:14 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Paul Larson @ 2002-11-20 17:24 UTC (permalink / raw) To: Jens Axboe; +Cc: lkml, davej [-- Attachment #1: Type: text/plain, Size: 602 bytes --] On Tue, 2002-11-19 at 11:02, Jens Axboe wrote: > This has been in the deadline-rbtree patches for some time (uses writes > to sysfs, too). > > ===== fs/sysfs/inode.c 1.59 vs edited ===== > --- 1.59/fs/sysfs/inode.c Wed Oct 30 21:27:35 2002 > +++ edited/fs/sysfs/inode.c Fri Nov 8 14:33:59 2002 > @@ -243,7 +243,7 @@ > if (kobj && kobj->subsys) > ops = kobj->subsys->sysfs_ops; > if (!ops || !ops->store) > - return 0; > + return -EINVAL; > > page = (char *)__get_free_page(GFP_KERNEL); > if (!page) No effect, the behaviour is still the same for me. -Paul Larson [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 240 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: writing to sysfs appears to hang 2002-11-20 17:24 ` Paul Larson @ 2002-11-20 19:14 ` Jens Axboe 2002-11-20 19:33 ` Patrick Mochel 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2002-11-20 19:14 UTC (permalink / raw) To: Paul Larson; +Cc: lkml, davej, Patrick Mochel On Wed, Nov 20 2002, Paul Larson wrote: > On Tue, 2002-11-19 at 11:02, Jens Axboe wrote: > > This has been in the deadline-rbtree patches for some time (uses writes > > to sysfs, too). > > > > ===== fs/sysfs/inode.c 1.59 vs edited ===== > > --- 1.59/fs/sysfs/inode.c Wed Oct 30 21:27:35 2002 > > +++ edited/fs/sysfs/inode.c Fri Nov 8 14:33:59 2002 > > @@ -243,7 +243,7 @@ > > if (kobj && kobj->subsys) > > ops = kobj->subsys->sysfs_ops; > > if (!ops || !ops->store) > > - return 0; > > + return -EINVAL; > > > > page = (char *)__get_free_page(GFP_KERNEL); > > if (!page) > > No effect, the behaviour is still the same for me. strace the program then, what is happening? What I saw was a program seeing write() return 0, assuming it didn't write anything, rewrite the whole thing. Repeat. I bet this wouldn't happen if just sysfs didn't allow write open of a file that doesn't have any writeable bits. Smells like another sysfs bug. Pat? -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: writing to sysfs appears to hang 2002-11-20 19:14 ` Jens Axboe @ 2002-11-20 19:33 ` Patrick Mochel 0 siblings, 0 replies; 6+ messages in thread From: Patrick Mochel @ 2002-11-20 19:33 UTC (permalink / raw) To: Jens Axboe; +Cc: Paul Larson, lkml, davej On Wed, 20 Nov 2002, Jens Axboe wrote: > On Wed, Nov 20 2002, Paul Larson wrote: > > On Tue, 2002-11-19 at 11:02, Jens Axboe wrote: > > > This has been in the deadline-rbtree patches for some time (uses writes > > > to sysfs, too). > > > > > > ===== fs/sysfs/inode.c 1.59 vs edited ===== > > > --- 1.59/fs/sysfs/inode.c Wed Oct 30 21:27:35 2002 > > > +++ edited/fs/sysfs/inode.c Fri Nov 8 14:33:59 2002 > > > @@ -243,7 +243,7 @@ > > > if (kobj && kobj->subsys) > > > ops = kobj->subsys->sysfs_ops; > > > if (!ops || !ops->store) > > > - return 0; > > > + return -EINVAL; > > > > > > page = (char *)__get_free_page(GFP_KERNEL); > > > if (!page) > > > > No effect, the behaviour is still the same for me. > > strace the program then, what is happening? What I saw was a program > seeing write() return 0, assuming it didn't write anything, rewrite the > whole thing. Repeat. > > I bet this wouldn't happen if just sysfs didn't allow write open of a > file that doesn't have any writeable bits. Smells like another sysfs > bug. Pat? Yes, it is. The attached patch (relative to current BK) should fix it. I'm planning on sending Linus an update soon, and this is in it. -pat --- linux-2.5-virgin/fs/sysfs/inode.c Wed Nov 20 12:13:06 2002 +++ linux-2.5-kobject/fs/sysfs/inode.c Wed Nov 20 12:15:18 2002 @@ -23,6 +23,8 @@ * Please see Documentation/filesystems/sysfs.txt for more information. */ +#undef DEBUG + #include <linux/list.h> #include <linux/init.h> #include <linux/pagemap.h> @@ -173,17 +175,11 @@ sysfs_read_file(struct file *file, char *buf, size_t count, loff_t *ppos) { struct attribute * attr = file->f_dentry->d_fsdata; - struct sysfs_ops * ops = NULL; - struct kobject * kobj; + struct sysfs_ops * ops = file->private_data; + struct kobject * kobj = file->f_dentry->d_parent->d_fsdata; unsigned char *page; ssize_t retval = 0; - kobj = file->f_dentry->d_parent->d_fsdata; - if (kobj && kobj->subsys) - ops = kobj->subsys->sysfs_ops; - if (!ops || !ops->show) - return 0; - if (count > PAGE_SIZE) count = PAGE_SIZE; @@ -234,16 +230,11 @@ sysfs_write_file(struct file *file, const char *buf, size_t count, loff_t *ppos) { struct attribute * attr = file->f_dentry->d_fsdata; - struct sysfs_ops * ops = NULL; - struct kobject * kobj; + struct sysfs_ops * ops = file->private_data; + struct kobject * kobj = file->f_dentry->d_parent->d_fsdata; ssize_t retval = 0; char * page; - kobj = file->f_dentry->d_parent->d_fsdata; - if (kobj && kobj->subsys) - ops = kobj->subsys->sysfs_ops; - if (!ops || !ops->store) - return -EINVAL; page = (char *)__get_free_page(GFP_KERNEL); if (!page) @@ -275,21 +266,72 @@ return retval; } -static int sysfs_open_file(struct inode * inode, struct file * filp) +static int check_perm(struct inode * inode, struct file * file) { - struct kobject * kobj; + struct kobject * kobj = kobject_get(file->f_dentry->d_parent->d_fsdata); + struct attribute * attr = file->f_dentry->d_fsdata; + struct sysfs_ops * ops = NULL; int error = 0; - kobj = filp->f_dentry->d_parent->d_fsdata; - if ((kobj = kobject_get(kobj))) { - struct attribute * attr = filp->f_dentry->d_fsdata; - if (!attr) - error = -EINVAL; - } else - error = -EINVAL; + if (!kobj || !attr) + goto Einval; + + if (kobj->subsys) + ops = kobj->subsys->sysfs_ops; + + /* No sysfs operations, either from having no subsystem, + * or the subsystem have no operations. + */ + if (!ops) + goto Eaccess; + + /* File needs write support. + * The inode's perms must say it's ok, + * and we must have a store method. + */ + if (file->f_mode & FMODE_WRITE) { + + if (!(inode->i_mode & S_IWUGO)) + goto Eperm; + if (!ops->store) + goto Eaccess; + + } + + /* File needs read support. + * The inode's perms must say it's ok, and we there + * must be a show method for it. + */ + if (file->f_mode & FMODE_READ) { + if (!(inode->i_mode & S_IRUGO)) + goto Eperm; + if (!ops->show) + goto Eaccess; + } + + /* No error? Great, store the ops in file->private_data + * for easy access in the read/write functions. + */ + file->private_data = ops; + goto Done; + + Einval: + error = -EINVAL; + goto Done; + Eaccess: + error = -EACCES; + goto Done; + Eperm: + error = -EPERM; + Done: return error; } +static int sysfs_open_file(struct inode * inode, struct file * filp) +{ + return check_perm(inode,filp); +} + static int sysfs_release(struct inode * inode, struct file * filp) { struct kobject * kobj = filp->f_dentry->d_parent->d_fsdata; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2002-11-20 19:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2002-11-15 23:00 writing to sysfs appears to hang Paul Larson 2002-11-16 0:47 ` Mike Anderson 2002-11-19 17:02 ` Jens Axboe 2002-11-20 17:24 ` Paul Larson 2002-11-20 19:14 ` Jens Axboe 2002-11-20 19:33 ` Patrick Mochel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox