Hi,
there has been (for quite some time) a bug in function driver_unregister()
- the lock/unlock sequence is protecting nothing and the actual
bus_remove_driver() is called outside critical section.
Please apply.
--- linux-2.6.11-rc2/drivers/base/driver.c.old 2005-01-22 02:48:48.000000000 +0100
+++ linux-2.6.11-rc2/drivers/base/driver.c 2005-01-24 19:19:33.243501684 +0100
@@ -106,8 +106,8 @@
void driver_unregister(struct device_driver * drv)
{
- bus_remove_driver(drv);
down(&drv->unload_sem);
+ bus_remove_driver(drv);
up(&drv->unload_sem);
}
--
JiKos.
* Jirka Kosina ([email protected]) wrote:
> there has been (for quite some time) a bug in function driver_unregister()
> - the lock/unlock sequence is protecting nothing and the actual
> bus_remove_driver() is called outside critical section.
Re-read the comment. It's intentionally done that way.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Mon, Jan 24, 2005 at 07:25:19PM +0100, Jirka Kosina wrote:
> Hi,
>
> there has been (for quite some time) a bug in function driver_unregister()
> - the lock/unlock sequence is protecting nothing and the actual
> bus_remove_driver() is called outside critical section.
>
> Please apply.
No, please read the comment in the code about why this is the way it is.
The code is correct as is.
Also, please CC the driver core maintainer next time for patches like
this so I don't miss them when they go by.
thanks,
greg k-h
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Greg KH wrote:
> On Mon, Jan 24, 2005 at 07:25:19PM +0100, Jirka Kosina wrote:
>
>>Hi,
>>
>>there has been (for quite some time) a bug in function driver_unregister()
>>- the lock/unlock sequence is protecting nothing and the actual
>>bus_remove_driver() is called outside critical section.
>>
>>Please apply.
>
>
> No, please read the comment in the code about why this is the way it is.
> The code is correct as is.
>
Why don't we clean this up as in the proposed attached patch (against
2.6.10). Compile-tested only.
- --
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me,
and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFB9fYjdQs4kOxk3/MRAgzrAJ96+aEawx/A0Sf0d5HqArsasgYrqQCZAVzp
wuGctEJpxqtxezPD7LNGS+U=
=77WW
-----END PGP SIGNATURE-----
Mike Waychison wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Greg KH wrote:
>
>>On Mon, Jan 24, 2005 at 07:25:19PM +0100, Jirka Kosina wrote:
>>
>>
>>>Hi,
>>>
>>>there has been (for quite some time) a bug in function driver_unregister()
>>>- the lock/unlock sequence is protecting nothing and the actual
>>>bus_remove_driver() is called outside critical section.
>>>
>>>Please apply.
>>
>>
>>No, please read the comment in the code about why this is the way it is.
>>The code is correct as is.
>>
>
>
> Why don't we clean this up as in the proposed attached patch (against
> 2.6.10). Compile-tested only.
Let's clean up the spelling as well
>
> - --
> Mike Waychison
> Sun Microsystems, Inc.
> 1 (650) 352-5299 voice
> 1 (416) 202-8336 voice
>
> Index: linux-2.6.10/drivers/base/driver.c
> ===================================================================
> --- linux-2.6.10.orig/drivers/base/driver.c 2004-12-24 16:35:25.000000000 -0500
> +++ linux-2.6.10/drivers/base/driver.c 2005-01-25 02:16:31.000000000 -0500
> @@ -79,14 +79,14 @@ void put_driver(struct device_driver * d
> * since most of the things we have to do deal with the bus
> * structures.
> *
> - * The one interesting aspect is that we initialize @drv->unload_sem
> - * to a locked state here. It will be unlocked when the driver
> - * reference count reaches 0.
> + * The one interesting aspect is that we setup @drv->unloaded
> + * as a completion that gets complete when the driver reference
> + * count reaches 0.
> */
> int driver_register(struct device_driver * drv)
> {
> INIT_LIST_HEAD(&drv->devices);
> - init_MUTEX_LOCKED(&drv->unload_sem);
> + init_completion(&drv->unloaded);
> return bus_add_driver(drv);
> }
>
> @@ -97,7 +97,7 @@ int driver_register(struct device_driver
> *
> * Again, we pass off most of the work to the bus-level call.
> *
> - * Though, once that is done, we attempt to take @drv->unload_sem.
> + * Though, once that is done, we wait until @drv->unloaded is copmleted.
------------------------------------------------------------------>completed
> * This will block until the driver refcount reaches 0, and it is
> * released. Only modular drivers will call this function, and we
> * have to guarantee that it won't complete, letting the driver
--
bill davidsen <[email protected]>
CTO TMR Associates, Inc
Doing interesting things with small computers since 1979
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Bill Davidsen wrote:
>
> Let's clean up the spelling as well
>> - * Though, once that is done, we attempt to take @drv->unload_sem.
>> + * Though, once that is done, we wait until @drv->unloaded is
>> copmleted.
>
> ------------------------------------------------------------------>completed
Thanks for pointing that out. Updated patch attached.
- --
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me,
and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFB9m+FdQs4kOxk3/MRAtusAJ9S+DSVR4DaAK2fLHquYzgRxamq7ACfZNSN
LkPoYAx27W1MpHiW24RL2yM=
=HBQX
-----END PGP SIGNATURE-----
Hmm.. I certainly like the "use completions" patch, since it makes it a
lot more obvious what is going on (and it is what completions were
designed for).
However, since it does change semantics very subtly: if you call
"driver_unregister()" twice (which is wrong, but looking at the code it
looks like it would just silently have worked), the old code would just
ignore it. The new code will block on the second one.
Now, I don't mind the blocking (it's a bug to call it twice, and blocking
should even give a nice callback when you do the "show tasks" sysrq, so
it's a good way to _find_ the bug), but together with Mike's comment about
"Compile-tested only", I'd really like somebody (Greg?) to say "trying to
doubly remove the driver is so illegal that we don't care, and btw, I
tested it and it's all ok".
Linus
On Tue, Jan 25, 2005 at 08:27:15AM -0800, Linus Torvalds wrote:
>
>
> Hmm.. I certainly like the "use completions" patch, since it makes it a
> lot more obvious what is going on (and it is what completions were
> designed for).
>
> However, since it does change semantics very subtly: if you call
> "driver_unregister()" twice (which is wrong, but looking at the code it
> looks like it would just silently have worked), the old code would just
> ignore it. The new code will block on the second one.
>
> Now, I don't mind the blocking (it's a bug to call it twice, and blocking
> should even give a nice callback when you do the "show tasks" sysrq, so
> it's a good way to _find_ the bug), but together with Mike's comment about
> "Compile-tested only", I'd really like somebody (Greg?) to say "trying to
> doubly remove the driver is so illegal that we don't care, and btw, I
> tested it and it's all ok".
I will add it to my queue of patches for the driver core, and test it
out accordingly before trying it out in the -mm tree for a while.
thanks,
greg k-h
On Tue, 2005-01-25 at 11:19 -0800, Greg KH wrote:
> On Tue, Jan 25, 2005 at 08:27:15AM -0800, Linus Torvalds wrote:
> >
> >
> > Hmm.. I certainly like the "use completions" patch, since it makes it a
> > lot more obvious what is going on (and it is what completions were
> > designed for).
> >
> > However, since it does change semantics very subtly: if you call
> > "driver_unregister()" twice (which is wrong, but looking at the code it
> > looks like it would just silently have worked), the old code would just
> > ignore it. The new code will block on the second one.
> >
> > Now, I don't mind the blocking (it's a bug to call it twice, and blocking
> > should even give a nice callback when you do the "show tasks" sysrq, so
> > it's a good way to _find_ the bug), but together with Mike's comment about
> > "Compile-tested only", I'd really like somebody (Greg?) to say "trying to
> > doubly remove the driver is so illegal that we don't care, and btw, I
> > tested it and it's all ok".
>
> I will add it to my queue of patches for the driver core, and test it
> out accordingly before trying it out in the -mm tree for a while.
>
Exactly the same patch is around since 2004-10-20.
http://marc.theaimsgroup.com/?l=linux-kernel&m=109836020930855&w=2
It never showed any problems and I have it in my kernels since then.
Also Ingo's RT patches have it since October.
tglx
On Tue, Jan 25, 2005 at 11:10:46AM -0500, Mike Waychison wrote:
> Get rid of semaphore abuse by converting device_driver->unload_sem
> semaphore to device_driver->unloaded completion.
>
> This should get rid of any confusion as well as save a few bytes in the
> process.
>
> Signed-off-by: Mike Waychison <[email protected]>
Thanks, I've applied this to my trees, and it will show up in the next
-mm releases.
greg k-h