diff mbox

[01/11] mwifiex: fixup error cases in mwifiex_add_virtual_intf()

Message ID 20170512164208.38725-1-briannorris@chromium.org (mailing list archive)
State Accepted
Commit 8535107aa4ef92520cbb9a4739563b389c5f8e2c
Delegated to: Kalle Valo
Headers show

Commit Message

Brian Norris May 12, 2017, 4:41 p.m. UTC
If we fail to add an interface in mwifiex_add_virtual_intf(), we might
hit a BUG_ON() in the networking code, because we didn't tear things
down properly. Among the problems:

 (a) when failing to allocate workqueues, we fail to unregister the
     netdev before calling free_netdev()
 (b) even if we do try to unregister the netdev, we're still holding the
     rtnl lock, so the device never properly unregistered; we'll be at
     state NETREG_UNREGISTERING, and then hit free_netdev()'s:
	BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
 (c) we're allocating some dependent resources (e.g., DFS workqueues)
     after we've registered the interface; this may or may not cause
     problems, but it's good practice to allocate these before registering
 (d) we're not even trying to unwind anything when mwifiex_send_cmd() or
     mwifiex_sta_init_cmd() fail

To fix these issues, let's:

 * add a stacked set of error handling labels, to keep error handling
   consistent and properly ordered (resolving (a) and (d))
 * move the workqueue allocations before the registration (to resolve
   (c); also resolves (b) by avoiding error cases where we have to
   unregister)

[Incidentally, it's pretty easy to interrupt the alloc_workqueue() in,
e.g., the following:

  iw phy phy0 interface add mlan0 type station

by sending it SIGTERM.]

This bugfix covers commits like commit 7d652034d1a0 ("mwifiex: channel
switch support for mwifiex"), but parts of this bug exist all the way
back to the introduction of dynamic interface handling in commit
93a1df48d224 ("mwifiex: add cfg80211 handlers add/del_virtual_intf").

Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/marvell/mwifiex/cfg80211.c | 71 ++++++++++++-------------
 1 file changed, 35 insertions(+), 36 deletions(-)

Comments

Xinming Hu May 17, 2017, 4:02 a.m. UTC | #1
Hi,

This patch serials looks fine, thanks.

> -----Original Message-----

> From: linux-wireless-owner@vger.kernel.org

> [mailto:linux-wireless-owner@vger.kernel.org] On Behalf Of Brian Norris

> Sent: 2017年5月13日 0:42

> To: Ganapathi Bhat; Nishant Sarmukadam

> Cc: linux-kernel@vger.kernel.org; Dmitry Torokhov; Amitkumar Karwar; Kalle

> Valo; linux-wireless@vger.kernel.org; Doug Anderson; Brian Norris

> Subject: [PATCH 01/11] mwifiex: fixup error cases in mwifiex_add_virtual_intf()

> 

> If we fail to add an interface in mwifiex_add_virtual_intf(), we might hit a

> BUG_ON() in the networking code, because we didn't tear things down

> properly. Among the problems:

> 

>  (a) when failing to allocate workqueues, we fail to unregister the

>      netdev before calling free_netdev()

>  (b) even if we do try to unregister the netdev, we're still holding the

>      rtnl lock, so the device never properly unregistered; we'll be at

>      state NETREG_UNREGISTERING, and then hit free_netdev()'s:

> 	BUG_ON(dev->reg_state != NETREG_UNREGISTERED);

>  (c) we're allocating some dependent resources (e.g., DFS workqueues)

>      after we've registered the interface; this may or may not cause

>      problems, but it's good practice to allocate these before registering

>  (d) we're not even trying to unwind anything when mwifiex_send_cmd() or

>      mwifiex_sta_init_cmd() fail

> 

> To fix these issues, let's:

> 

>  * add a stacked set of error handling labels, to keep error handling

>    consistent and properly ordered (resolving (a) and (d))

>  * move the workqueue allocations before the registration (to resolve

>    (c); also resolves (b) by avoiding error cases where we have to

>    unregister)

> 

> [Incidentally, it's pretty easy to interrupt the alloc_workqueue() in, e.g., the

> following:

> 

>   iw phy phy0 interface add mlan0 type station

> 

> by sending it SIGTERM.]

> 

> This bugfix covers commits like commit 7d652034d1a0 ("mwifiex: channel

> switch support for mwifiex"), but parts of this bug exist all the way back to the

> introduction of dynamic interface handling in commit

> 93a1df48d224 ("mwifiex: add cfg80211 handlers add/del_virtual_intf").

> 

> Cc: <stable@vger.kernel.org>

> Signed-off-by: Brian Norris <briannorris@chromium.org>

> ---

>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 71

> ++++++++++++-------------

>  1 file changed, 35 insertions(+), 36 deletions(-)

> 

> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c

> b/drivers/net/wireless/marvell/mwifiex/cfg80211.c

> index 7ec06bf13413..025bc06a19d6 100644

> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c

> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c

> @@ -2964,10 +2964,8 @@ struct wireless_dev

> *mwifiex_add_virtual_intf(struct wiphy *wiphy,

>  	if (!dev) {

>  		mwifiex_dbg(adapter, ERROR,

>  			    "no memory available for netdevice\n");

> -		memset(&priv->wdev, 0, sizeof(priv->wdev));

> -		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;

> -		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;

> -		return ERR_PTR(-ENOMEM);

> +		ret = -ENOMEM;

> +		goto err_alloc_netdev;

>  	}

> 

>  	mwifiex_init_priv_params(priv, dev);

> @@ -2976,11 +2974,11 @@ struct wireless_dev

> *mwifiex_add_virtual_intf(struct wiphy *wiphy,

>  	ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,

>  			       HostCmd_ACT_GEN_SET, 0, NULL, true);

>  	if (ret)

> -		return ERR_PTR(ret);

> +		goto err_set_bss_mode;

> 

>  	ret = mwifiex_sta_init_cmd(priv, false, false);

>  	if (ret)

> -		return ERR_PTR(ret);

> +		goto err_sta_init;

> 

>  	mwifiex_setup_ht_caps(&wiphy->bands[NL80211_BAND_2GHZ]->ht_cap,

> priv);

>  	if (adapter->is_hw_11ac_capable)

> @@ -3011,31 +3009,14 @@ struct wireless_dev

> *mwifiex_add_virtual_intf(struct wiphy *wiphy,

> 

>  	SET_NETDEV_DEV(dev, adapter->dev);

> 

> -	/* Register network device */

> -	if (register_netdevice(dev)) {

> -		mwifiex_dbg(adapter, ERROR,

> -			    "cannot register virtual network device\n");

> -		free_netdev(dev);

> -		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;

> -		priv->netdev = NULL;

> -		memset(&priv->wdev, 0, sizeof(priv->wdev));

> -		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;

> -		return ERR_PTR(-EFAULT);

> -	}

> -

>  	priv->dfs_cac_workqueue = alloc_workqueue("MWIFIEX_DFS_CAC%s",

>  						  WQ_HIGHPRI |

>  						  WQ_MEM_RECLAIM |

>  						  WQ_UNBOUND, 1, name);

>  	if (!priv->dfs_cac_workqueue) {

> -		mwifiex_dbg(adapter, ERROR,

> -			    "cannot register virtual network device\n");

> -		free_netdev(dev);

> -		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;

> -		priv->netdev = NULL;

> -		memset(&priv->wdev, 0, sizeof(priv->wdev));

> -		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;

> -		return ERR_PTR(-ENOMEM);

> +		mwifiex_dbg(adapter, ERROR, "cannot alloc DFS CAC queue\n");

> +		ret = -ENOMEM;

> +		goto err_alloc_cac;

>  	}

> 

>  	INIT_DELAYED_WORK(&priv->dfs_cac_work,

> mwifiex_dfs_cac_work_queue); @@ -3044,16 +3025,9 @@ struct wireless_dev

> *mwifiex_add_virtual_intf(struct wiphy *wiphy,

>  						      WQ_HIGHPRI | WQ_UNBOUND |

>  						      WQ_MEM_RECLAIM, 1, name);

>  	if (!priv->dfs_chan_sw_workqueue) {

> -		mwifiex_dbg(adapter, ERROR,

> -			    "cannot register virtual network device\n");

> -		free_netdev(dev);

> -		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;

> -		priv->netdev = NULL;

> -		memset(&priv->wdev, 0, sizeof(priv->wdev));

> -		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;

> -		destroy_workqueue(priv->dfs_cac_workqueue);

> -		priv->dfs_cac_workqueue = NULL;

> -		return ERR_PTR(-ENOMEM);

> +		mwifiex_dbg(adapter, ERROR, "cannot alloc DFS channel sw

> queue\n");

> +		ret = -ENOMEM;

> +		goto err_alloc_chsw;

>  	}

> 

>  	INIT_DELAYED_WORK(&priv->dfs_chan_sw_work,

> @@ -3061,6 +3035,13 @@ struct wireless_dev

> *mwifiex_add_virtual_intf(struct wiphy *wiphy,

> 

>  	sema_init(&priv->async_sem, 1);

> 

> +	/* Register network device */

> +	if (register_netdevice(dev)) {

> +		mwifiex_dbg(adapter, ERROR, "cannot register network device\n");

> +		ret = -EFAULT;

> +		goto err_reg_netdev;

> +	}

> +

>  	mwifiex_dbg(adapter, INFO,

>  		    "info: %s: Marvell 802.11 Adapter\n", dev->name);

> 

> @@ -3081,11 +3062,29 @@ struct wireless_dev

> *mwifiex_add_virtual_intf(struct wiphy *wiphy,

>  		adapter->curr_iface_comb.p2p_intf++;

>  		break;

>  	default:

> +		/* This should be dead code; checked above */

>  		mwifiex_dbg(adapter, ERROR, "type not supported\n");

>  		return ERR_PTR(-EINVAL);

>  	}

> 

>  	return &priv->wdev;

> +

> +err_reg_netdev:

> +	destroy_workqueue(priv->dfs_chan_sw_workqueue);

> +	priv->dfs_chan_sw_workqueue = NULL;

> +err_alloc_chsw:

> +	destroy_workqueue(priv->dfs_cac_workqueue);

> +	priv->dfs_cac_workqueue = NULL;

> +err_alloc_cac:

> +	free_netdev(dev);

> +	priv->netdev = NULL;

> +err_sta_init:

> +err_set_bss_mode:

> +err_alloc_netdev:

> +	memset(&priv->wdev, 0, sizeof(priv->wdev));

> +	priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;

> +	priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;

> +	return ERR_PTR(ret);

>  }

>  EXPORT_SYMBOL_GPL(mwifiex_add_virtual_intf);

> 


Regards,
Simon

> --

> 2.13.0.rc2.291.g57267f2277-goog
Kalle Valo May 19, 2017, 6:02 a.m. UTC | #2
Brian Norris <briannorris@chromium.org> wrote:
> If we fail to add an interface in mwifiex_add_virtual_intf(), we might
> hit a BUG_ON() in the networking code, because we didn't tear things
> down properly. Among the problems:
> 
>  (a) when failing to allocate workqueues, we fail to unregister the
>      netdev before calling free_netdev()
>  (b) even if we do try to unregister the netdev, we're still holding the
>      rtnl lock, so the device never properly unregistered; we'll be at
>      state NETREG_UNREGISTERING, and then hit free_netdev()'s:
> 	BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
>  (c) we're allocating some dependent resources (e.g., DFS workqueues)
>      after we've registered the interface; this may or may not cause
>      problems, but it's good practice to allocate these before registering
>  (d) we're not even trying to unwind anything when mwifiex_send_cmd() or
>      mwifiex_sta_init_cmd() fail
> 
> To fix these issues, let's:
> 
>  * add a stacked set of error handling labels, to keep error handling
>    consistent and properly ordered (resolving (a) and (d))
>  * move the workqueue allocations before the registration (to resolve
>    (c); also resolves (b) by avoiding error cases where we have to
>    unregister)
> 
> [Incidentally, it's pretty easy to interrupt the alloc_workqueue() in,
> e.g., the following:
> 
>   iw phy phy0 interface add mlan0 type station
> 
> by sending it SIGTERM.]
> 
> This bugfix covers commits like commit 7d652034d1a0 ("mwifiex: channel
> switch support for mwifiex"), but parts of this bug exist all the way
> back to the introduction of dynamic interface handling in commit
> 93a1df48d224 ("mwifiex: add cfg80211 handlers add/del_virtual_intf").
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

11 patches applied to wireless-drivers-next.git, thanks.

8535107aa4ef mwifiex: fixup error cases in mwifiex_add_virtual_intf()
e0b636e5ee15 mwifiex: Don't release tx_ba_stream_tbl_lock while iterating
90ad0be83676 mwifiex: Don't release cmd_pending_q_lock while iterating
09bdb6500551 mwifiex: Add locking to mwifiex_11n_delba
0f13acf0c612 mwifiex: don't drop lock between list-retrieval / list-deletion
6eb2d002d4ea mwifiex: don't leak stashed beacon buffer on reset
bc69ca391eff mwifiex: remove useless 'mwifiex_lock'
7170862738dc mwifiex: remove redundant 'adapter' check in mwifiex_adapter_cleanup
7ade530e7384 mwifiex: 11h: drop unnecessary check for '!priv'
fa4651e12ae8 mwifiex: pcie: remove useless pdev check
68efd0386988 mwifiex: pcie: stop setting/clearing 'surprise_removed'
diff mbox

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 7ec06bf13413..025bc06a19d6 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -2964,10 +2964,8 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 	if (!dev) {
 		mwifiex_dbg(adapter, ERROR,
 			    "no memory available for netdevice\n");
-		memset(&priv->wdev, 0, sizeof(priv->wdev));
-		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
-		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto err_alloc_netdev;
 	}
 
 	mwifiex_init_priv_params(priv, dev);
@@ -2976,11 +2974,11 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 	ret = mwifiex_send_cmd(priv, HostCmd_CMD_SET_BSS_MODE,
 			       HostCmd_ACT_GEN_SET, 0, NULL, true);
 	if (ret)
-		return ERR_PTR(ret);
+		goto err_set_bss_mode;
 
 	ret = mwifiex_sta_init_cmd(priv, false, false);
 	if (ret)
-		return ERR_PTR(ret);
+		goto err_sta_init;
 
 	mwifiex_setup_ht_caps(&wiphy->bands[NL80211_BAND_2GHZ]->ht_cap, priv);
 	if (adapter->is_hw_11ac_capable)
@@ -3011,31 +3009,14 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 
 	SET_NETDEV_DEV(dev, adapter->dev);
 
-	/* Register network device */
-	if (register_netdevice(dev)) {
-		mwifiex_dbg(adapter, ERROR,
-			    "cannot register virtual network device\n");
-		free_netdev(dev);
-		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
-		priv->netdev = NULL;
-		memset(&priv->wdev, 0, sizeof(priv->wdev));
-		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
-		return ERR_PTR(-EFAULT);
-	}
-
 	priv->dfs_cac_workqueue = alloc_workqueue("MWIFIEX_DFS_CAC%s",
 						  WQ_HIGHPRI |
 						  WQ_MEM_RECLAIM |
 						  WQ_UNBOUND, 1, name);
 	if (!priv->dfs_cac_workqueue) {
-		mwifiex_dbg(adapter, ERROR,
-			    "cannot register virtual network device\n");
-		free_netdev(dev);
-		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
-		priv->netdev = NULL;
-		memset(&priv->wdev, 0, sizeof(priv->wdev));
-		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
-		return ERR_PTR(-ENOMEM);
+		mwifiex_dbg(adapter, ERROR, "cannot alloc DFS CAC queue\n");
+		ret = -ENOMEM;
+		goto err_alloc_cac;
 	}
 
 	INIT_DELAYED_WORK(&priv->dfs_cac_work, mwifiex_dfs_cac_work_queue);
@@ -3044,16 +3025,9 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 						      WQ_HIGHPRI | WQ_UNBOUND |
 						      WQ_MEM_RECLAIM, 1, name);
 	if (!priv->dfs_chan_sw_workqueue) {
-		mwifiex_dbg(adapter, ERROR,
-			    "cannot register virtual network device\n");
-		free_netdev(dev);
-		priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
-		priv->netdev = NULL;
-		memset(&priv->wdev, 0, sizeof(priv->wdev));
-		priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
-		destroy_workqueue(priv->dfs_cac_workqueue);
-		priv->dfs_cac_workqueue = NULL;
-		return ERR_PTR(-ENOMEM);
+		mwifiex_dbg(adapter, ERROR, "cannot alloc DFS channel sw queue\n");
+		ret = -ENOMEM;
+		goto err_alloc_chsw;
 	}
 
 	INIT_DELAYED_WORK(&priv->dfs_chan_sw_work,
@@ -3061,6 +3035,13 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 
 	sema_init(&priv->async_sem, 1);
 
+	/* Register network device */
+	if (register_netdevice(dev)) {
+		mwifiex_dbg(adapter, ERROR, "cannot register network device\n");
+		ret = -EFAULT;
+		goto err_reg_netdev;
+	}
+
 	mwifiex_dbg(adapter, INFO,
 		    "info: %s: Marvell 802.11 Adapter\n", dev->name);
 
@@ -3081,11 +3062,29 @@  struct wireless_dev *mwifiex_add_virtual_intf(struct wiphy *wiphy,
 		adapter->curr_iface_comb.p2p_intf++;
 		break;
 	default:
+		/* This should be dead code; checked above */
 		mwifiex_dbg(adapter, ERROR, "type not supported\n");
 		return ERR_PTR(-EINVAL);
 	}
 
 	return &priv->wdev;
+
+err_reg_netdev:
+	destroy_workqueue(priv->dfs_chan_sw_workqueue);
+	priv->dfs_chan_sw_workqueue = NULL;
+err_alloc_chsw:
+	destroy_workqueue(priv->dfs_cac_workqueue);
+	priv->dfs_cac_workqueue = NULL;
+err_alloc_cac:
+	free_netdev(dev);
+	priv->netdev = NULL;
+err_sta_init:
+err_set_bss_mode:
+err_alloc_netdev:
+	memset(&priv->wdev, 0, sizeof(priv->wdev));
+	priv->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
+	priv->bss_mode = NL80211_IFTYPE_UNSPECIFIED;
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(mwifiex_add_virtual_intf);