Hi Dmitry, On Thu, Mar 16, 2017 at 11:33:17AM -0700, Dmitry Torokhov wrote: > On Thu, Mar 16, 2017 at 03:58:52PM +0530, Amitkumar Karwar wrote: > > We observed a SHUTDOWN command timeout during reboot stress test > > due to a corner case firmware bug. It leads to use-after-free on > > adapter structure pointer and crash. > > > > Let's add MWIFIEX_IFACE_WORK_DONT_RUN work flag to avoid executing > > any work scheduled after cancel_work_sync() call in teardown path > > to resolve the issue. > > > > Signed-off-by: Amitkumar Karwar <[email protected]> > > --- > > v2: New work_flag has been added to resolve the issue cleanly as per > > Brian's suggestion. > > --- > > drivers/net/wireless/marvell/mwifiex/main.h | 1 + > > drivers/net/wireless/marvell/mwifiex/pcie.c | 4 ++++ > > drivers/net/wireless/marvell/mwifiex/sdio.c | 4 ++++ > > 3 files changed, 9 insertions(+) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h > > b/drivers/net/wireless/marvell/mwifiex/main.h > > index 5c82972..d5b1fd6 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/main.h > > +++ b/drivers/net/wireless/marvell/mwifiex/main.h > > @@ -510,6 +510,7 @@ struct mwifiex_roc_cfg { > > enum mwifiex_iface_work_flags { > > MWIFIEX_IFACE_WORK_DEVICE_DUMP, > > MWIFIEX_IFACE_WORK_CARD_RESET, > > + MWIFIEX_IFACE_WORK_DONT_RUN, > > }; > > > > struct mwifiex_private { > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c > > b/drivers/net/wireless/marvell/mwifiex/pcie.c > > index a0d9180..bb3d798 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > > @@ -294,6 +294,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev) > > if (!adapter || !adapter->priv_num) > > return; > > > > + set_bit(MWIFIEX_IFACE_WORK_DONT_RUN, &card->work_flags); > > cancel_work_sync(&card->work); > > > > reg = card->pcie.reg; > > @@ -2721,6 +2722,9 @@ static void mwifiex_pcie_work(struct work_struct > > *work) > > struct pcie_service_card *card = > > container_of(work, struct pcie_service_card, work); > > > > + if (test_bit(MWIFIEX_IFACE_WORK_DONT_RUN, &card->work_flags)) > > + return; > > I do not see how this could possible prevent use-after-free, assuming > that the "card" memory is gone by the time mwifiex_pcie_work() gets to > run.
The 'card' memory isn't getting freed; it's the 'adapter' memory we're worried about. This is either already freed (because the FW init procedure failed), or else it's freed later in this function via mwifiex_remove_card(). (We're also worried about having the FW dump race with the FW shutdown sequence, which can begin later in this function. This patch blocks both races AFAICT.) > You need to check this flag before queueing firmware dump work, and > make sure it is not racy with setting this flag in mwifiex_pcie_remove() > (and sdio). That's another approach that could work, but it's a little more invasive. I'm still reviewing and testing this, but I believe this is nearly equivalent to my own draft version, which tested out fine. Brian
