[BUG] kobject module-ref race-condition or unsafe module_put(THIS_MODULE)

[BUG] kobject module-ref race-condition or unsafe module_put(THIS_MODULE)

[David Herrmann]

Hi

I currently do not understand how kobjects keep a reference to the
owning module. Lets assume I provide a "release" method via a
kobj_type for my kobject. I want to go sure my module is still alive
when this method is called. Otherwise, this "release" method would be
no longer available and we would jump into invalid memory. Therefore,
I need to take a reference to my own module. This seems trivial.
However, how do I release this reference again?
The most simple solution might be calling module_put(THIS_MODULE) in
my "release" method. However, if this call drops the module-refcount
to 0 and immediately removes the module, the module_put() returns to
my "release" function which now is no longer available.

It seems quite unlikely that the cleanup of a module is faster than
two function-returns, however, theoretically there is a race
condition.

The following example is based on fs/char_dev.c. Lets assume my module
provides a  kobject structure. On init I take a ref to myself with
try_module_get(THIS_MODULE).
I add my own kobj_type with the following release function:

static void mydev_put(struct mydev *p)
{
	if (p) {
		kobject_put(&p->kobj);
		module_put(THIS_MODULE);
	}
}

How can we go sure that module_put() doesn't free my own module before
it returns? Isn't a call to module_put(THIS_MODULE) always unsafe
(unless I own at least two references)?

grep "module_put(THIS_MODULE)" -r ./kernel_src/ | wc -l
returns 118 results.
I doubt that all these 118 places can got sure that they own at least
2 references when calling this, otherwise it would be quite unsafe to
call this function.


Maybe I have missed some important fact here, but this seems quite
unsafe to me. Adding a "owner" field to a kobj_type would fix that
issue for kobjects/devices.

Regards
David

Re: [BUG] kobject module-ref race-condition or unsafe module_put(THIS_MODULE)

[Greg KH]

On Sat, Oct 29, 2011 at 07:52:47PM +0200, David Herrmann wrote:
> Hi
> 
> I currently do not understand how kobjects keep a reference to the
> owning module. Lets assume I provide a "release" method via a
> kobj_type for my kobject. I want to go sure my module is still alive
> when this method is called. Otherwise, this "release" method would be
> no longer available and we would jump into invalid memory. Therefore,
> I need to take a reference to my own module. This seems trivial.
> However, how do I release this reference again?
> The most simple solution might be calling module_put(THIS_MODULE) in
> my "release" method. However, if this call drops the module-refcount
> to 0 and immediately removes the module, the module_put() returns to
> my "release" function which now is no longer available.
> 
> It seems quite unlikely that the cleanup of a module is faster than
> two function-returns, however, theoretically there is a race
> condition.
> 
> The following example is based on fs/char_dev.c. Lets assume my module
> provides a  kobject structure. On init I take a ref to myself with
> try_module_get(THIS_MODULE).

No, never do that, why would you?

> I add my own kobj_type with the following release function:
> 
> static void mydev_put(struct mydev *p)
> {
> 	if (p) {
> 		kobject_put(&p->kobj);
> 		module_put(THIS_MODULE);
> 	}
> }

Ick, don't do that.

> How can we go sure that module_put() doesn't free my own module before
> it returns? Isn't a call to module_put(THIS_MODULE) always unsafe
> (unless I own at least two references)?

Yes, that's why you shouldn't be doing this :)

> Maybe I have missed some important fact here, but this seems quite
> unsafe to me. Adding a "owner" field to a kobj_type would fix that
> issue for kobjects/devices.

No, let's determine exactly what you are trying to do first, and why in
the world you are dealing with "raw" kobjects.  You should almost never
never never do that.

What driver are you writing?  Have a pointer to the code somewhere?

greg k-h

Re: [BUG] kobject module-ref race-condition or unsafe module_put(THIS_MODULE)

[David Herrmann]

On Sun, Oct 30, 2011 at 12:39 AM, Greg KH <gregkh@suse.de> wrote:
> On Sat, Oct 29, 2011 at 07:52:47PM +0200, David Herrmann wrote:
>> Hi
>>
>> I currently do not understand how kobjects keep a reference to the
>> owning module. Lets assume I provide a "release" method via a
>> kobj_type for my kobject. I want to go sure my module is still alive
>> when this method is called. Otherwise, this "release" method would be
>> no longer available and we would jump into invalid memory. Therefore,
>> I need to take a reference to my own module. This seems trivial.
>> However, how do I release this reference again?
>> The most simple solution might be calling module_put(THIS_MODULE) in
>> my "release" method. However, if this call drops the module-refcount
>> to 0 and immediately removes the module, the module_put() returns to
>> my "release" function which now is no longer available.
>>
>> It seems quite unlikely that the cleanup of a module is faster than
>> two function-returns, however, theoretically there is a race
>> condition.
>>
>> The following example is based on fs/char_dev.c. Lets assume my module
>> provides a  kobject structure. On init I take a ref to myself with
>> try_module_get(THIS_MODULE).
>
> No, never do that, why would you?
>
>> I add my own kobj_type with the following release function:
>>
>> static void mydev_put(struct mydev *p)
>> {
>>       if (p) {
>>               kobject_put(&p->kobj);
>>               module_put(THIS_MODULE);
>>       }
>> }
>
> Ick, don't do that.
>
>> How can we go sure that module_put() doesn't free my own module before
>> it returns? Isn't a call to module_put(THIS_MODULE) always unsafe
>> (unless I own at least two references)?
>
> Yes, that's why you shouldn't be doing this :)
>
>> Maybe I have missed some important fact here, but this seems quite
>> unsafe to me. Adding a "owner" field to a kobj_type would fix that
>> issue for kobjects/devices.
>
> No, let's determine exactly what you are trying to do first, and why in
> the world you are dealing with "raw" kobjects.  You should almost never
> never never do that.
>
> What driver are you writing?  Have a pointer to the code somewhere?

Initially, I was looking at hci_dev at net/bluetooth/hci_core.c and
hci_sysfs.c. It registers a "struct device" with a device_type
structure and a static release function. I was wondering how I can be
sure that the "release" function is still available when it is called.
I couldn't find any module reference in "struct device" nor can I be
sure that the module is still loaded when the device is freed.

There are several bugfixes in padovan's tree and pending on the ML so
I can't point you to the source. Reading net/bluetooth/hci* really
doesn't help here. However, what should I do in the following case:

I provide hci_alloc_dev() which creates a hci_dev with an embedded
"struct device" and hci_dev_get/put which map to get/put_device(). The
device is registered with a "device_type" including a "release"
callback which simply calls kfree() on the hci_dev.
Now how can I be sure the "release" callback pointing to my function
is still available when a device is freed? When unloading the
bluetooth module I cannot wait for all hci_dev structures to be
freed(). This would make ref-counts useless. What is the recommended
way to do that?

Also, module_put(THIS_MODULE) is used quite frequently in the tree.
Some cases use it in error-paths where they actually know they have
another reference. But quite many places use it the wrong way. See for
instance drivers/watchdog/softdog.c.

> greg k-h
>

I am confused. I don't know how to protect "struct device" structures
as they don't have an "owner" field.

Regards
David

Re: [BUG] kobject module-ref race-condition or unsafe module_put(THIS_MODULE)

[Greg KH]

On Sun, Oct 30, 2011 at 01:36:08AM +0200, David Herrmann wrote:
> On Sun, Oct 30, 2011 at 12:39 AM, Greg KH <gregkh@suse.de> wrote:
> > On Sat, Oct 29, 2011 at 07:52:47PM +0200, David Herrmann wrote:
> >> Hi
> >>
> >> I currently do not understand how kobjects keep a reference to the
> >> owning module. Lets assume I provide a "release" method via a
> >> kobj_type for my kobject. I want to go sure my module is still alive
> >> when this method is called. Otherwise, this "release" method would be
> >> no longer available and we would jump into invalid memory. Therefore,
> >> I need to take a reference to my own module. This seems trivial.
> >> However, how do I release this reference again?
> >> The most simple solution might be calling module_put(THIS_MODULE) in
> >> my "release" method. However, if this call drops the module-refcount
> >> to 0 and immediately removes the module, the module_put() returns to
> >> my "release" function which now is no longer available.
> >>
> >> It seems quite unlikely that the cleanup of a module is faster than
> >> two function-returns, however, theoretically there is a race
> >> condition.
> >>
> >> The following example is based on fs/char_dev.c. Lets assume my module
> >> provides a  kobject structure. On init I take a ref to myself with
> >> try_module_get(THIS_MODULE).
> >
> > No, never do that, why would you?
> >
> >> I add my own kobj_type with the following release function:
> >>
> >> static void mydev_put(struct mydev *p)
> >> {
> >>       if (p) {
> >>               kobject_put(&p->kobj);
> >>               module_put(THIS_MODULE);
> >>       }
> >> }
> >
> > Ick, don't do that.
> >
> >> How can we go sure that module_put() doesn't free my own module before
> >> it returns? Isn't a call to module_put(THIS_MODULE) always unsafe
> >> (unless I own at least two references)?
> >
> > Yes, that's why you shouldn't be doing this :)
> >
> >> Maybe I have missed some important fact here, but this seems quite
> >> unsafe to me. Adding a "owner" field to a kobj_type would fix that
> >> issue for kobjects/devices.
> >
> > No, let's determine exactly what you are trying to do first, and why in
> > the world you are dealing with "raw" kobjects.  You should almost never
> > never never do that.
> >
> > What driver are you writing?  Have a pointer to the code somewhere?
> 
> Initially, I was looking at hci_dev at net/bluetooth/hci_core.c and
> hci_sysfs.c. It registers a "struct device" with a device_type
> structure and a static release function. I was wondering how I can be
> sure that the "release" function is still available when it is called.

Ah, so you are a bus, that's good, that is the only thing that should be
caring about this.

> I couldn't find any module reference in "struct device" nor can I be
> sure that the module is still loaded when the device is freed.

That's because a struct device doesn't care about a module, it's the
larger structure that wraps it that should, if it really needs to.

Look at how USB and PCI handle this, that's the way to properly do this
if you really feel you need to do so.

> There are several bugfixes in padovan's tree and pending on the ML so
> I can't point you to the source. Reading net/bluetooth/hci* really
> doesn't help here. However, what should I do in the following case:
> 
> I provide hci_alloc_dev() which creates a hci_dev with an embedded
> "struct device" and hci_dev_get/put which map to get/put_device(). The
> device is registered with a "device_type" including a "release"
> callback which simply calls kfree() on the hci_dev.

Good.

> Now how can I be sure the "release" callback pointing to my function
> is still available when a device is freed? When unloading the
> bluetooth module I cannot wait for all hci_dev structures to be
> freed().

Why not?  That sounds like the correct thing to do, right?  It's what
the network stack does.

> This would make ref-counts useless. What is the recommended
> way to do that?

No, please don't confuse module counts with reference counts of devices,
they are two totally different things.

Device reference counts count the users of the memory for the device.

Module reference counts count the users of the memory for the code for
the module.

Usually the two are never linked, as they are different.

hope this helps,

greg k-h

Re: [BUG] kobject module-ref race-condition or unsafe module_put(THIS_MODULE)

[David Herrmann]

On Sun, Oct 30, 2011 at 12:39 AM, Greg KH <gregkh@suse.de> wrote:
> On Sat, Oct 29, 2011 at 07:52:47PM +0200, David Herrmann wrote:
>> Hi
>>
>> I currently do not understand how kobjects keep a reference to the
>> owning module. Lets assume I provide a "release" method via a
>> kobj_type for my kobject. I want to go sure my module is still alive
>> when this method is called. Otherwise, this "release" method would be
>> no longer available and we would jump into invalid memory. Therefore,
>> I need to take a reference to my own module. This seems trivial.
>> However, how do I release this reference again?
>> The most simple solution might be calling module_put(THIS_MODULE) in
>> my "release" method. However, if this call drops the module-refcount
>> to 0 and immediately removes the module, the module_put() returns to
>> my "release" function which now is no longer available.
>>
>> It seems quite unlikely that the cleanup of a module is faster than
>> two function-returns, however, theoretically there is a race
>> condition.
>>
>> The following example is based on fs/char_dev.c. Lets assume my module
>> provides a  kobject structure. On init I take a ref to myself with
>> try_module_get(THIS_MODULE).
>
> No, never do that, why would you?
>
>> I add my own kobj_type with the following release function:
>>
>> static void mydev_put(struct mydev *p)
>> {
>>       if (p) {
>>               kobject_put(&p->kobj);
>>               module_put(THIS_MODULE);
>>       }
>> }
>
> Ick, don't do that.
>
>> How can we go sure that module_put() doesn't free my own module before
>> it returns? Isn't a call to module_put(THIS_MODULE) always unsafe
>> (unless I own at least two references)?
>
> Yes, that's why you shouldn't be doing this :)
>
>> Maybe I have missed some important fact here, but this seems quite
>> unsafe to me. Adding a "owner" field to a kobj_type would fix that
>> issue for kobjects/devices.
>
> No, let's determine exactly what you are trying to do first, and why in
> the world you are dealing with "raw" kobjects.  You should almost never
> never never do that.
>
> What driver are you writing?  Have a pointer to the code somewhere?

Or for instance look at drivers/input/input.c at input_dev_release().
It also calls module_put(THIS_MODULE) and it looks quite unsafe to me.
However, I really have no idea how to do this right in these cases.
I think this is the same as the thing I wanted to describe about hci_dev.

> greg k-h

Regards
David