[PATCH 1/2] Bluetooth: bcm203x: Fix race condition on disconnect

[PATCH 1/2] Bluetooth: bcm203x: Fix race condition on disconnect

[David Herrmann]

When disconnecting a bcm203x device we kill and destroy the usb-urb, however,
there might still be a pending work-structure which resubmits the now invalid
urb. To avoid this race condition, we simply set a shutdown-flag and
synchronously kill the worker first.

This also adds a comment to all schedule_work()s, as it is really not clear
that they are used as replacement for short timers (which can be seen in the git
history).

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 drivers/bluetooth/bcm203x.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c
index 8b1b643..6b42732 100644
--- a/drivers/bluetooth/bcm203x.c
+++ b/drivers/bluetooth/bcm203x.c
@@ -24,6 +24,7 @@
 
 #include <linux/module.h>
 
+#include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -65,6 +66,7 @@ struct bcm203x_data {
 	unsigned long		state;
 
 	struct work_struct	work;
+	atomic_t		shutdown;
 
 	struct urb		*urb;
 	unsigned char		*buffer;
@@ -97,6 +99,7 @@ static void bcm203x_complete(struct urb *urb)
 
 		data->state = BCM203X_SELECT_MEMORY;
 
+		/* use workqueue to have a small delay */
 		schedule_work(&data->work);
 		break;
 
@@ -155,6 +158,10 @@ static void bcm203x_work(struct work_struct *work)
 	struct bcm203x_data *data =
 		container_of(work, struct bcm203x_data, work);
 
+	smp_rmb();
+	if (atomic_read(&data->shutdown))
+		return;
+
 	if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0)
 		BT_ERR("Can't submit URB");
 }
@@ -243,6 +250,7 @@ static int bcm203x_probe(struct usb_interface *intf, const struct usb_device_id
 
 	usb_set_intfdata(intf, data);
 
+	/* use workqueue to have a small delay */
 	schedule_work(&data->work);
 
 	return 0;
@@ -254,6 +262,10 @@ static void bcm203x_disconnect(struct usb_interface *intf)
 
 	BT_DBG("intf %p", intf);
 
+	atomic_inc(&data->shutdown);
+	smp_wmb();
+	cancel_work_sync(&data->work);
+
 	usb_kill_urb(data->urb);
 
 	usb_set_intfdata(intf, NULL);
-- 
1.7.7.1

[PATCH 2/2] Bluetooth: bcm203x: Use GFP_KERNEL in workqueue

[David Herrmann]

A workqueue is allowed to sleep so we can safely use GFP_KERNEL instead of
GFP_ATOMIC. This is still legacy code when the driver used timer BHs and not a
worqueue.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 drivers/bluetooth/bcm203x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c
index 6b42732..d644cf7 100644
--- a/drivers/bluetooth/bcm203x.c
+++ b/drivers/bluetooth/bcm203x.c
@@ -162,7 +162,7 @@ static void bcm203x_work(struct work_struct *work)
 	if (atomic_read(&data->shutdown))
 		return;
 
-	if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0)
+	if (usb_submit_urb(data->urb, GFP_KERNEL) < 0)
 		BT_ERR("Can't submit URB");
 }
 
-- 
1.7.7.1

Re: [PATCH 2/2] Bluetooth: bcm203x: Use GFP_KERNEL in workqueue

[Marcel Holtmann]

Hi David,

> A workqueue is allowed to sleep so we can safely use GFP_KERNEL instead of
> GFP_ATOMIC. This is still legacy code when the driver used timer BHs and not a
> worqueue.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
>  drivers/bluetooth/bcm203x.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

good catch here. We should not use GFP_ATOMIC until we have to.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: bcm203x: Fix race condition on disconnect

[Marcel Holtmann]

Hi David,

> When disconnecting a bcm203x device we kill and destroy the usb-urb, however,
> there might still be a pending work-structure which resubmits the now invalid
> urb. To avoid this race condition, we simply set a shutdown-flag and
> synchronously kill the worker first.
> 
> This also adds a comment to all schedule_work()s, as it is really not clear
> that they are used as replacement for short timers (which can be seen in the git
> history).
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
>  drivers/bluetooth/bcm203x.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c
> index 8b1b643..6b42732 100644
> --- a/drivers/bluetooth/bcm203x.c
> +++ b/drivers/bluetooth/bcm203x.c
> @@ -24,6 +24,7 @@
>  
>  #include <linux/module.h>
>  
> +#include <linux/atomic.h>
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> @@ -65,6 +66,7 @@ struct bcm203x_data {
>  	unsigned long		state;
>  
>  	struct work_struct	work;
> +	atomic_t		shutdown;
>  
>  	struct urb		*urb;
>  	unsigned char		*buffer;
> @@ -97,6 +99,7 @@ static void bcm203x_complete(struct urb *urb)
>  
>  		data->state = BCM203X_SELECT_MEMORY;
>  
> +		/* use workqueue to have a small delay */
>  		schedule_work(&data->work);
>  		break;
>  
> @@ -155,6 +158,10 @@ static void bcm203x_work(struct work_struct *work)
>  	struct bcm203x_data *data =
>  		container_of(work, struct bcm203x_data, work);
>  
> +	smp_rmb();
> +	if (atomic_read(&data->shutdown))
> +		return;
> +
>  	if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0)
>  		BT_ERR("Can't submit URB");
>  }
> @@ -243,6 +250,7 @@ static int bcm203x_probe(struct usb_interface *intf, const struct usb_device_id
>  
>  	usb_set_intfdata(intf, data);
>  
> +	/* use workqueue to have a small delay */
>  	schedule_work(&data->work);
>  
>  	return 0;
> @@ -254,6 +262,10 @@ static void bcm203x_disconnect(struct usb_interface *intf)
>  
>  	BT_DBG("intf %p", intf);
>  
> +	atomic_inc(&data->shutdown);
> +	smp_wmb();
> +	cancel_work_sync(&data->work);
> +

can you quickly explain to me why we need the memory barrier here.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: bcm203x: Fix race condition on disconnect

[David Herrmann]

On Wed, Oct 26, 2011 at 10:57 AM, Marcel Holtmann <marcel@holtmann.org> wro=
te:
> Hi David,
>
>> When disconnecting a bcm203x device we kill and destroy the usb-urb, how=
ever,
>> there might still be a pending work-structure which resubmits the now in=
valid
>> urb. To avoid this race condition, we simply set a shutdown-flag and
>> synchronously kill the worker first.
>>
>> This also adds a comment to all schedule_work()s, as it is really not cl=
ear
>> that they are used as replacement for short timers (which can be seen in=
 the git
>> history).
>>
>> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
>> ---
>> =A0drivers/bluetooth/bcm203x.c | =A0 12 ++++++++++++
>> =A01 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c
>> index 8b1b643..6b42732 100644
>> --- a/drivers/bluetooth/bcm203x.c
>> +++ b/drivers/bluetooth/bcm203x.c
>> @@ -24,6 +24,7 @@
>>
>> =A0#include <linux/module.h>
>>
>> +#include <linux/atomic.h>
>> =A0#include <linux/kernel.h>
>> =A0#include <linux/init.h>
>> =A0#include <linux/slab.h>
>> @@ -65,6 +66,7 @@ struct bcm203x_data {
>> =A0 =A0 =A0 unsigned long =A0 =A0 =A0 =A0 =A0 state;
>>
>> =A0 =A0 =A0 struct work_struct =A0 =A0 =A0work;
>> + =A0 =A0 atomic_t =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0shutdown;
>>
>> =A0 =A0 =A0 struct urb =A0 =A0 =A0 =A0 =A0 =A0 =A0*urb;
>> =A0 =A0 =A0 unsigned char =A0 =A0 =A0 =A0 =A0 *buffer;
>> @@ -97,6 +99,7 @@ static void bcm203x_complete(struct urb *urb)
>>
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 data->state =3D BCM203X_SELECT_MEMORY;
>>
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* use workqueue to have a small delay */
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 schedule_work(&data->work);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>>
>> @@ -155,6 +158,10 @@ static void bcm203x_work(struct work_struct *work)
>> =A0 =A0 =A0 struct bcm203x_data *data =3D
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 container_of(work, struct bcm203x_data, work=
);
>>
>> + =A0 =A0 smp_rmb();
>> + =A0 =A0 if (atomic_read(&data->shutdown))
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> +
>> =A0 =A0 =A0 if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 BT_ERR("Can't submit URB");
>> =A0}
>> @@ -243,6 +250,7 @@ static int bcm203x_probe(struct usb_interface *intf,=
 const struct usb_device_id
>>
>> =A0 =A0 =A0 usb_set_intfdata(intf, data);
>>
>> + =A0 =A0 /* use workqueue to have a small delay */
>> =A0 =A0 =A0 schedule_work(&data->work);
>>
>> =A0 =A0 =A0 return 0;
>> @@ -254,6 +262,10 @@ static void bcm203x_disconnect(struct usb_interface=
 *intf)
>>
>> =A0 =A0 =A0 BT_DBG("intf %p", intf);
>>
>> + =A0 =A0 atomic_inc(&data->shutdown);
>> + =A0 =A0 smp_wmb();
>> + =A0 =A0 cancel_work_sync(&data->work);
>> +
>
> can you quickly explain to me why we need the memory barrier here.

atomic_inc() and atomic_read() do not imply memory barriers, hence,
cancel_work_sync() may finish before atomic_inc() is visible to the
worker. Then the worker could restart and resend the urb without
seeing the shutdown-flag and we still have the same race.
However, I think cancel_work_sync() uses spinlocks or some other kind
of lock and implies a memory barrier so both smp_wmb() and smp_rmb()
may be dropped.
I am not sure whether it is valid to rely on cancel_work_sync() to
behave that way.

> Regards
>
> Marcel

Regards
David

Re: [PATCH 1/2] Bluetooth: bcm203x: Fix race condition on disconnect

[Marcel Holtmann]

Hi David,

> >> When disconnecting a bcm203x device we kill and destroy the usb-urb, however,
> >> there might still be a pending work-structure which resubmits the now invalid
> >> urb. To avoid this race condition, we simply set a shutdown-flag and
> >> synchronously kill the worker first.
> >>
> >> This also adds a comment to all schedule_work()s, as it is really not clear
> >> that they are used as replacement for short timers (which can be seen in the git
> >> history).
> >>
> >> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> >> ---
> >>  drivers/bluetooth/bcm203x.c |   12 ++++++++++++
> >>  1 files changed, 12 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c
> >> index 8b1b643..6b42732 100644
> >> --- a/drivers/bluetooth/bcm203x.c
> >> +++ b/drivers/bluetooth/bcm203x.c
> >> @@ -24,6 +24,7 @@
> >>
> >>  #include <linux/module.h>
> >>
> >> +#include <linux/atomic.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/init.h>
> >>  #include <linux/slab.h>
> >> @@ -65,6 +66,7 @@ struct bcm203x_data {
> >>       unsigned long           state;
> >>
> >>       struct work_struct      work;
> >> +     atomic_t                shutdown;
> >>
> >>       struct urb              *urb;
> >>       unsigned char           *buffer;
> >> @@ -97,6 +99,7 @@ static void bcm203x_complete(struct urb *urb)
> >>
> >>               data->state = BCM203X_SELECT_MEMORY;
> >>
> >> +             /* use workqueue to have a small delay */
> >>               schedule_work(&data->work);
> >>               break;
> >>
> >> @@ -155,6 +158,10 @@ static void bcm203x_work(struct work_struct *work)
> >>       struct bcm203x_data *data =
> >>               container_of(work, struct bcm203x_data, work);
> >>
> >> +     smp_rmb();
> >> +     if (atomic_read(&data->shutdown))
> >> +             return;
> >> +
> >>       if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0)
> >>               BT_ERR("Can't submit URB");
> >>  }
> >> @@ -243,6 +250,7 @@ static int bcm203x_probe(struct usb_interface *intf, const struct usb_device_id
> >>
> >>       usb_set_intfdata(intf, data);
> >>
> >> +     /* use workqueue to have a small delay */
> >>       schedule_work(&data->work);
> >>
> >>       return 0;
> >> @@ -254,6 +262,10 @@ static void bcm203x_disconnect(struct usb_interface *intf)
> >>
> >>       BT_DBG("intf %p", intf);
> >>
> >> +     atomic_inc(&data->shutdown);
> >> +     smp_wmb();
> >> +     cancel_work_sync(&data->work);
> >> +
> >
> > can you quickly explain to me why we need the memory barrier here.
> 
> atomic_inc() and atomic_read() do not imply memory barriers, hence,
> cancel_work_sync() may finish before atomic_inc() is visible to the
> worker. Then the worker could restart and resend the urb without
> seeing the shutdown-flag and we still have the same race.
> However, I think cancel_work_sync() uses spinlocks or some other kind
> of lock and implies a memory barrier so both smp_wmb() and smp_rmb()
> may be dropped.
> I am not sure whether it is valid to rely on cancel_work_sync() to
> behave that way.

I think that we can rely on the behavior of cancel_work_sync. So lets
drop the memory barriers and then the patch looks good.

Regards

Marcel

[PATCH v2] Bluetooth: bcm203x: Fix race condition on disconnect

[David Herrmann]

When disconnecting a bcm203x device we kill and destroy the usb-urb, however,
there might still be a pending work-structure which resubmits the now invalid
urb. To avoid this race condition, we simply set a shutdown-flag and
synchronously kill the worker first.

This also adds a comment to all schedule_work()s, as it is really not clear
that they are used as replacement for short timers (which can be seen in the git
history).

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
V2: Remove memory barriers

 drivers/bluetooth/bcm203x.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/bluetooth/bcm203x.c b/drivers/bluetooth/bcm203x.c
index 8b1b643..ec743c2 100644
--- a/drivers/bluetooth/bcm203x.c
+++ b/drivers/bluetooth/bcm203x.c
@@ -24,6 +24,7 @@
 
 #include <linux/module.h>
 
+#include <linux/atomic.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -65,6 +66,7 @@ struct bcm203x_data {
 	unsigned long		state;
 
 	struct work_struct	work;
+	atomic_t		shutdown;
 
 	struct urb		*urb;
 	unsigned char		*buffer;
@@ -97,6 +99,7 @@ static void bcm203x_complete(struct urb *urb)
 
 		data->state = BCM203X_SELECT_MEMORY;
 
+		/* use workqueue to have a small delay */
 		schedule_work(&data->work);
 		break;
 
@@ -155,6 +158,9 @@ static void bcm203x_work(struct work_struct *work)
 	struct bcm203x_data *data =
 		container_of(work, struct bcm203x_data, work);
 
+	if (atomic_read(&data->shutdown))
+		return;
+
 	if (usb_submit_urb(data->urb, GFP_ATOMIC) < 0)
 		BT_ERR("Can't submit URB");
 }
@@ -243,6 +249,7 @@ static int bcm203x_probe(struct usb_interface *intf, const struct usb_device_id
 
 	usb_set_intfdata(intf, data);
 
+	/* use workqueue to have a small delay */
 	schedule_work(&data->work);
 
 	return 0;
@@ -254,6 +261,9 @@ static void bcm203x_disconnect(struct usb_interface *intf)
 
 	BT_DBG("intf %p", intf);
 
+	atomic_inc(&data->shutdown);
+	cancel_work_sync(&data->work);
+
 	usb_kill_urb(data->urb);
 
 	usb_set_intfdata(intf, NULL);
-- 
1.7.7.1

Re: [PATCH v2] Bluetooth: bcm203x: Fix race condition on disconnect

[Marcel Holtmann]

Hi David,

> When disconnecting a bcm203x device we kill and destroy the usb-urb, however,
> there might still be a pending work-structure which resubmits the now invalid
> urb. To avoid this race condition, we simply set a shutdown-flag and
> synchronously kill the worker first.
> 
> This also adds a comment to all schedule_work()s, as it is really not clear
> that they are used as replacement for short timers (which can be seen in the git
> history).
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> V2: Remove memory barriers
> 
>  drivers/bluetooth/bcm203x.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)

looks good to me now.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH v2] Bluetooth: bcm203x: Fix race condition on disconnect

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@googlemail.com> [2011-10-26 11:13:13 +0200]:

> When disconnecting a bcm203x device we kill and destroy the usb-urb, however,
> there might still be a pending work-structure which resubmits the now invalid
> urb. To avoid this race condition, we simply set a shutdown-flag and
> synchronously kill the worker first.
> 
> This also adds a comment to all schedule_work()s, as it is really not clear
> that they are used as replacement for short timers (which can be seen in the git
> history).
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
> V2: Remove memory barriers
> 
>  drivers/bluetooth/bcm203x.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)

Applied, thanks.

	Gustavo

Re: [PATCH 2/2] Bluetooth: bcm203x: Use GFP_KERNEL in workqueue

[Gustavo Padovan]

Hi David,

* David Herrmann <dh.herrmann@googlemail.com> [2011-10-25 21:13:36 +0200]:

> A workqueue is allowed to sleep so we can safely use GFP_KERNEL instead of
> GFP_ATOMIC. This is still legacy code when the driver used timer BHs and not a
> worqueue.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
>  drivers/bluetooth/bcm203x.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applied, thanks.

	Gustavo