[PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup

[PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup

[Ganapathi Bhat]

Driver calls del_timer_sync(hold_timer), in unregister_dev(), but
there exists is a case when the timer is yet to be initialized. A
restructure of init and cleanup is needed to synchronize timer
creation and delee. Make use of init_if() / cleanup_if() handlers
to get this done.

Reported-by: syzbot+373e6719b49912399d21@syzkaller.appspotmail.com
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
---
 drivers/net/wireless/marvell/mwifiex/usb.c | 32 +++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index c2365ee..939f1e9 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
 
 	for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
 		port = &card->port[idx];
+		if (!port->tx_data_ep)
+			continue;
 		if (adapter->bus_aggr.enable)
 			while ((skb_tmp =
 				skb_dequeue(&port->tx_aggr.aggr_list)))
@@ -1365,8 +1367,6 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
 
 	mwifiex_usb_free(card);
 
-	mwifiex_usb_cleanup_tx_aggr(adapter);
-
 	card->adapter = NULL;
 }
 
@@ -1510,7 +1510,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter,
 static int mwifiex_usb_dnld_fw(struct mwifiex_adapter *adapter,
 			struct mwifiex_fw_image *fw)
 {
-	int ret;
+	int ret = 0;
 	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
 
 	if (card->usb_boot_state == USB8XXX_FW_DNLD) {
@@ -1523,10 +1523,6 @@ static int mwifiex_usb_dnld_fw(struct mwifiex_adapter *adapter,
 			return -1;
 	}
 
-	ret = mwifiex_usb_rx_init(adapter);
-	if (!ret)
-		ret = mwifiex_usb_tx_init(adapter);
-
 	return ret;
 }
 
@@ -1584,7 +1580,29 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
 	return 0;
 }
 
+static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
+{
+	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
+	int ret = 0;
+
+	if (card->usb_boot_state == USB8XXX_FW_DNLD)
+		return 0;
+
+	ret = mwifiex_usb_rx_init(adapter);
+	if (!ret)
+		ret = mwifiex_usb_tx_init(adapter);
+
+	return ret;
+}
+
+static void mwifiex_cleanup_usb(struct mwifiex_adapter *adapter)
+{
+	mwifiex_usb_cleanup_tx_aggr(adapter);
+}
+
 static struct mwifiex_if_ops usb_ops = {
+	.init_if =		mwifiex_init_usb,
+	.cleanup_if =		mwifiex_cleanup_usb,
 	.register_dev =		mwifiex_register_dev,
 	.unregister_dev =	mwifiex_unregister_dev,
 	.wakeup =		mwifiex_pm_wakeup_card,
-- 
1.9.1

Re: [PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup

[Brian Norris]

Hi Ganapathi,

This looks kinda wrong, but I'm not totally sure, as I'm not very familiar with
your USB driver.

On Wed, Jun 12, 2019 at 09:24:33PM +0530, Ganapathi Bhat wrote:
> Driver calls del_timer_sync(hold_timer), in unregister_dev(), but
> there exists is a case when the timer is yet to be initialized. A
> restructure of init and cleanup is needed to synchronize timer
> creation and delee. Make use of init_if() / cleanup_if() handlers

s/delee/delete/

> to get this done.
> 
> Reported-by: syzbot+373e6719b49912399d21@syzkaller.appspotmail.com
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/usb.c | 32 +++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index c2365ee..939f1e9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
>  
>  	for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
>  		port = &card->port[idx];
> +		if (!port->tx_data_ep)
> +			continue;

It's not clear to me what this is about. Are you sure you're not just cleaning
stuff up in the wrong order?

>  		if (adapter->bus_aggr.enable)
>  			while ((skb_tmp =
>  				skb_dequeue(&port->tx_aggr.aggr_list)))

...

> @@ -1584,7 +1580,29 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
>  	return 0;
>  }
>  
> +static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
> +{
> +	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
> +	int ret = 0;
> +
> +	if (card->usb_boot_state == USB8XXX_FW_DNLD)
> +		return 0;

This looks wrong. You don't want to skip your basic initialization just because
firmware isn't loaded yet. In fact, init_if() always gets called before FW
init, so haven't you basically stubbed out this function most of the time?

I guess the question is: is this step supposed to go before, or after firmware
initilization? Based on that answer, we can make an appropriate patch.

(The original code does this after FW initialization, and now you're only sort
of moving it before.)

> +
> +	ret = mwifiex_usb_rx_init(adapter);
> +	if (!ret)
> +		ret = mwifiex_usb_tx_init(adapter);
> +
> +	return ret;
> +}

Brian

RE: [EXT] Re: [PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup

[Ganapathi Bhat]

Hi Brian,

Sorry for delayed response;

> > @@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct
> > mwifiex_adapter *adapter)
> >
> >  	for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
> >  		port = &card->port[idx];
> > +		if (!port->tx_data_ep)
> > +			continue;
> 
> It's not clear to me what this is about. Are you sure you're not just cleaning
> stuff up in the wrong order?
OK; 
> 
> >  		if (adapter->bus_aggr.enable)
> >  			while ((skb_tmp =
> >  				skb_dequeue(&port->tx_aggr.aggr_list)))
> 
> ...
> 
> > @@ -1584,7 +1580,29 @@ static void
> mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
> >  	return 0;
> >  }
> >
> > +static int mwifiex_init_usb(struct mwifiex_adapter *adapter) {
> > +	struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
> > +	int ret = 0;
> > +
> > +	if (card->usb_boot_state == USB8XXX_FW_DNLD)
> > +		return 0;
> 
> This looks wrong. You don't want to skip your basic initialization just because
> firmware isn't loaded yet. In fact, init_if() always gets called before FW init,
> so haven't you basically stubbed out this function most of the time?

I think, we are not skipping the basic initialization:
original flow:
PID_1: mwifiex_usb_probe  >>> "do driver initialization" >>> "download the firmware"
PID_2: mwifiex_usb_probe  >>> "do driver initialization" >>> "do interface initialization" >>> "do firmware initialization"
new flow:
PID_1: mwifiex_usb_probe  >>> "do driver initialization" >>> "download the firmware"
PID_2: mwifiex_usb_probe  >>> "do interface initialization" >>> "do driver initialization"  >>> "do firmware initialization"

> 
> I guess the question is: is this step supposed to go before, or after firmware
> initilization? Based on that answer, we can make an appropriate patch.

It was before firmware initialization, in the existing driver( and the new change also follows this);

> 
> (The original code does this after FW initialization, and now you're only sort
> of moving it before.)

I don't think so; as per the above flow, things which we moved to init_if() were previously done in mwifiex_usb_dnld_fw(),
just before it return(for PID_2); FW initialization (mwifiex_init_fw()) happens after that;

Regards,
Ganapathi