Re: race in sysfs between sysfs_remove_file() and read()/write() #2

[Greg KH <gregkh@suse.de>]

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