[PATCH] mwifiex: handle race during mwifiex_usb_disconnect

[PATCH] mwifiex: handle race during mwifiex_usb_disconnect

[Ganapathi Bhat]

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);
+
 	mwifiex_usb_free(card);
 
 	mwifiex_dbg(adapter, FATAL,
-- 
1.9.1

Re: mwifiex: handle race during mwifiex_usb_disconnect

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/patch/10424763/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] mwifiex: handle race during mwifiex_usb_disconnect

[Brian Norris]

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
> 

RE: [EXT] Re: [PATCH] mwifiex: handle race during mwifiex_usb_disconnect

[Ganapathi Bhat]

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