[PATCH] fix bad locking in drivers/base/driver.c

[PATCH] fix bad locking in drivers/base/driver.c

[Jirka Kosina]

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.

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Chris Wright]

* Jirka Kosina (jikos@jikos.cz) 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

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Greg KH]

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

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Mike Waychison]

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

-----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-----

[-- Attachment #2: convert_unload_sem_to_completion.patch --]
[-- Type: text/x-patch, Size: 3046 bytes --]

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 <michael.waychison@sun.com>
---

 drivers/base/bus.c     |    2 +-
 drivers/base/driver.c  |   13 ++++++-------
 include/linux/device.h |    2 +-
 3 files changed, 8 insertions(+), 9 deletions(-)

Index: linux-2.6.10/drivers/base/bus.c
===================================================================
--- linux-2.6.10.orig/drivers/base/bus.c	2004-12-24 16:34:26.000000000 -0500
+++ linux-2.6.10/drivers/base/bus.c	2005-01-25 02:14:10.000000000 -0500
@@ -65,7 +65,7 @@ static struct sysfs_ops driver_sysfs_ops
 static void driver_release(struct kobject * kobj)
 {
 	struct device_driver * drv = to_driver(kobj);
-	up(&drv->unload_sem);
+	complete(&drv->unloaded);
 }
 
 static struct kobj_type ktype_driver = {
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.
  *	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
@@ -107,8 +107,7 @@ int driver_register(struct device_driver
 void driver_unregister(struct device_driver * drv)
 {
 	bus_remove_driver(drv);
-	down(&drv->unload_sem);
-	up(&drv->unload_sem);
+	wait_for_completion(&drv->unloaded);
 }
 
 /**
Index: linux-2.6.10/include/linux/device.h
===================================================================
--- linux-2.6.10.orig/include/linux/device.h	2004-12-24 16:35:28.000000000 -0500
+++ linux-2.6.10/include/linux/device.h	2005-01-25 02:13:13.716384240 -0500
@@ -102,7 +102,7 @@ struct device_driver {
 	char			* name;
 	struct bus_type		* bus;
 
-	struct semaphore	unload_sem;
+	struct completion	unloaded;
 	struct kobject		kobj;
 	struct list_head	devices;
 

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Bill Davidsen]

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 <davidsen@tmr.com>
   CTO TMR Associates, Inc
   Doing interesting things with small computers since 1979

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Mike Waychison]

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

-----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-----

[-- Attachment #2: convert_unload_sem_to_completion.patch --]
[-- Type: text/x-patch, Size: 3045 bytes --]

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 <michael.waychison@sun.com>
---

 drivers/base/bus.c     |    2 +-
 drivers/base/driver.c  |   13 ++++++-------
 include/linux/device.h |    2 +-
 3 files changed, 8 insertions(+), 9 deletions(-)

Index: linux-2.6.10/drivers/base/bus.c
===================================================================
--- linux-2.6.10.orig/drivers/base/bus.c	2004-12-24 16:34:26.000000000 -0500
+++ linux-2.6.10/drivers/base/bus.c	2005-01-25 02:14:10.000000000 -0500
@@ -65,7 +65,7 @@ static struct sysfs_ops driver_sysfs_ops
 static void driver_release(struct kobject * kobj)
 {
 	struct device_driver * drv = to_driver(kobj);
-	up(&drv->unload_sem);
+	complete(&drv->unloaded);
 }
 
 static struct kobj_type ktype_driver = {
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 11:09:23.579643112 -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 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
@@ -107,8 +107,7 @@ int driver_register(struct device_driver
 void driver_unregister(struct device_driver * drv)
 {
 	bus_remove_driver(drv);
-	down(&drv->unload_sem);
-	up(&drv->unload_sem);
+	wait_for_completion(&drv->unloaded);
 }
 
 /**
Index: linux-2.6.10/include/linux/device.h
===================================================================
--- linux-2.6.10.orig/include/linux/device.h	2004-12-24 16:35:28.000000000 -0500
+++ linux-2.6.10/include/linux/device.h	2005-01-25 02:13:13.000000000 -0500
@@ -102,7 +102,7 @@ struct device_driver {
 	char			* name;
 	struct bus_type		* bus;
 
-	struct semaphore	unload_sem;
+	struct completion	unloaded;
 	struct kobject		kobj;
 	struct list_head	devices;
 

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Linus Torvalds]

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

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Greg KH]

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

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Thomas Gleixner]

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

Re: [PATCH] fix bad locking in drivers/base/driver.c

[Greg KH]

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 <michael.waychison@sun.com>

Thanks, I've applied this to my trees, and it will show up in the next
-mm releases.

greg k-h