Hi,
Alan Stern has discovered a race in sysfs, whereby driver callbacks could be
called after sysfs_remove_file() has run. The attached patch should fix it.
It introduces a new data structure acting as a collection of all sysfs_buffers
associated with an attribute. Upon removal of an attribute the buffers are
marked orphaned and IO on them returns -ENODEV. Thus sysfs_remove_file()
makes sure that sysfs won't bother a driver after that call, making it safe
to free the associated data structures and to unload the driver.
Regards
Oliver
On Fri, Dec 01, 2006 at 11:43:06PM +0100, Oliver Neukum wrote:
> Hi,
>
> Alan Stern has discovered a race in sysfs, whereby driver callbacks could be
> called after sysfs_remove_file() has run. The attached patch should fix it.
>
> It introduces a new data structure acting as a collection of all sysfs_buffers
> associated with an attribute. Upon removal of an attribute the buffers are
> marked orphaned and IO on them returns -ENODEV. Thus sysfs_remove_file()
> makes sure that sysfs won't bother a driver after that call, making it safe
> to free the associated data structures and to unload the driver.
>
> Regards
> Oliver
Hi Oliver,
Thanks for the explaining the patch but some description about the race
would also help here. At the least the callpath to the race would be useful.
Thanks
Maneesh
> --- linux-2.6.19-rc6/fs/sysfs/file.c 2006-11-25 23:49:07.000000000 +0100
> +++ sysfs/fs/sysfs/file.c 2006-12-01 09:47:24.000000000 +0100
> @@ -7,6 +7,7 @@
> #include <linux/kobject.h>
> #include <linux/namei.h>
> #include <linux/poll.h>
> +#include <linux/list.h>
> #include <asm/uaccess.h>
> #include <asm/semaphore.h>
>
> @@ -50,17 +51,29 @@
> .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 @@
> 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_dentry,buffer)))
> goto out;
> @@ -165,7 +182,6 @@
> return retval;
> }
>
> -
> /**
> * fill_write_buffer - copy buffer from userspace.
> * @buffer: data buffer for file.
> @@ -240,19 +256,25 @@
> 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_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_dentry->d_parent);
> struct attribute * attr = to_attr(file->f_dentry);
> + struct sysfs_buffer_collection *set;
> struct sysfs_buffer * buffer;
> struct sysfs_ops * ops = NULL;
> int error = 0;
> @@ -282,6 +304,18 @@
> 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.
> @@ -307,9 +341,11 @@
> */
> 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;
> @@ -327,11 +363,6 @@
> 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_dentry->d_parent);
> @@ -339,6 +370,8 @@
> 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. */
> @@ -545,7 +578,9 @@
>
> void sysfs_remove_file(struct kobject * kobj, const struct attribute * attr)
> {
> - sysfs_hash_and_remove(kobj->dentry,attr->name);
> + struct dentry *d = kobj->dentry;
> +
> + sysfs_hash_and_remove(d, attr->name);
> }
>
>
> --- linux-2.6.19-rc6/fs/sysfs/sysfs.h 2006-11-16 05:03:40.000000000 +0100
> +++ sysfs/fs/sysfs/sysfs.h 2006-12-01 09:47:44.000000000 +0100
> @@ -33,6 +33,22 @@
> struct kobject * target_kobj;
> };
>
> +struct sysfs_buffer {
> + struct list_head associates;
> + size_t count;
> + loff_t pos;
> + char * page;
> + struct sysfs_ops * ops;
> + struct semaphore sem;
> + int orphaned;
> + int needs_read_fill;
> + int event;
> +};
> +
> +struct sysfs_buffer_collection {
> + struct list_head associates;
> +};
> +
> static inline struct kobject * to_kobj(struct dentry * dentry)
> {
> struct sysfs_dirent * sd = dentry->d_fsdata;
> @@ -95,4 +111,3 @@
> if (atomic_dec_and_test(&sd->s_count))
> release_sysfs_dirent(sd);
> }
> -
> --- linux-2.6.19-rc6/fs/sysfs/mount.c 2006-11-16 05:03:40.000000000 +0100
> +++ sysfs/fs/sysfs/mount.c 2006-11-26 14:00:13.000000000 +0100
> @@ -18,9 +18,12 @@
> struct super_block * sysfs_sb = NULL;
> kmem_cache_t *sysfs_dir_cachep;
>
> +static void sysfs_clear_inode(struct inode *inode);
> +
> static struct super_operations sysfs_ops = {
> .statfs = simple_statfs,
> .drop_inode = generic_delete_inode,
> + .clear_inode = sysfs_clear_inode,
> };
>
> static struct sysfs_dirent sysfs_root = {
> @@ -31,6 +34,11 @@
> .s_iattr = NULL,
> };
>
> +static void sysfs_clear_inode(struct inode *inode)
> +{
> + kfree(inode->i_private);
> +}
> +
> static int sysfs_fill_super(struct super_block *sb, void *data, int silent)
> {
> struct inode *inode;
> --- linux-2.6.19-rc6/fs/sysfs/inode.c 2006-11-16 05:03:40.000000000 +0100
> +++ sysfs/fs/sysfs/inode.c 2006-12-01 09:48:11.000000000 +0100
> @@ -209,6 +209,22 @@
> return NULL;
> }
>
> +static inline void orphan_all_buffers(struct inode *node)
> +{
> + struct sysfs_buffer_collection *set = node->i_private;
> + struct sysfs_buffer *buf;
> +
> + mutex_lock(&node->i_mutex);
> + if (node->i_private) {
> + list_for_each_entry(buf, &set->associates, associates) {
> + down(&buf->sem);
> + buf->orphaned = 1;
> + up(&buf->sem);
> + }
> + }
> + mutex_unlock(&node->i_mutex);
> +}
> +
>
> /*
> * Unhashes the dentry corresponding to given sysfs_dirent
> @@ -217,16 +233,23 @@
> void sysfs_drop_dentry(struct sysfs_dirent * sd, struct dentry * parent)
> {
> struct dentry * dentry = sd->s_dentry;
> + struct inode *inode;
>
> if (dentry) {
> spin_lock(&dcache_lock);
> spin_lock(&dentry->d_lock);
> if (!(d_unhashed(dentry) && dentry->d_inode)) {
> + inode = dentry->d_inode;
> + spin_lock(&inode->i_lock);
> + __iget(inode);
> + spin_unlock(&inode->i_lock);
> dget_locked(dentry);
> __d_drop(dentry);
> spin_unlock(&dentry->d_lock);
> spin_unlock(&dcache_lock);
> simple_unlink(parent->d_inode, dentry);
> + orphan_all_buffers(inode);
> + iput(inode);
> } else {
> spin_unlock(&dentry->d_lock);
> spin_unlock(&dcache_lock);
Am Montag, 4. Dezember 2006 05:43 schrieb Maneesh Soni:
> On Fri, Dec 01, 2006 at 11:43:06PM +0100, Oliver Neukum wrote:
> > Hi,
> >
> > Alan Stern has discovered a race in sysfs, whereby driver callbacks could be
> > called after sysfs_remove_file() has run. The attached patch should fix it.
> >
> > It introduces a new data structure acting as a collection of all sysfs_buffers
> > associated with an attribute. Upon removal of an attribute the buffers are
> > marked orphaned and IO on them returns -ENODEV. Thus sysfs_remove_file()
> > makes sure that sysfs won't bother a driver after that call, making it safe
> > to free the associated data structures and to unload the driver.
> >
> > Regards
> > Oliver
>
> Hi Oliver,
>
> Thanks for the explaining the patch but some description about the race
> would also help here. At the least the callpath to the race would be useful.
>
>
> Thanks
> Maneesh
We have code like this:
static void tv_disconnect(struct usb_interface *interface)
{
struct trancevibrator *dev;
dev = usb_get_intfdata (interface);
device_remove_file(&interface->dev, &dev_attr_speed);
usb_set_intfdata(interface, NULL);
usb_put_dev(dev->udev);
kfree(dev);
}
This has a race:
CPU A CPU B
open sysfs
device_remove_file
kfree
reading attr
We cannot do refcounting as sysfs doesn't export open/close. Therefore
we must be sure that device_remove_file() makes sure that sysfs will
leave a driver alone after the return of device_remove_file(). Currently
open will fail, but IO on an already opened file will work. The patch makes
sure it will fail with -ENODEV without calling into the driver, which may
indeed be already unloaded.
Regards
Oliver
On Mon, Dec 04, 2006 at 07:38:00AM +0100, Oliver Neukum wrote:
> Am Montag, 4. Dezember 2006 05:43 schrieb Maneesh Soni:
> > On Fri, Dec 01, 2006 at 11:43:06PM +0100, Oliver Neukum wrote:
> > > Hi,
> > >
> > > Alan Stern has discovered a race in sysfs, whereby driver callbacks could be
> > > called after sysfs_remove_file() has run. The attached patch should fix it.
> > >
> > > It introduces a new data structure acting as a collection of all sysfs_buffers
> > > associated with an attribute. Upon removal of an attribute the buffers are
> > > marked orphaned and IO on them returns -ENODEV. Thus sysfs_remove_file()
> > > makes sure that sysfs won't bother a driver after that call, making it safe
> > > to free the associated data structures and to unload the driver.
> > >
> > > Regards
> > > Oliver
> >
> > Hi Oliver,
> >
> > Thanks for the explaining the patch but some description about the race
> > would also help here. At the least the callpath to the race would be useful.
> >
> >
> > Thanks
> > Maneesh
>
> We have code like this:
> static void tv_disconnect(struct usb_interface *interface)
> {
> struct trancevibrator *dev;
>
> dev = usb_get_intfdata (interface);
> device_remove_file(&interface->dev, &dev_attr_speed);
> usb_set_intfdata(interface, NULL);
> usb_put_dev(dev->udev);
> kfree(dev);
> }
>
> This has a race:
>
> CPU A CPU B
> open sysfs
> device_remove_file
> kfree
> reading attr
>
> We cannot do refcounting as sysfs doesn't export open/close. Therefore
> we must be sure that device_remove_file() makes sure that sysfs will
> leave a driver alone after the return of device_remove_file(). Currently
> open will fail, but IO on an already opened file will work. The patch makes
> sure it will fail with -ENODEV without calling into the driver, which may
> indeed be already unloaded.
>
> Regards
> Oliver
hmm, I guess Greg has to say the final word. The question is either to fail
the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
fail the removal then your patch is the way to go.
Greg?
Thanks
Maneesh
Am Montag, 4. Dezember 2006 14:04 schrieb Maneesh Soni:
> > > Hi Oliver,
> > >
> > > Thanks for the explaining the patch but some description about the race
> > > would also help here. At the least the callpath to the race would be useful.
> > >
> > >
> > > Thanks
> > > Maneesh
> >
> > We have code like this:
> > ?static void tv_disconnect(struct usb_interface *interface)
> > {
> > ??????struct trancevibrator *dev;
> >
> > ??????dev = usb_get_intfdata (interface);
> > ??????device_remove_file(&interface->dev, &dev_attr_speed);
> > ??????usb_set_intfdata(interface, NULL);
> > ??????usb_put_dev(dev->udev);
> > ??????kfree(dev);
> > }
> >
> > This has a race:
> >
> > CPU A?????????????????????????CPU B
> > open sysfs
> > ??????????????????????????????????????device_remove_file
> > ??????????????????????????????????????kfree
> > reading attr
> >
> > We cannot do refcounting as sysfs doesn't export open/close. Therefore
> > we must be sure that device_remove_file() makes sure that sysfs will
> > leave a driver alone after the return of device_remove_file(). Currently
> > open will fail, but IO on an already opened file will work. The patch makes
> > sure it will fail with -ENODEV without calling into the driver, which may
> > indeed be already unloaded.
> >
> > ??????Regards
> > ??????????????Oliver
>
> hmm, I guess Greg has to say the final word. The question is either to fail
> the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
> fail the removal then your patch is the way to go.
Failing the removal is problematic. This happens in the disconnect()
code path, which cannot fail in a benign way. Plus, if we do so, the
module refcounting in sysfs is incorrect, that is too early.
Regards
Oliver
On Mon, 4 Dec 2006, Maneesh Soni wrote:
> hmm, I guess Greg has to say the final word. The question is either to fail
> the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
> fail the removal then your patch is the way to go.
>
> Greg?
Oliver is right that we cannot allow device_remove_file() to fail. In
fact we can't even allow it to block until all the existing open file
references are closed.
Our major questions have to do with the details of the patch itself. In
particular, we are worried about possible races with the VFS and the
handling of the inode's usage count. Can you examine the patch carefully
to see if it is okay?
Also, Oliver, it looks like the latest version of your patch makes an
unnecessary change to sysfs_remove_file().
Alan Stern
Am Montag, 4. Dezember 2006 17:06 schrieb Alan Stern:
> On Mon, 4 Dec 2006, Maneesh Soni wrote:
>
> > hmm, I guess Greg has to say the final word. The question is either to fail
> > the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
> > fail the removal then your patch is the way to go.
> >
> > Greg?
>
> Oliver is right that we cannot allow device_remove_file() to fail. In
> fact we can't even allow it to block until all the existing open file
> references are closed.
Yes, we must have an upper bound with respect to time.
> Our major questions have to do with the details of the patch itself. In
> particular, we are worried about possible races with the VFS and the
> handling of the inode's usage count. Can you examine the patch carefully
> to see if it is okay?
>
> Also, Oliver, it looks like the latest version of your patch makes an
> unnecessary change to sysfs_remove_file().
Code like:
int d(int a, int b)
{
return a + b;
}
int c(int a, int b)
{
return d(a, b);
}
is a detrimental to correct understanding and thence coding.
In fact reading sysfs source code is like jumping all around the kernel
tree. Such changes made it readable by normal people. I have to
understand which method I am coding on to do reasonable work. ;-)
Regards
Oliver
On Mon, 4 Dec 2006, Oliver Neukum wrote:
> > Also, Oliver, it looks like the latest version of your patch makes an
> > unnecessary change to sysfs_remove_file().
>
> Code like:
>
> int d(int a, int b)
> {
> return a + b;
> }
>
> int c(int a, int b)
> {
> return d(a, b);
> }
>
> is a detrimental to correct understanding and thence coding.
> In fact reading sysfs source code is like jumping all around the kernel
> tree. Such changes made it readable by normal people. I have to
> understand which method I am coding on to do reasonable work. ;-)
I was referring to sysfs_remove_file(), not sysfs_open_file() -- I agree
that getting rid of the check_perm() routine is good. But this isn't:
> void sysfs_remove_file(struct kobject * kobj, const struct attribute * attr)
> {
> - sysfs_hash_and_remove(kobj->dentry,attr->name);
> + struct dentry *d = kobj->dentry;
> +
> + sysfs_hash_and_remove(d, attr->name);
> }
There's no apparent advantage to introducing the local variable d, either
in terms of execution speed or readability. (Although the original source
line should have a space after the comma.)
Alan Stern
Am Montag, 4. Dezember 2006 17:57 schrieb Alan Stern:
> I was referring to sysfs_remove_file(), not sysfs_open_file() -- I agree
> that getting rid of the check_perm() routine is good. But this isn't:
>
> > void sysfs_remove_file(struct kobject * kobj, const struct attribute * attr)
> > {
> > - sysfs_hash_and_remove(kobj->dentry,attr->name);
> > + struct dentry *d = kobj->dentry;
> > +
> > + sysfs_hash_and_remove(d, attr->name);
> > }
>
> There's no apparent advantage to introducing the local variable d, either
> in terms of execution speed or readability. (Although the original source
> line should have a space after the comma.)
Yes, correct, it is a remainder of using the dentry twice in that routine.
Then a local variable saved a recomputation. I can redo it, sorry.
However, it doesn't affect correctness, so I won't distract further by
doing an essentially cosmetic change.
Regards
Oliver
On Mon, Dec 04, 2006 at 06:34:06PM +0530, Maneesh Soni wrote:
> On Mon, Dec 04, 2006 at 07:38:00AM +0100, Oliver Neukum wrote:
> > Am Montag, 4. Dezember 2006 05:43 schrieb Maneesh Soni:
> > > On Fri, Dec 01, 2006 at 11:43:06PM +0100, Oliver Neukum wrote:
> > > > Hi,
> > > >
> > > > Alan Stern has discovered a race in sysfs, whereby driver callbacks could be
> > > > called after sysfs_remove_file() has run. The attached patch should fix it.
> > > >
> > > > It introduces a new data structure acting as a collection of all sysfs_buffers
> > > > associated with an attribute. Upon removal of an attribute the buffers are
> > > > marked orphaned and IO on them returns -ENODEV. Thus sysfs_remove_file()
> > > > makes sure that sysfs won't bother a driver after that call, making it safe
> > > > to free the associated data structures and to unload the driver.
> > > >
> > > > Regards
> > > > Oliver
> > >
> > > Hi Oliver,
> > >
> > > Thanks for the explaining the patch but some description about the race
> > > would also help here. At the least the callpath to the race would be useful.
> > >
> > >
> > > Thanks
> > > Maneesh
> >
> > We have code like this:
> > static void tv_disconnect(struct usb_interface *interface)
> > {
> > struct trancevibrator *dev;
> >
> > dev = usb_get_intfdata (interface);
> > device_remove_file(&interface->dev, &dev_attr_speed);
> > usb_set_intfdata(interface, NULL);
> > usb_put_dev(dev->udev);
> > kfree(dev);
> > }
> >
> > This has a race:
> >
> > CPU A CPU B
> > open sysfs
> > device_remove_file
> > kfree
> > reading attr
> >
> > We cannot do refcounting as sysfs doesn't export open/close. Therefore
> > we must be sure that device_remove_file() makes sure that sysfs will
> > leave a driver alone after the return of device_remove_file(). Currently
> > open will fail, but IO on an already opened file will work. The patch makes
> > sure it will fail with -ENODEV without calling into the driver, which may
> > indeed be already unloaded.
> >
> > Regards
> > Oliver
>
> hmm, I guess Greg has to say the final word. The question is either to fail
> the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
> fail the removal then your patch is the way to go.
We can't fail the removal, as we just aren't allowed to do that at times
(the device is physically removed).
So failing the IO is the way to go.
thanks,
greg k-h
On Mon, Dec 04, 2006 at 11:06:41AM -0500, Alan Stern wrote:
> On Mon, 4 Dec 2006, Maneesh Soni wrote:
>
> > hmm, I guess Greg has to say the final word. The question is either to fail
> > the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
> > fail the removal then your patch is the way to go.
> >
> > Greg?
>
> Oliver is right that we cannot allow device_remove_file() to fail. In
> fact we can't even allow it to block until all the existing open file
> references are closed.
>
> Our major questions have to do with the details of the patch itself. In
> particular, we are worried about possible races with the VFS and the
> handling of the inode's usage count. Can you examine the patch carefully
> to see if it is okay?
>
Sorry for late reply.. I reviewed the patch and it looks ok me.
Thanks
Maneesh
On Mon, Dec 11, 2006 at 04:13:06PM +0530, Maneesh Soni wrote:
> On Mon, Dec 04, 2006 at 11:06:41AM -0500, Alan Stern wrote:
> > On Mon, 4 Dec 2006, Maneesh Soni wrote:
> >
> > > hmm, I guess Greg has to say the final word. The question is either to fail
> > > the IO (-ENODEV) or fail the file removal (-EBUSY). If we are not going to
> > > fail the removal then your patch is the way to go.
> > >
> > > Greg?
> >
> > Oliver is right that we cannot allow device_remove_file() to fail. In
> > fact we can't even allow it to block until all the existing open file
> > references are closed.
> >
> > Our major questions have to do with the details of the patch itself. In
> > particular, we are worried about possible races with the VFS and the
> > handling of the inode's usage count. Can you examine the patch carefully
> > to see if it is okay?
> >
>
> Sorry for late reply.. I reviewed the patch and it looks ok me.
Thanks for the review. Oliver, care to resend it to me so I can give it
some testing in the -mm tree?
thanks,
greg k-h