Skip to contentSkip to conversation

writing to sysfs appears to hang

DISCUSSION Paul Larson 4 participants 6 messages Nov 15, 2002 – Nov 20, 2002 Driver core

lore
Share & tools
lore

Paul Larson

author
#0 lore raw

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

#1 lore raw
Quoted 8 lines · Paul Larson [[email protected]] 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
[email protected]

#2 lore raw
Quoted 8 lines · Mike Anderson
Quoted 7 lines · Paul Larson [[email protected]] 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.

Quoted 7 lines
Quoted 1 line

-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 =====

fs/sysfs/inode.c +1 -1
@@ -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)
Signature · 1 line
Jens Axboe
author
#3 lore raw
Quoted 7 lines · Jens Axboe

This has been in the deadline-rbtree patches for some time (uses writes to sysfs, too).

===== fs/sysfs/inode.c 1.59 vs edited =====

fs/sysfs/inode.c line 249

Quoted 8 lines
fs/sysfs/inode.c +1 -1
@@ -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

#4 lore raw
Quoted 14 lines · Paul Larson
Quoted 13 lines · Jens Axboe

This has been in the deadline-rbtree patches for some time (uses writes to sysfs, too).

===== fs/sysfs/inode.c 1.59 vs edited =====

fs/sysfs/inode.c +1 -1
@@ -243,7 +243,7 @@
 	if (kobj && kobj->subsys)
 		ops = kobj->subsys->sysfs_ops;
 	if (!ops || !ops->store)
-		return 0;
+		return -EINVAL;
Quoted 4 lines

fs/sysfs/inode.c line 249

Quoted 2 lines
fs/sysfs/inode.c +0 -0
 	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?

Signature · 1 line
Jens Axboe
#5 lore raw
Quoted 20 lines · Jens Axboe
Quoted 14 lines · Paul Larson
Quoted 13 lines · Jens Axboe

This has been in the deadline-rbtree patches for some time (uses writes to sysfs, too).

===== fs/sysfs/inode.c 1.59 vs edited =====

fs/sysfs/inode.c +1 -1
@@ -243,7 +243,7 @@
 	if (kobj && kobj->subsys)
 		ops = kobj->subsys->sysfs_ops;
 	if (!ops || !ops->store)
-		return 0;
+		return -EINVAL;
Quoted 4 lines

fs/sysfs/inode.c line 249

Quoted 2 lines
fs/sysfs/inode.c +0 -0
 	page = (char *)__get_free_page(GFP_KERNEL);
 	if (!page)

No effect, the behaviour is still the same for me.

Quoted 7 lines

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

fs/sysfs/inode.c +66 -24
@@ -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;