| Message ID | 1527169707-27317-1-git-send-email-gbhat@marvell.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b817047ae70c0bd67b677b65d0d69d72cd6e9728 |
| Delegated to: | Kalle Valo |
| Headers | show |
Ganapathi Bhat <gbhat@marvell.com> wrote: > Race condition is observed during rmmod of mwifiex_usb: > > 1. The rmmod thread will call mwifiex_usb_disconnect(), download > SHUTDOWN command and do wait_event_interruptible_timeout(), > waiting for response. > > 2. The main thread will handle the response and will do a > wake_up_interruptible(), unblocking rmmod thread. > > 3. On getting unblocked, rmmod thread will make rx_cmd.urb = NULL in > mwifiex_usb_free(). > > 4. The main thread will try to resubmit rx_cmd.urb in > mwifiex_usb_submit_rx_urb(), which is NULL. > > To fix, wait for main thread to complete before calling > mwifiex_usb_free(). > > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> Patch applied to wireless-drivers-next.git, thanks. b817047ae70c mwifiex: handle race during mwifiex_usb_disconnect
Hi Ganapathi, On Thu, May 24, 2018 at 07:18:27PM +0530, Ganapathi Bhat wrote: > Race condition is observed during rmmod of mwifiex_usb: > > 1. The rmmod thread will call mwifiex_usb_disconnect(), download > SHUTDOWN command and do wait_event_interruptible_timeout(), > waiting for response. > > 2. The main thread will handle the response and will do a > wake_up_interruptible(), unblocking rmmod thread. > > 3. On getting unblocked, rmmod thread will make rx_cmd.urb = NULL in > mwifiex_usb_free(). > > 4. The main thread will try to resubmit rx_cmd.urb in > mwifiex_usb_submit_rx_urb(), which is NULL. > > To fix, wait for main thread to complete before calling > mwifiex_usb_free(). > > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> > --- > drivers/net/wireless/marvell/mwifiex/usb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c > index bc475b8..6e3cf98 100644 > --- a/drivers/net/wireless/marvell/mwifiex/usb.c > +++ b/drivers/net/wireless/marvell/mwifiex/usb.c > @@ -644,6 +644,9 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf) > MWIFIEX_FUNC_SHUTDOWN); > } > > + if (adapter->workqueue) > + flush_workqueue(adapter->workqueue); This seems like a bad fix. I'm fairly sure there's another race in here somewhere, and at a minimum, this is fragile code. Instead, can't you just move the mwifiex_usb_free() into a .cleanup_if() or .unregister_dev() callback? That's what your other drivers (PCIe and SDIO) use to clean up old buffers and stop bus activity; those are called after the appropriate synchronization points; and I'm pretty sure I've already audited those to be more or less safe. Brian > + > mwifiex_usb_free(card); > > mwifiex_dbg(adapter, FATAL, > -- > 1.9.1 >
Hi Brian, > > @@ -644,6 +644,9 @@ static void mwifiex_usb_disconnect(struct > usb_interface *intf) > > MWIFIEX_FUNC_SHUTDOWN); > > } > > > > + if (adapter->workqueue) > > + flush_workqueue(adapter->workqueue); > > This seems like a bad fix. I'm fairly sure there's another race in here > somewhere, and at a minimum, this is fragile code. Ok. Did you mean there can be some RX work pending at this point, which can cause a similar race for rx URBs? > > Instead, can't you just move the mwifiex_usb_free() into a .cleanup_if() > or .unregister_dev() callback? That's what your other drivers (PCIe and > SDIO) use to clean up old buffers and stop bus activity; those are > called after the appropriate synchronization points; and I'm pretty sure > I've already audited those to be more or less safe. Ok. Yes, this is a better fix for this issue. I will revert the earlier fix and upstream this version. > > Brian > > > + > > mwifiex_usb_free(card); > > > > mwifiex_dbg(adapter, FATAL, > > -- > > 1.9.1 > > Thanks, Ganapathi
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c index bc475b8..6e3cf98 100644 --- a/drivers/net/wireless/marvell/mwifiex/usb.c +++ b/drivers/net/wireless/marvell/mwifiex/usb.c @@ -644,6 +644,9 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf) MWIFIEX_FUNC_SHUTDOWN); } + if (adapter->workqueue) + flush_workqueue(adapter->workqueue); + mwifiex_usb_free(card); mwifiex_dbg(adapter, FATAL,
Race condition is observed during rmmod of mwifiex_usb: 1. The rmmod thread will call mwifiex_usb_disconnect(), download SHUTDOWN command and do wait_event_interruptible_timeout(), waiting for response. 2. The main thread will handle the response and will do a wake_up_interruptible(), unblocking rmmod thread. 3. On getting unblocked, rmmod thread will make rx_cmd.urb = NULL in mwifiex_usb_free(). 4. The main thread will try to resubmit rx_cmd.urb in mwifiex_usb_submit_rx_urb(), which is NULL. To fix, wait for main thread to complete before calling mwifiex_usb_free(). Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> --- drivers/net/wireless/marvell/mwifiex/usb.c | 3 +++ 1 file changed, 3 insertions(+)