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

Hi David,

quoted

quoted

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