device_remove_file and disconnect

device_remove_file and disconnect

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

Hi,

I have a question about sysfs interface.

If you open a sysfs file created by a module, then remove it (rmmoding 
the module that create this sysfs file), then try to read the opened 
file, you often get strange result (segdefault or oppps).

I attach a small program to test it : open your sysfs file with it 
`wait_read /sysfs/file', rmmod the module, and press enter.

I was wondering if it is to user of sysfs to prevent that (with mutex, 
...) or it is a sysfs bug ?

If it is the first case, I fear that lot's of modules are broken.

Regards,

Matthieu

PS : CC me as I am not subscribed to lkml.


[-- Attachment #2: wait_read.c --]
[-- Type: text/x-csrc, Size: 624 bytes --]

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>

int main(int argc, char *argv[])
{
        char c;
        char buf[1024];
        int fd, i, n;

        if (argc != 2)
                return -1;

        fd = open(argv[1], O_RDONLY);
        if (fd < 0) {
                perror("wait_read - open fail");
                return -1;
        }

        c = getc(stdin);
        n = read(fd, buf, 1024);

        for (i = 0; i < n; i++)
                putc(buf[i], stdout);

        if (n < 0)
                perror("wait_read - read fail");
        else
                putc('\n', stdout);

        close(fd);
}

Re: device_remove_file and disconnect

[Greg KH]

On Wed, Jun 29, 2005 at 06:59:00PM +0200, matthieu castet wrote:
> Hi,
> 
> I have a question about sysfs interface.
> 
> If you open a sysfs file created by a module, then remove it (rmmoding 
> the module that create this sysfs file), then try to read the opened 
> file, you often get strange result (segdefault or oppps).

What file did you do this for?  The module count should be incremented
if you do this, to prevent the module from being unloaded.

> I attach a small program to test it : open your sysfs file with it 
> `wait_read /sysfs/file', rmmod the module, and press enter.
> 
> I was wondering if it is to user of sysfs to prevent that (with mutex, 
> ...) or it is a sysfs bug ?

Driver bug, odds are they don't set the module owner for the attribute
properly.

> If it is the first case, I fear that lot's of modules are broken.

Remember, only root can unload modules, so it really isn't _that_ big of
a deal (I can do a lot more damage as root than just oopsing the
kernel...)

thanks,

greg k-h

Re: device_remove_file and disconnect

[matthieu castet]

Hi,

Greg KH wrote:
> On Wed, Jun 29, 2005 at 06:59:00PM +0200, matthieu castet wrote:
> 
>>Hi,
>>
>>I have a question about sysfs interface.
>>
>>If you open a sysfs file created by a module, then remove it (rmmoding 
>>the module that create this sysfs file), then try to read the opened 
>>file, you often get strange result (segdefault or oppps).
> 
> 
> What file did you do this for?  The module count should be incremented
> if you do this, to prevent the module from being unloaded.
> 
Ok, but if we unplug a device, then disconnect will be called even if we 
opened a sysfs file.

Couldn't be a race between the moment we read our private data and check 
it is valid and the moment we use it :

Process A (read/write sysfs file) 		Process B (disconnect)
recover our private data from struct device
check it is valid
						free our private data
do operation on private data


>>If it is the first case, I fear that lot's of modules are broken.
> 
> 
> Remember, only root can unload modules, so it really isn't _that_ big of
> a deal (I can do a lot more damage as root than just oopsing the
> kernel...)
> 
Yes I know, but fewer possible opps won't hurt ;)

thanks

Matthieu

Re: device_remove_file and disconnect

[Greg KH]

On Wed, Jun 29, 2005 at 10:17:59PM +0200, matthieu castet wrote:
> Hi,
> 
> Greg KH wrote:
> >On Wed, Jun 29, 2005 at 06:59:00PM +0200, matthieu castet wrote:
> >
> >>Hi,
> >>
> >>I have a question about sysfs interface.
> >>
> >>If you open a sysfs file created by a module, then remove it (rmmoding 
> >>the module that create this sysfs file), then try to read the opened 
> >>file, you often get strange result (segdefault or oppps).
> >
> >
> >What file did you do this for?  The module count should be incremented
> >if you do this, to prevent the module from being unloaded.
> >
> Ok, but if we unplug a device, then disconnect will be called even if we 
> opened a sysfs file.

Yes but the device structure will still be in memory, so you will be ok.

> Couldn't be a race between the moment we read our private data and check 
> it is valid and the moment we use it :
> 
> Process A (read/write sysfs file) 		Process B (disconnect)
> recover our private data from struct device
> check it is valid
> 						free our private data
> do operation on private data

No, you should not be freeing your private data on your own.  You should
do that in the device release function.

Again, any specific place in the kernel that you see not doing this?

thanks,

greg k-h

Re: device_remove_file and disconnect

[mat]

Hi,

On Wed, Jun 29, 2005 at 03:42:35PM -0700, Greg KH wrote:
> On Wed, Jun 29, 2005 at 10:17:59PM +0200, matthieu castet wrote:
> > Hi,
> > 
> > Greg KH wrote:
> > >On Wed, Jun 29, 2005 at 06:59:00PM +0200, matthieu castet wrote:
> > >
> > >>Hi,
> > >>
> > >>I have a question about sysfs interface.
> > >>
> > >>If you open a sysfs file created by a module, then remove it (rmmoding 
> > >>the module that create this sysfs file), then try to read the opened 
> > >>file, you often get strange result (segdefault or oppps).
> > >
> > >
> > >What file did you do this for?  The module count should be incremented
> > >if you do this, to prevent the module from being unloaded.
> > >
> > Ok, but if we unplug a device, then disconnect will be called even if we 
> > opened a sysfs file.
> 
> Yes but the device structure will still be in memory, so you will be ok.
> 
disconnect method isn't supposed to clear the device structure in memory ?

> > Couldn't be a race between the moment we read our private data and check 
> > it is valid and the moment we use it :
> > 
> > Process A (read/write sysfs file) 		Process B (disconnect)
> > recover our private data from struct device
> > check it is valid
> > 						free our private data
> > do operation on private data
> 
> No, you should not be freeing your private data on your own.  You should
> do that in the device release function.
But that's what I do !
I free it in  device release function = usb_disconnect in my case =
Process B. Process A is call by sysfs and don't seem serialized with
Process B.
Is there another place where I could free my private data later : I don't
think so.

The problem is that device_remove_file don't block if the sysfs file is
opened. So we must serialized A and B with mutex.
> 
> Again, any specific place in the kernel that you see not doing this?
I believe some drivers expected that sysfs read/write callback are always
called when the device is plugged so they don't check if
to_usb_interface/usb_get_intfdata return valid pointer.

Also I always see driver free their privatre data in device disconnect,
so if read/write from sysfs aren't serialized with device disconnect
there are still a possible race like I show in my example.



thanks,

Matthieu

Re: device_remove_file and disconnect

[Greg KH]

On Thu, Jun 30, 2005 at 09:26:43AM +0200, mat@mut38-1-82-67-62-65.fbx.proxad.net wrote:
> Hi,
> 
> On Wed, Jun 29, 2005 at 03:42:35PM -0700, Greg KH wrote:
> > On Wed, Jun 29, 2005 at 10:17:59PM +0200, matthieu castet wrote:
> > > Hi,
> > > 
> > > Greg KH wrote:
> > > >On Wed, Jun 29, 2005 at 06:59:00PM +0200, matthieu castet wrote:
> > > >
> > > >>Hi,
> > > >>
> > > >>I have a question about sysfs interface.
> > > >>
> > > >>If you open a sysfs file created by a module, then remove it (rmmoding 
> > > >>the module that create this sysfs file), then try to read the opened 
> > > >>file, you often get strange result (segdefault or oppps).
> > > >
> > > >
> > > >What file did you do this for?  The module count should be incremented
> > > >if you do this, to prevent the module from being unloaded.
> > > >
> > > Ok, but if we unplug a device, then disconnect will be called even if we 
> > > opened a sysfs file.
> > 
> > Yes but the device structure will still be in memory, so you will be ok.
> > 
> disconnect method isn't supposed to clear the device structure in memory ?

It all depends on the bus you are talking about :)

> > > Couldn't be a race between the moment we read our private data and check 
> > > it is valid and the moment we use it :
> > > 
> > > Process A (read/write sysfs file) 		Process B (disconnect)
> > > recover our private data from struct device
> > > check it is valid
> > > 						free our private data
> > > do operation on private data
> > 
> > No, you should not be freeing your private data on your own.  You should
> > do that in the device release function.
> But that's what I do !

What type of driver?

> I free it in  device release function = usb_disconnect in my case =
> Process B. Process A is call by sysfs and don't seem serialized with
> Process B.
> Is there another place where I could free my private data later : I don't
> think so.

For usb, no, you are correct.  I was referring to the driver core when
you were mentioning the sysfs stuff, sorry.

Hm, in thinking about it, it might make more sense to rework the usb
core to handle this better.  Possibly add a release() callback to the
driver when the device is actually being freed.  Wouldn't be that hard
to do so, and might cut down on some of the common locking errors.

> The problem is that device_remove_file don't block if the sysfs file is
> opened. So we must serialized A and B with mutex.

Yes.

> > Again, any specific place in the kernel that you see not doing this?
> I believe some drivers expected that sysfs read/write callback are always
> called when the device is plugged so they don't check if
> to_usb_interface/usb_get_intfdata return valid pointer.

Then they should be fixed.  Any specific examples?

> Also I always see driver free their privatre data in device disconnect,
> so if read/write from sysfs aren't serialized with device disconnect
> there are still a possible race like I show in my example.

Yes, you are correct.  Again, any specific drivers you see with this
problem?

thanks,

greg k-h

Re: device_remove_file and disconnect

[matthieu castet]

Hi,

Greg KH wrote:

>>>>Ok, but if we unplug a device, then disconnect will be called even if we 
>>>>opened a sysfs file.
>>>
>>>Yes but the device structure will still be in memory, so you will be ok.
>>>
>>
>>disconnect method isn't supposed to clear the device structure in memory ?
> 
> 
> It all depends on the bus you are talking about :)
> 
> 
usb, but should be for all bus that accept a hot remove of the device...
>>>>Couldn't be a race between the moment we read our private data and check 
>>>>it is valid and the moment we use it :
>>>>
>>>>Process A (read/write sysfs file) 		Process B (disconnect)
>>>>recover our private data from struct device
>>>>check it is valid
>>>>						free our private data
>>>>do operation on private data
>>>
>>>No, you should not be freeing your private data on your own.  You should
>>>do that in the device release function.
>>
>>But that's what I do !
> 
> 
> What type of driver?
> 
> 
a driver for an usb modem.
>>I free it in  device release function = usb_disconnect in my case =
>>Process B. Process A is call by sysfs and don't seem serialized with
>>Process B.
>>Is there another place where I could free my private data later : I don't
>>think so.
> 
> 
> For usb, no, you are correct.  I was referring to the driver core when
> you were mentioning the sysfs stuff, sorry.
> 
> Hm, in thinking about it, it might make more sense to rework the usb
> core to handle this better.  Possibly add a release() callback to the
> driver when the device is actually being freed.  Wouldn't be that hard
> to do so, and might cut down on some of the common locking errors.
> 
> 
So a diconnect when the device is removed and a released when the 
resource counter reach 0 ?


>>I believe some drivers expected that sysfs read/write callback are always
>>called when the device is plugged so they don't check if
>>to_usb_interface/usb_get_intfdata return valid pointer.
> 
> 
> Then they should be fixed.  Any specific examples?
> 
> 
I am a little lasy to list all, but some drivers in driver/usb should 
have this problem : the first driver I look : ./misc/phidgetkit.c do 
[1]. So sysfs read don't check if to_usb_interface or usb_get_intfdata 
return NULL pointer...
And it is a bit your fault, as many developper should have read your 
great tutorial [2] ;)

>>Also I always see driver free their privatre data in device disconnect,
>>so if read/write from sysfs aren't serialized with device disconnect
>>there are still a possible race like I show in my example.
> 
> 
> Yes, you are correct.  Again, any specific drivers you see with this
> problem?
I believe near all drivers that use sysfs via device_create_file, as I 
never see them use mutex in read/write in order to check there aren't in 
the same time in their disconnect that could free there private data 
when they do operation on it...


Couldn't be possible the make device_remove_file blocking until all the 
open file are closed ?

thanks,

Matthieu

[1]
#define show_input(value)       \
static ssize_t show_input##value(struct device *dev, char *buf) \
{                                                                       \
         struct usb_interface *intf = to_usb_interface(dev);             \
         struct phidget_interfacekit *kit = usb_get_intfdata(intf);      \
                                                                         \
         return sprintf(buf, "%d\n", kit->inputs[value - 1]);            \
}                                                                       \
static DEVICE_ATTR(input##value, S_IRUGO, show_input##value, NULL);

[2] http://www.linuxjournal.com/article/7353

Re: device_remove_file and disconnect

[matthieu castet]

Hi,
matthieu castet wrote:
>>
>>
>> Then they should be fixed.  Any specific examples?
>>
>>
> I am a little lasy to list all, but some drivers in driver/usb should 
> have this problem : the first driver I look : ./misc/phidgetkit.c do 
> [1]. So sysfs read don't check if to_usb_interface or usb_get_intfdata 
> return NULL pointer...
> And it is a bit your fault, as many developper should have read your 
> great tutorial [2] ;)
> 
>>> Also I always see driver free their privatre data in device disconnect,
>>> so if read/write from sysfs aren't serialized with device disconnect
>>> there are still a possible race like I show in my example.
>>
>>
>>
>> Yes, you are correct.  Again, any specific drivers you see with this
>> problem?
> 
> I believe near all drivers that use sysfs via device_create_file, as I 
> never see them use mutex in read/write in order to check there aren't in 
> the same time in their disconnect that could free there private data 
> when they do operation on it...
> 
> 
> Couldn't be possible the make device_remove_file blocking until all the 
> open file are closed ?
> 
> thanks,
> 
> Matthieu
> 
> [1]
> #define show_input(value)       \
> static ssize_t show_input##value(struct device *dev, char *buf) \
> {                                                                       \
>         struct usb_interface *intf = to_usb_interface(dev);             \
>         struct phidget_interfacekit *kit = usb_get_intfdata(intf);      \
>                                                                         \
>         return sprintf(buf, "%d\n", kit->inputs[value - 1]);            \
> }                                                                       \
> static DEVICE_ATTR(input##value, S_IRUGO, show_input##value, NULL);
> 
> [2] http://www.linuxjournal.com/article/7353
> 

So will there be a fix in the kernel, or all this driver are broken and 
should be fixed ?

thanks

Matthieu

Re: device_remove_file and disconnect

[Dmitry Torokhov]

On Saturday 02 July 2005 18:27, matthieu castet wrote:
> Hi,
> matthieu castet wrote:
> >>
> >>
> >> Then they should be fixed.  Any specific examples?
> >>
> >>
> > I am a little lasy to list all, but some drivers in driver/usb should 
> > have this problem : the first driver I look : ./misc/phidgetkit.c do 
> > [1]. So sysfs read don't check if to_usb_interface or usb_get_intfdata 
> > return NULL pointer...
> > And it is a bit your fault, as many developper should have read your 
> > great tutorial [2] ;)
> > 
> >>> Also I always see driver free their privatre data in device disconnect,
> >>> so if read/write from sysfs aren't serialized with device disconnect
> >>> there are still a possible race like I show in my example.
> >>
> >>
> >>
> >> Yes, you are correct.  Again, any specific drivers you see with this
> >> problem?
> > 
> > I believe near all drivers that use sysfs via device_create_file, as I 
> > never see them use mutex in read/write in order to check there aren't in 
> > the same time in their disconnect that could free there private data 
> > when they do operation on it...
> > 
> > 
> > Couldn't be possible the make device_remove_file blocking until all the 
> > open file are closed ?
> >

I don't think we want "rmmod <moudule> < /sys/..../attribute" wedge the box.
 
> > thanks,
> > 
> > Matthieu
> > 
> > [1]
> > #define show_input(value)       \
> > static ssize_t show_input##value(struct device *dev, char *buf) \
> > {                                                                       \
> >         struct usb_interface *intf = to_usb_interface(dev);             \
> >         struct phidget_interfacekit *kit = usb_get_intfdata(intf);      \
> >                                                                         \
> >         return sprintf(buf, "%d\n", kit->inputs[value - 1]);            \
> > }                                                                       \
> > static DEVICE_ATTR(input##value, S_IRUGO, show_input##value, NULL);
> > 
> > [2] http://www.linuxjournal.com/article/7353
> > 
> 
> So will there be a fix in the kernel, or all this driver are broken and 
> should be fixed ?
> 

I think the drivers should be fixed, most likely with the help of bus code
they are on.

-- 
Dmitry

Re: device_remove_file and disconnect

[Greg KH]

On Thu, Jun 30, 2005 at 10:31:44PM +0200, matthieu castet wrote:
> >Then they should be fixed.  Any specific examples?
> >
> >
> I am a little lasy to list all, but some drivers in driver/usb should 
> have this problem : the first driver I look : ./misc/phidgetkit.c do 
> [1]. So sysfs read don't check if to_usb_interface or usb_get_intfdata 
> return NULL pointer...
> And it is a bit your fault, as many developper should have read your 
> great tutorial [2] ;)

You are correct, I'll go fix those drivers, thanks for pointing it out.
Others pointed out the same thing this week at OLS :)

> >>Also I always see driver free their privatre data in device disconnect,
> >>so if read/write from sysfs aren't serialized with device disconnect
> >>there are still a possible race like I show in my example.
> >
> >
> >Yes, you are correct.  Again, any specific drivers you see with this
> >problem?
> I believe near all drivers that use sysfs via device_create_file, as I 
> never see them use mutex in read/write in order to check there aren't in 
> the same time in their disconnect that could free there private data 
> when they do operation on it...
> 
> 
> Couldn't be possible the make device_remove_file blocking until all the 
> open file are closed ?

Hm, do we even know that those files are opened then?  I'll poke around
in sysfs and see if we could do that.

thanks,

greg k-h

Re: device_remove_file and disconnect

[Dmitry Torokhov]

On 6/30/05, Greg KH <greg@kroah.com> wrote:
> On Thu, Jun 30, 2005 at 09:26:43AM +0200, mat@mut38-1-82-67-62-65.fbx.proxad.net wrote:
> 
> > > Again, any specific place in the kernel that you see not doing this?
> > I believe some drivers expected that sysfs read/write callback are always
> > called when the device is plugged so they don't check if
> > to_usb_interface/usb_get_intfdata return valid pointer.
> 
> Then they should be fixed.  Any specific examples?
> 

A lot of USB drivers implement sysfs attributes and then to something like this:

static ssize_t show_tabletProductId(struct device *dev, char *buf)
{
        struct aiptek *aiptek = dev_get_drvdata(dev);

        if (aiptek == NULL)
                return 0;

        return snprintf(buf, PAGE_SIZE, "0x%04x\n",
                        aiptek->inputdev->id.product);
}

aiptek structure is freed in aiptek_disconnect. It is possible that
CPU1 just passed that aiptek==NULL check and the task gets
rescheduled. Second CPU will do disconnect and kfree(aiptek).

You really need a semaphore in USB driver core to make sure that
device is not taken from you and that the driver that is bound to the
device is still the same.

-- 
Dmitry

Re: device_remove_file and disconnect

[Dmitry Torokhov]

On Thursday 30 June 2005 12:04, Greg KH wrote:
> Hm, in thinking about it, it might make more sense to rework the usb
> core to handle this better.  Possibly add a release() callback to the
> driver when the device is actually being freed.  Wouldn't be that hard
> to do so, and might cut down on some of the common locking errors.
> 

I amafraid this will not help in this particular case - the problem is
with driver-specific memory which is freed way before device disappears.
Consider unloading aiptek module - we need to free aiptek structure when
we unbinding driver but corrsponding USB device will stay there (fully
fucntional with its 'struct device' not going anywhere) until you
physically pull tablet's cord out.

-- 
Dmitry