How should an exit routine wait for release() callbacks?

How should an exit routine wait for release() callbacks?

[Alan Stern]

Here's a not-so-theoretical question.

I've got a module which registers a struct device.  (It represents a
virtual device, not a real one, but that doesn't matter.)  Obviously the
module's exit routine has to wait until the release() routine for that
device has been invoked -- if it returned too early then the release()
call would oops.

How should it wait?

The most straightforward approach is to use a struct completion, like 
this:

	static struct {
		struct device dev;
		...
	} my_dev;

	static DECLARE_COMPLETION(my_completion);

	static void my_release(struct device *dev)
	{
		complete(&my_completion);
	}

	static void __exit my_exit(void)
	{
		device_unregister(&my_dev.dev);
		wait_for_completion(&my_completion);
	}

The problem is that there is no guarantee a context switch won't take
place after my_release() has called complete() and before my_release()  
returns.  If that happens and my_exit() finishes running, then the module
will be unloaded and the next context switch back to finish off
my_release() will oops.

Other approaches have similar defects.  So how can this problem be solved?

Alan Stern

Re: How should an exit routine wait for release() callbacks?

[Cornelia Huck]

On Thu, 12 Apr 2007 17:23:18 -0400 (EDT),
Alan Stern <stern@rowland.harvard.edu> wrote:

> Here's a not-so-theoretical question.
> 
> I've got a module which registers a struct device.  (It represents a
> virtual device, not a real one, but that doesn't matter.)  Obviously the
> module's exit routine has to wait until the release() routine for that
> device has been invoked -- if it returned too early then the release()
> call would oops.
> 
> How should it wait?

Device lifetime vs. module lifetime - that's a fun one...

> 
> The most straightforward approach is to use a struct completion, like 
> this:
> 
> 	static struct {
> 		struct device dev;
> 		...
> 	} my_dev;
> 
> 	static DECLARE_COMPLETION(my_completion);
> 
> 	static void my_release(struct device *dev)
> 	{
> 		complete(&my_completion);
> 	}
> 
> 	static void __exit my_exit(void)
> 	{
> 		device_unregister(&my_dev.dev);
> 		wait_for_completion(&my_completion);
> 	}
> 
> The problem is that there is no guarantee a context switch won't take
> place after my_release() has called complete() and before my_release()  
> returns.  If that happens and my_exit() finishes running, then the module
> will be unloaded and the next context switch back to finish off
> my_release() will oops.
> 
> Other approaches have similar defects.  So how can this problem be solved?

What I see that a device driver may do now is the following:
- disallow module unloading (duh)
- move the release function outside the module

To make the completion approach work, the complete() would need to be
after the release function. This would imply an upper layer, but this
upper layer would need to access the completion structure in the
module...

One could think about a owner field (for getting/putting the module
reference) for the object (with a final module_put() after the release
function has been called). The problem there would be that it would
preclude unloading of the module if there isn't a "self destruct" knob
for the object.

Re: How should an exit routine wait for release() callbacks?

[Markus Rechberger]

Alan,

seems like you have the same problem as the dvb framework has/had.

http://mcentral.de/hg/~mrec/v4l-dvb-stable

The last 3 changesets do the trick to not oops, it will delay the 
deinitialization of the device till the last user closed the device node.

Markus

Cornelia Huck wrote:
> On Thu, 12 Apr 2007 17:23:18 -0400 (EDT),
> Alan Stern <stern@rowland.harvard.edu> wrote:
>
>   
>> Here's a not-so-theoretical question.
>>
>> I've got a module which registers a struct device.  (It represents a
>> virtual device, not a real one, but that doesn't matter.)  Obviously the
>> module's exit routine has to wait until the release() routine for that
>> device has been invoked -- if it returned too early then the release()
>> call would oops.
>>
>> How should it wait?
>>     
>
> Device lifetime vs. module lifetime - that's a fun one...
>
>   
>> The most straightforward approach is to use a struct completion, like 
>> this:
>>
>> 	static struct {
>> 		struct device dev;
>> 		...
>> 	} my_dev;
>>
>> 	static DECLARE_COMPLETION(my_completion);
>>
>> 	static void my_release(struct device *dev)
>> 	{
>> 		complete(&my_completion);
>> 	}
>>
>> 	static void __exit my_exit(void)
>> 	{
>> 		device_unregister(&my_dev.dev);
>> 		wait_for_completion(&my_completion);
>> 	}
>>
>> The problem is that there is no guarantee a context switch won't take
>> place after my_release() has called complete() and before my_release()  
>> returns.  If that happens and my_exit() finishes running, then the module
>> will be unloaded and the next context switch back to finish off
>> my_release() will oops.
>>
>> Other approaches have similar defects.  So how can this problem be solved?
>>     
>
> What I see that a device driver may do now is the following:
> - disallow module unloading (duh)
> - move the release function outside the module
>
> To make the completion approach work, the complete() would need to be
> after the release function. This would imply an upper layer, but this
> upper layer would need to access the completion structure in the
> module...
>
> One could think about a owner field (for getting/putting the module
> reference) for the object (with a final module_put() after the release
> function has been called). The problem there would be that it would
> preclude unloading of the module if there isn't a "self destruct" knob
> for the object.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>
>
>   


-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
  System   |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
  Center   |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: How should an exit routine wait for release() callbacks?

[Cornelia Huck]

On Fri, 13 Apr 2007 13:42:04 +0200,
"Markus Rechberger" <markus.rechberger@amd.com> wrote:

> seems like you have the same problem as the dvb framework has/had.
> 
> http://mcentral.de/hg/~mrec/v4l-dvb-stable
> 
> The last 3 changesets do the trick to not oops, it will delay the 
> deinitialization of the device till the last user closed the device node.

Probably dumb question (since I'm not at all familiar with the dvb
code): Isn't that a different race you're solving there? I don't see
any driver core objects involved (except class devices created by
class_device_create, which obviously don't have the release function
problem). This looks more like a race of "we want an object to go
away, but a user still has a file open" (which would be similar to the
kobject<->sysfs lifetime rules issues, where work is currently ongoing).

Re: How should an exit routine wait for release() callbacks?

[Markus Rechberger]

Cornelia Huck wrote:
> On Fri, 13 Apr 2007 13:42:04 +0200,
> "Markus Rechberger" <markus.rechberger@amd.com> wrote:
>
>   
>> seems like you have the same problem as the dvb framework has/had.
>>
>> http://mcentral.de/hg/~mrec/v4l-dvb-stable
>>
>> The last 3 changesets do the trick to not oops, it will delay the 
>> deinitialization of the device till the last user closed the device node.
>>     
>
> Probably dumb question (since I'm not at all familiar with the dvb
> code): Isn't that a different race you're solving there? I don't see
> any driver core objects involved (except class devices created by
> class_device_create, which obviously don't have the release function
> problem). This looks more like a race of "we want an object to go
> away, but a user still has a file open" (which would be similar to the
> kobject<->sysfs lifetime rules issues, where work is currently ongoing).
>
>   
most dvb usb drivers call the device node unregistration when a device 
gets unplugged (when
At this time the filehandle can still be open, the patch on that site 
sets a flag that disallows
any further access to the device node (in the DVB framework there are 4 
of such nodes)
This can happen any time, so while someone is reading or accessing the 
device some structures
might have gone away already and this could cause an oops.
The problem of the DVB framework is file operation related, the last 
user calls fops_put on the existing
structure and sets the pointer to NULL before it wakes up the other 
function which frees the file operation
structure.
In Alan's case isn't there any users flag available that shows that the 
structure is still beeing accessed?
If that would be the case he could set a flag when he enters my_exit 
which would disable access to all other
functions by returning an error value at the beginning of the other 
functions, the only way out would be
to call my_release for existing users and wake up my_exit when the last 
reference to that structure is gone.

Some more information about the whole driver/scenario would be helpful.

Markus

-- 
           |           AMD Saxony Limited Liability Company & Co. KG
 Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
 System    |                  Register Court Dresden: HRA 4896
 Research  |              General Partner authorized to represent:
 Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
           | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

Re: How should an exit routine wait for release() callbacks?

[Cornelia Huck]

On Fri, 13 Apr 2007 16:15:18 +0200,
"Markus Rechberger" <markus.rechberger@amd.com> wrote:

> most dvb usb drivers call the device node unregistration when a device 
> gets unplugged (when
> At this time the filehandle can still be open, the patch on that site 
> sets a flag that disallows
> any further access to the device node (in the DVB framework there are 4 
> of such nodes)
> This can happen any time, so while someone is reading or accessing the 
> device some structures
> might have gone away already and this could cause an oops.
> The problem of the DVB framework is file operation related, the last 
> user calls fops_put on the existing
> structure and sets the pointer to NULL before it wakes up the other 
> function which frees the file operation
> structure.

OK, thanks for the explanation.

> In Alan's case isn't there any users flag available that shows that the 
> structure is still beeing accessed?
> If that would be the case he could set a flag when he enters my_exit 
> which would disable access to all other
> functions by returning an error value at the beginning of the other 
> functions, the only way out would be
> to call my_release for existing users and wake up my_exit when the last 
> reference to that structure is gone.
> 
> Some more information about the whole driver/scenario would be helpful.

In this case the race is not a user space vs. kernel object one (where
you can track users). Basically the problem is as follows:

- A module registers a device. The device's release function is defined
in the module.
- Since the device can now be looked up in the device tree, someone can
obtain a reference to it (e. g. by walking the tree).
- The module is unloaded. In its exit function, it deregisters the
device. The module has now given up any reference to the device it
held, however the someone from above still holds a reference. While no
new reference to the device can be obtained, the device still exists.
- After the module is unloaded, the device's release function goes away.
- The last reference to the device is given up. The driver core now
tries to call the device's release function, which was in the deleted
module. Oops.

The completion approach unfortunately still leaves a race window, as
Alan explained in his original mail.

Re: How should an exit routine wait for release() callbacks?

[Alan Stern]

Tejun, it just occurred to me that you would be interested in this email 
thread.  Just to bring you up to speed, here's the original question:

> I've got a module which registers a struct device.  (It represents a
> virtual device, not a real one, but that doesn't matter.)  Obviously the
> module's exit routine has to wait until the release() routine for that
> device has been invoked -- if it returned too early then the release()
> call would oops.
> 
> How should it wait?
> 
> The most straightforward approach is to use a struct completion, like 
> this:
> 
> 	static struct {
> 		struct device dev;
> 		...
> 	} my_dev;
> 
> 	static DECLARE_COMPLETION(my_completion);
> 
> 	static void my_release(struct device *dev)
> 	{
> 		complete(&my_completion);
> 	}
> 
> 	static void __exit my_exit(void)
> 	{
> 		device_unregister(&my_dev.dev);
> 		wait_for_completion(&my_completion);
> 	}
> 
> The problem is that there is no guarantee a context switch won't take
> place after my_release() has called complete() and before my_release()  
> returns.  If that happens and my_exit() finishes running, then the module
> will be unloaded and the next context switch back to finish off
> my_release() will oops.


On Fri, 13 Apr 2007, Cornelia Huck wrote:

> In this case the race is not a user space vs. kernel object one (where
> you can track users). Basically the problem is as follows:
> 
> - A module registers a device. The device's release function is defined
> in the module.
> - Since the device can now be looked up in the device tree, someone can
> obtain a reference to it (e. g. by walking the tree).
> - The module is unloaded. In its exit function, it deregisters the
> device. The module has now given up any reference to the device it
> held, however the someone from above still holds a reference. While no
> new reference to the device can be obtained, the device still exists.
> - After the module is unloaded, the device's release function goes away.
> - The last reference to the device is given up. The driver core now
> tries to call the device's release function, which was in the deleted
> module. Oops.
> 
> The completion approach unfortunately still leaves a race window, as
> Alan explained in his original mail.

After thinking about it some more, I realized the conventional answer 
would be to give out a module reference.  When the device is registered, a 
reference to it and its release routine gets passed to the driver core.  
Hence a module reference to the owner of the release routine must be 
passed as well.

Unforunately that won't work very well in this case, because it would 
create circular module references preventing the driver from ever being 
unloaded at all!  Here's what I mean:

	my_device is registered with some core subsystem.  The subsystem
	acquires a module reference to my module and registers a child
	of my_device.  It can't drop the module reference until the
	child is gone and it has finished using my_device.  So my driver
	can't be unloaded until it deregisters my_device or the subsystem
	itself is unloaded (and unregisters the child).

	But the module containing my driver depends on the subsystem,
	because it calls the subsystem's registration routine.  So the
	subsystem can't be unloaded without unloading my driver first.

What's needed is a way to force my driver to unregister itself without
actually unloading it from memory.  One way to accomplish this would be to
tell rmmod that it should call the module's exit routine but then wait
for the module's refcount to drop to 0 before unloading it.  Like doing 
rmmod -w.

However even this would have a problem.  Let's say my_exit() unregisters 
my_device.  Eventually the child's release routine runs, and it does a 
put_module() on my module.  At that point my module can be unloaded.  But 
my_release() still hasn't been called!  The subsystem's release routine 
runs first, because children are released before their parents.

I have to admit, this is a puzzler.  I'm beginning to think there should 
be two types of module references: Those which (like module dependency) 
will prevent rmmod from running, and those which (like the one here) would 
automatically be dropped by deregistration.  Then every kobject could have 
an owner and could hold a reference of the second type to its owner until 
its release routine returns.

Alan Stern

Re: How should an exit routine wait for release() callbacks?

[Cornelia Huck]

On Fri, 13 Apr 2007 11:24:58 -0400 (EDT),
Alan Stern <stern@rowland.harvard.edu> wrote:

> I have to admit, this is a puzzler.  I'm beginning to think there should 
> be two types of module references: Those which (like module dependency) 
> will prevent rmmod from running, and those which (like the one here) would 
> automatically be dropped by deregistration.  Then every kobject could have 
> an owner and could hold a reference of the second type to its owner until 
> its release routine returns.

This sounds like the most promising idea yet.

We could make kobject_add() bump up this second-type refcount (since
from now on it may be looked up). kobject_get()/kobject_put() wouldn't
need to grab an extra reference (we already have refcounting for this
object in place). kobject_cleanup() could do something like:

struct module * kobject_owner = kobj->owner;
...
call_release();
put_second_module_refcount(kobject_owner);

combined with the module unloading code waiting after calling the exit
function until the second type refcount dropped to 0. This would make
sure that the module is not deleted until the last release function has
been called.

The module would stay in memory (not be unloaded) until the last
kobject created by the module is deleted, but I think that is just what
we want. At least this doesn't mean that the module blocks its own
unloading.

Re: How should an exit routine wait for release() callbacks?

[Alan Stern]

On Mon, 16 Apr 2007, Cornelia Huck wrote:

> On Fri, 13 Apr 2007 11:24:58 -0400 (EDT),
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > I have to admit, this is a puzzler.  I'm beginning to think there should 
> > be two types of module references: Those which (like module dependency) 
> > will prevent rmmod from running, and those which (like the one here) would 
> > automatically be dropped by deregistration.  Then every kobject could have 
> > an owner and could hold a reference of the second type to its owner until 
> > its release routine returns.
> 
> This sounds like the most promising idea yet.
> 
> We could make kobject_add() bump up this second-type refcount (since

Actually it has to be done in kobject_init() since the release method can 
be called any time after that, even if the kobject is never add'ed.

> from now on it may be looked up). kobject_get()/kobject_put() wouldn't
> need to grab an extra reference (we already have refcounting for this
> object in place). kobject_cleanup() could do something like:
> 
> struct module * kobject_owner = kobj->owner;
> ...
> call_release();
> put_second_module_refcount(kobject_owner);
> 
> combined with the module unloading code waiting after calling the exit
> function until the second type refcount dropped to 0. This would make
> sure that the module is not deleted until the last release function has
> been called.
> 
> The module would stay in memory (not be unloaded) until the last
> kobject created by the module is deleted, but I think that is just what
> we want. At least this doesn't mean that the module blocks its own
> unloading.

Yes, that's what I had in mind.

It would have to apply to other things besides kobjects -- in principle,
anything with a release routine, although in many cases it wouldn't be 
needed.  But in particular, it _would_ be needed for struct device.

(In fact, perhaps kobject would not need it.  There aren't too many places
where a raw kobject is used; almost always it is embedded in some larger
object -- like struct device -- along with a release method pointer.  
This larger object would need an owner but its embedded kobject usually
would not.)

On the other hand, this proposal involves adding a fair amount of overhead
(all those .owner fields) for a rather small benefit.  And it involves
modifying a core kernel subsystem (kernel/module.c).  All to prevent
certain unlikely sorts of errors when removing a module -- something which
Linus has said repeatedly need not be supported terribly well.

So I'm uncertain whether other people will be in favor of all this.

Alan Stern

Re: [linux-usb-devel] How should an exit routine wait for release() callbacks?

[Robert Marquardt]

Alan Stern wrote:

> On the other hand, this proposal involves adding a fair amount of overhead
> (all those .owner fields) for a rather small benefit.  And it involves
> modifying a core kernel subsystem (kernel/module.c).  All to prevent
> certain unlikely sorts of errors when removing a module -- something which
> Linus has said repeatedly need not be supported terribly well.
> 
> So I'm uncertain whether other people will be in favor of all this.

I think Linus is in error here. Either do it right or not at all. I 
would say that modules should work *always* and without any conceptual 
problem.

Re: How should an exit routine wait for release() callbacks?

[Cornelia Huck]

On Mon, 16 Apr 2007 10:43:01 -0400 (EDT),
Alan Stern <stern@rowland.harvard.edu> wrote:

> Actually it has to be done in kobject_init() since the release method can 
> be called any time after that, even if the kobject is never add'ed.

True. This would also imply that we only ever must free a kobject with
kobject_put() if kobject_init() has been called.

> 
> > from now on it may be looked up). kobject_get()/kobject_put() wouldn't
> > need to grab an extra reference (we already have refcounting for this
> > object in place). kobject_cleanup() could do something like:
> > 
> > struct module * kobject_owner = kobj->owner;
> > ...
> > call_release();
> > put_second_module_refcount(kobject_owner);
> > 
> > combined with the module unloading code waiting after calling the exit
> > function until the second type refcount dropped to 0. This would make
> > sure that the module is not deleted until the last release function has
> > been called.
> > 
> > The module would stay in memory (not be unloaded) until the last
> > kobject created by the module is deleted, but I think that is just what
> > we want. At least this doesn't mean that the module blocks its own
> > unloading.
> 
> Yes, that's what I had in mind.
> 
> It would have to apply to other things besides kobjects -- in principle,
> anything with a release routine, although in many cases it wouldn't be 
> needed.  But in particular, it _would_ be needed for struct device.
> 
> (In fact, perhaps kobject would not need it.  There aren't too many places
> where a raw kobject is used; almost always it is embedded in some larger
> object -- like struct device -- along with a release method pointer.  
> This larger object would need an owner but its embedded kobject usually
> would not.)

Yes, struct device might be enough for most use cases. However, this
would involve looking hard at the code :)

> 
> On the other hand, this proposal involves adding a fair amount of overhead
> (all those .owner fields) for a rather small benefit.  And it involves
> modifying a core kernel subsystem (kernel/module.c).  All to prevent
> certain unlikely sorts of errors when removing a module -- something which
> Linus has said repeatedly need not be supported terribly well.

The basic infrastructure isn't too hard (I'm having a patch using mkobj
on my disk that is in nearly workable state). And I think that this is
something that really should be fixed - it is just way too easy for a
driver writer to mess this up (and the race window becomes even bigger
on virtualized platforms).

Re: [linux-usb-devel] How should an exit routine wait for release() callbacks?

[Greg KH]

Ah, just found this original thread, now Cornelia's patches make more
sense...

On Fri, Apr 13, 2007 at 11:24:58AM -0400, Alan Stern wrote:
> Tejun, it just occurred to me that you would be interested in this email 
> thread.  Just to bring you up to speed, here's the original question:
> 
> > I've got a module which registers a struct device.  (It represents a
> > virtual device, not a real one, but that doesn't matter.)

Wait, that's the issue right there.

Don't do that.

devices should be created by busses or the platform core, which owns the
release function for them.  Individual drivers should not create
devices.

Hm, but then, how would you ever unload a bus, as the same issue might
be there too...

Any specific code in the kernel you can point to that has this issue
today?

thanks,

greg k-h

Re: [linux-usb-devel] How should an exit routine wait for release() callbacks?

[Cornelia Huck]

On Mon, 16 Apr 2007 15:12:47 -0700,
Greg KH <greg@kroah.com> wrote:

> Ah, just found this original thread, now Cornelia's patches make more
> sense...

I would have included a pointer, but couldn't access marc yesterday
evening, sorry...

> 
> On Fri, Apr 13, 2007 at 11:24:58AM -0400, Alan Stern wrote:
> > Tejun, it just occurred to me that you would be interested in this email 
> > thread.  Just to bring you up to speed, here's the original question:
> > 
> > > I've got a module which registers a struct device.  (It represents a
> > > virtual device, not a real one, but that doesn't matter.)
> 
> Wait, that's the issue right there.
> 
> Don't do that.
> 
> devices should be created by busses or the platform core, which owns the
> release function for them.  Individual drivers should not create
> devices.

There are drivers that do it, for a variety of reasons.

> 
> Hm, but then, how would you ever unload a bus, as the same issue might
> be there too...

Exactly :) This would imply busses/subsystems could never be unloadable
modules.

> 
> Any specific code in the kernel you can point to that has this issue
> today?

zfcp comes to mind. They register some units in zfcp_aux.c, which don't
have anything to do with the bus the zfcp adapter is on (the ccw bus).
Currently, that is "solved" by disallowing zfcp module unloading.

Re: [linux-usb-devel] How should an exit routine wait for release() callbacks?

[Alan Stern]

On Mon, 16 Apr 2007, Greg KH wrote:

> > > I've got a module which registers a struct device.  (It represents a
> > > virtual device, not a real one, but that doesn't matter.)
> 
> Wait, that's the issue right there.
> 
> Don't do that.
> 
> devices should be created by busses or the platform core, which owns the
> release function for them.  Individual drivers should not create
> devices.
> 
> Hm, but then, how would you ever unload a bus, as the same issue might
> be there too...
> 
> Any specific code in the kernel you can point to that has this issue
> today?

I first noticed it while working with dummy_hcd.  It creates a virtual 
host controller platform device and a virtual device controller platform 
device.  Its exit routine doesn't wait (and has no way to wait) for the 
release methods of those devices to finish executing.

But the same issue tends to pop up anywhere you have a "bridge" device --
one that connects two different kinds of bus.  For instance, a PC-card
essentially bridges a CardBus to whatever you can plug into that card.  
usb-storage essentially bridges a USB bus to a SCSI bus.  uhci-hcd
essentially bridges a PCI bus to a USB bus.

The problem itself isn't very noticeable.  Existing uses of
try_module_get() generally reduce it to an unlikely race, and the
uncoupling of sysfs from devices will make it even rarer.

But the problem still exists potentially.

Alan Stern