* [PATCH] mwifiex: handle race during mwifiex_usb_disconnect
@ 2018-05-24 13:48 Ganapathi Bhat
2018-05-29 7:23 ` Kalle Valo
2018-05-29 21:59 ` [PATCH] " Brian Norris
0 siblings, 2 replies; 4+ messages in thread
From: Ganapathi Bhat @ 2018-05-24 13:48 UTC (permalink / raw)
To: linux-wireless
Cc: Brian Norris, Cathy Luo, Xinming Hu, Zhiyuan Yang, James Cao,
Mangesh Malusare, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: mwifiex: handle race during mwifiex_usb_disconnect
2018-05-24 13:48 [PATCH] mwifiex: handle race during mwifiex_usb_disconnect Ganapathi Bhat
@ 2018-05-29 7:23 ` Kalle Valo
2018-05-29 21:59 ` [PATCH] " Brian Norris
1 sibling, 0 replies; 4+ messages in thread
From: Kalle Valo @ 2018-05-29 7:23 UTC (permalink / raw)
To: Ganapathi Bhat
Cc: linux-wireless, Brian Norris, Cathy Luo, Xinming Hu,
Zhiyuan Yang, James Cao, Mangesh Malusare, Ganapathi Bhat
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mwifiex: handle race during mwifiex_usb_disconnect
2018-05-24 13:48 [PATCH] mwifiex: handle race during mwifiex_usb_disconnect Ganapathi Bhat
2018-05-29 7:23 ` Kalle Valo
@ 2018-05-29 21:59 ` Brian Norris
2018-05-30 7:20 ` [EXT] " Ganapathi Bhat
1 sibling, 1 reply; 4+ messages in thread
From: Brian Norris @ 2018-05-29 21:59 UTC (permalink / raw)
To: Ganapathi Bhat
Cc: linux-wireless, Cathy Luo, Xinming Hu, Zhiyuan Yang, James Cao,
Mangesh Malusare
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [EXT] Re: [PATCH] mwifiex: handle race during mwifiex_usb_disconnect
2018-05-29 21:59 ` [PATCH] " Brian Norris
@ 2018-05-30 7:20 ` Ganapathi Bhat
0 siblings, 0 replies; 4+ messages in thread
From: Ganapathi Bhat @ 2018-05-30 7:20 UTC (permalink / raw)
To: Brian Norris
Cc: linux-wireless, Cathy Luo, Xinming Hu, Zhiyuan Yang, James Cao,
Mangesh Malusare
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-30 7:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-24 13:48 [PATCH] mwifiex: handle race during mwifiex_usb_disconnect Ganapathi Bhat
2018-05-29 7:23 ` Kalle Valo
2018-05-29 21:59 ` [PATCH] " Brian Norris
2018-05-30 7:20 ` [EXT] " Ganapathi Bhat
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.