From 94bebf4d1b8e7719f0f3944c037a21cfd99a4af7 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Wed, 20 Dec 2006 10:52:44 +0100 Subject: Driver core: fix race in sysfs between sysfs_remove_file() and read()/write() This patch prevents a race between IO and removing a file from sysfs. It introduces a list of sysfs_buffers associated with a file at the inode. Upon removal of a file the list is walked and the buffers marked orphaned. IO to orphaned buffers fails with -ENODEV. The driver can safely free associated data structures or be unloaded. Signed-off-by: Oliver Neukum Acked-by: Maneesh Soni Signed-off-by: Greg Kroah-Hartman --- fs/sysfs/file.c | 67 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 17 deletions(-) (limited to 'fs/sysfs/file.c') diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 9cfe53e1e00d..cba4c1c7383c 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -50,17 +51,29 @@ static struct sysfs_ops subsys_sysfs_ops = { .store = subsys_attr_store, }; +/** + * add_to_collection - add buffer to a collection + * @buffer: buffer to be added + * @node inode of set to add to + */ -struct sysfs_buffer { - size_t count; - loff_t pos; - char * page; - struct sysfs_ops * ops; - struct semaphore sem; - int needs_read_fill; - int event; -}; +static inline void +add_to_collection(struct sysfs_buffer *buffer, struct inode *node) +{ + struct sysfs_buffer_collection *set = node->i_private; + mutex_lock(&node->i_mutex); + list_add(&buffer->associates, &set->associates); + mutex_unlock(&node->i_mutex); +} + +static inline void +remove_from_collection(struct sysfs_buffer *buffer, struct inode *node) +{ + mutex_lock(&node->i_mutex); + list_del(&buffer->associates); + mutex_unlock(&node->i_mutex); +} /** * fill_read_buffer - allocate and fill buffer from object. @@ -153,6 +166,10 @@ sysfs_read_file(struct file *file, char __user *buf, size_t count, loff_t *ppos) ssize_t retval = 0; down(&buffer->sem); + if (buffer->orphaned) { + retval = -ENODEV; + goto out; + } if (buffer->needs_read_fill) { if ((retval = fill_read_buffer(file->f_path.dentry,buffer))) goto out; @@ -165,7 +182,6 @@ out: return retval; } - /** * fill_write_buffer - copy buffer from userspace. * @buffer: data buffer for file. @@ -243,19 +259,25 @@ sysfs_write_file(struct file *file, const char __user *buf, size_t count, loff_t ssize_t len; down(&buffer->sem); + if (buffer->orphaned) { + len = -ENODEV; + goto out; + } len = fill_write_buffer(buffer, buf, count); if (len > 0) len = flush_write_buffer(file->f_path.dentry, buffer, len); if (len > 0) *ppos += len; +out: up(&buffer->sem); return len; } -static int check_perm(struct inode * inode, struct file * file) +static int sysfs_open_file(struct inode *inode, struct file *file) { struct kobject *kobj = sysfs_get_kobject(file->f_path.dentry->d_parent); struct attribute * attr = to_attr(file->f_path.dentry); + struct sysfs_buffer_collection *set; struct sysfs_buffer * buffer; struct sysfs_ops * ops = NULL; int error = 0; @@ -285,6 +307,18 @@ static int check_perm(struct inode * inode, struct file * file) if (!ops) goto Eaccess; + /* make sure we have a collection to add our buffers to */ + mutex_lock(&inode->i_mutex); + if (!(set = inode->i_private)) { + if (!(set = inode->i_private = kmalloc(sizeof(struct sysfs_buffer_collection), GFP_KERNEL))) { + error = -ENOMEM; + goto Done; + } else { + INIT_LIST_HEAD(&set->associates); + } + } + mutex_unlock(&inode->i_mutex); + /* File needs write support. * The inode's perms must say it's ok, * and we must have a store method. @@ -310,9 +344,11 @@ static int check_perm(struct inode * inode, struct file * file) */ buffer = kzalloc(sizeof(struct sysfs_buffer), GFP_KERNEL); if (buffer) { + INIT_LIST_HEAD(&buffer->associates); init_MUTEX(&buffer->sem); buffer->needs_read_fill = 1; buffer->ops = ops; + add_to_collection(buffer, inode); file->private_data = buffer; } else error = -ENOMEM; @@ -330,11 +366,6 @@ static int check_perm(struct inode * inode, struct file * file) 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 = to_kobj(filp->f_path.dentry->d_parent); @@ -342,6 +373,8 @@ static int sysfs_release(struct inode * inode, struct file * filp) struct module * owner = attr->owner; struct sysfs_buffer * buffer = filp->private_data; + if (buffer) + remove_from_collection(buffer, inode); if (kobj) kobject_put(kobj); /* After this point, attr should not be accessed. */ @@ -548,7 +581,7 @@ EXPORT_SYMBOL_GPL(sysfs_chmod_file); void sysfs_remove_file(struct kobject * kobj, const struct attribute * attr) { - sysfs_hash_and_remove(kobj->dentry,attr->name); + sysfs_hash_and_remove(kobj->dentry, attr->name); } -- cgit v1.2.3 From f75065367077bd3b77842a5aa523ecd05d33e82d Mon Sep 17 00:00:00 2001 From: Mariusz Kozlowski Date: Tue, 2 Jan 2007 13:41:10 +0100 Subject: sysfs: kobject_put cleanup This patch removes redundant argument checks for kobject_put(). Signed-off-by: Mariusz Kozlowski Signed-off-by: Greg Kroah-Hartman --- fs/sysfs/file.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'fs/sysfs/file.c') diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index cba4c1c7383c..46618f81ae48 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -361,7 +361,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file) error = -EACCES; module_put(attr->owner); Done: - if (error && kobj) + if (error) kobject_put(kobj); return error; } @@ -375,8 +375,7 @@ static int sysfs_release(struct inode * inode, struct file * filp) if (buffer) remove_from_collection(buffer, inode); - if (kobj) - kobject_put(kobj); + kobject_put(kobj); /* After this point, attr should not be accessed. */ module_put(owner); -- cgit v1.2.3 From 82244b169ed2eee1ef7f97a3a6693f5a6eff8a69 Mon Sep 17 00:00:00 2001 From: Oliver Neukum Date: Tue, 2 Jan 2007 08:48:08 +0100 Subject: sysfs: error handling in sysfs, fill_read_buffer() if a driver returns an error in fill_read_buffer(), the buffer will be marked as filled. Subsequent reads will return eof. But there is no data because of an error, not because it has been read. Not marking the buffer filled is the obvious fix. Signed-off-by: Oliver Neukum Signed-off-by: Greg Kroah-Hartman --- fs/sysfs/file.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'fs/sysfs/file.c') diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c index 46618f81ae48..c0e117649a4d 100644 --- a/fs/sysfs/file.c +++ b/fs/sysfs/file.c @@ -83,7 +83,8 @@ remove_from_collection(struct sysfs_buffer *buffer, struct inode *node) * Allocate @buffer->page, if it hasn't been already, then call the * kobject's show() method to fill the buffer with this attribute's * data. - * This is called only once, on the file's first read. + * This is called only once, on the file's first read unless an error + * is returned. */ static int fill_read_buffer(struct dentry * dentry, struct sysfs_buffer * buffer) { @@ -101,12 +102,13 @@ static int fill_read_buffer(struct dentry * dentry, struct sysfs_buffer * buffer buffer->event = atomic_read(&sd->s_event); count = ops->show(kobj,attr,buffer->page); - buffer->needs_read_fill = 0; BUG_ON(count > (ssize_t)PAGE_SIZE); - if (count >= 0) + if (count >= 0) { + buffer->needs_read_fill = 0; buffer->count = count; - else + } else { ret = count; + } return ret; } -- cgit v1.2.3