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

[Oliver Neukum <oliver@neukum.org>]

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