[PATCH 10/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

[PATCH 10/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

[Lennert Buytenhek]

mwl8k_configure_filter() passes pointers to total_flags and the
multicast address list to a workqueue function, while there is no
guarantee that those pointers will still be valid by the time the
workqueue function runs.

Solve this by passing total_flags by value, and by passing an
already built multicast address setup command packet to the workqueue
function so that we don't have to look at the multicast address list
itself outside of mwl8k_configure_filter().

Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
---
 drivers/net/wireless/mwl8k.c |   72 +++++++++++++++++------------------------
 1 files changed, 30 insertions(+), 42 deletions(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index 3a201a2..e266bc1 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -1613,38 +1613,39 @@ struct mwl8k_cmd_mac_multicast_adr {
 
 #define MWL8K_ENABLE_RX_MULTICAST 0x000F
 
-static int mwl8k_cmd_mac_multicast_adr(struct ieee80211_hw *hw,
-					int mc_count,
-					struct dev_addr_list *mclist)
+static struct mwl8k_cmd_pkt *
+__mwl8k_cmd_mac_multicast_adr(struct ieee80211_hw *hw,
+			      int mc_count, struct dev_addr_list *mclist)
 {
+	struct mwl8k_priv *priv = hw->priv;
 	struct mwl8k_cmd_mac_multicast_adr *cmd;
-	int index = 0;
-	int rc;
-	int size = sizeof(*cmd) + mc_count * ETH_ALEN;
+	int size;
+	int i;
+
+	if (mc_count > priv->num_mcaddrs)
+		mc_count = priv->num_mcaddrs;
 
-	cmd = kzalloc(size, GFP_KERNEL);
+	size = sizeof(*cmd) + mc_count * ETH_ALEN;
+
+	cmd = kzalloc(size, GFP_ATOMIC);
 	if (cmd == NULL)
-		return -ENOMEM;
+		return NULL;
 
 	cmd->header.code = cpu_to_le16(MWL8K_CMD_MAC_MULTICAST_ADR);
 	cmd->header.length = cpu_to_le16(size);
 	cmd->action = cpu_to_le16(MWL8K_ENABLE_RX_MULTICAST);
 	cmd->numaddr = cpu_to_le16(mc_count);
 
-	while (index < mc_count && mclist) {
+	for (i = 0; i < mc_count && mclist; i++) {
 		if (mclist->da_addrlen != ETH_ALEN) {
-			rc = -EINVAL;
-			goto mwl8k_cmd_mac_multicast_adr_exit;
+			kfree(cmd);
+			return NULL;
 		}
-		memcpy(cmd->addr[index++], mclist->da_addr, ETH_ALEN);
+		memcpy(cmd->addr[i], mclist->da_addr, ETH_ALEN);
 		mclist = mclist->next;
 	}
 
-	rc = mwl8k_post_cmd(hw, &cmd->header);
-
-mwl8k_cmd_mac_multicast_adr_exit:
-	kfree(cmd);
-	return rc;
+	return &cmd->header;
 }
 
 /*
@@ -3101,9 +3102,8 @@ static void mwl8k_bss_info_changed(struct ieee80211_hw *hw,
 struct mwl8k_configure_filter_worker {
 	struct mwl8k_work_struct header;
 	unsigned int changed_flags;
-	unsigned int *total_flags;
-	int mc_count;
-	struct dev_addr_list *mclist;
+	unsigned int total_flags;
+	struct mwl8k_cmd_pkt *set_multicast_adr;
 };
 
 #define MWL8K_SUPPORTED_IF_FLAGS	FIF_BCN_PRBRESP_PROMISC
@@ -3112,18 +3112,14 @@ static int mwl8k_configure_filter_wt(struct work_struct *wt)
 {
 	struct mwl8k_configure_filter_worker *worker =
 		(struct mwl8k_configure_filter_worker *)wt;
-
 	struct ieee80211_hw *hw = worker->header.hw;
 	unsigned int changed_flags = worker->changed_flags;
-	unsigned int *total_flags = worker->total_flags;
-	int mc_count = worker->mc_count;
-	struct dev_addr_list *mclist = worker->mclist;
-
+	unsigned int total_flags = worker->total_flags;
 	struct mwl8k_priv *priv = hw->priv;
 	int rc = 0;
 
 	if (changed_flags & FIF_BCN_PRBRESP_PROMISC) {
-		if (*total_flags & FIF_BCN_PRBRESP_PROMISC)
+		if (total_flags & FIF_BCN_PRBRESP_PROMISC)
 			rc = mwl8k_cmd_set_pre_scan(hw);
 		else {
 			u8 *bssid;
@@ -3136,20 +3132,12 @@ static int mwl8k_configure_filter_wt(struct work_struct *wt)
 		}
 	}
 
-	if (rc)
-		goto mwl8k_configure_filter_exit;
-	if (mc_count) {
-		if (mc_count > priv->num_mcaddrs)
-			mc_count = priv->num_mcaddrs;
-
-		rc = mwl8k_cmd_mac_multicast_adr(hw, mc_count, mclist);
-		if (rc)
-			printk(KERN_ERR
-			"%s()Error setting multicast addresses\n",
-			__func__);
+	if (worker->set_multicast_adr != NULL) {
+		if (!rc)
+			rc = mwl8k_post_cmd(hw, worker->set_multicast_adr);
+		kfree(worker->set_multicast_adr);
 	}
 
-mwl8k_configure_filter_exit:
 	return rc;
 }
 
@@ -3159,7 +3147,6 @@ static void mwl8k_configure_filter(struct ieee80211_hw *hw,
 				   int mc_count,
 				   struct dev_addr_list *mclist)
 {
-
 	struct mwl8k_configure_filter_worker *worker;
 	struct mwl8k_priv *priv = hw->priv;
 
@@ -3175,9 +3162,10 @@ static void mwl8k_configure_filter(struct ieee80211_hw *hw,
 
 	worker->header.options = MWL8K_WQ_QUEUE_ONLY | MWL8K_WQ_TX_WAIT_EMPTY;
 	worker->changed_flags = changed_flags;
-	worker->total_flags = total_flags;
-	worker->mc_count = mc_count;
-	worker->mclist = mclist;
+	worker->total_flags = *total_flags;
+	if (mc_count)
+		worker->set_multicast_adr =
+			__mwl8k_cmd_mac_multicast_adr(hw, mc_count, mclist);
 
 	mwl8k_queue_work(hw, &worker->header, priv->config_wq,
 			 mwl8k_configure_filter_wt);
-- 
1.5.6.4

Re: [PATCH 10/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

[Lennert Buytenhek]

On Tue, Aug 18, 2009 at 05:52:33AM +0200, Lennert Buytenhek wrote:

> mwl8k_configure_filter() passes pointers to total_flags and the
> multicast address list to a workqueue function, while there is no
> guarantee that those pointers will still be valid by the time the
> workqueue function runs.
> 
> Solve this by passing total_flags by value, and by passing an
> already built multicast address setup command packet to the workqueue
> function so that we don't have to look at the multicast address list
> itself outside of mwl8k_configure_filter().
> 
> Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>

This will clash with:

	http://article.gmane.org/gmane.linux.kernel.wireless.general/38141

But that is easy to fix up.  (The easiest would probably be to not
apply the mwl8k part of that patch, and I'll send a followup patch
to implement ->prepare_multicast() for mwl8k.)

Re: [PATCH 10/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

On Tue, 2009-08-18 at 06:00 +0200, Lennert Buytenhek wrote:

> This will clash with:
> 
> 	http://article.gmane.org/gmane.linux.kernel.wireless.general/38141
> 
> But that is easy to fix up.  (The easiest would probably be to not
> apply the mwl8k part of that patch, and I'll send a followup patch
> to implement ->prepare_multicast() for mwl8k.)

It's not quite that easy because that patch changes the arguments to the
functions so the multicast list is no longer available in
->configure_filter().

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 10/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

[Lennert Buytenhek]

On Tue, Aug 18, 2009 at 09:23:18AM +0200, Johannes Berg wrote:

> > This will clash with:
> > 
> > 	http://article.gmane.org/gmane.linux.kernel.wireless.general/38141
> > 
> > But that is easy to fix up.  (The easiest would probably be to not
> > apply the mwl8k part of that patch, and I'll send a followup patch
> > to implement ->prepare_multicast() for mwl8k.)
> 
> It's not quite that easy because that patch changes the arguments to the
> functions so the multicast list is no longer available in
> ->configure_filter().

OK, if my patch set gets applied first, please use the mwl8k
->prepare_multicast() implementation that's below in your patch.

If your patch gets applied first, I'll fix it up in my set.



diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index 2c7c85d..316a728 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -2654,26 +2654,33 @@ out:
 	mwl8k_fw_unlock(hw);
 }
 
-struct mwl8k_configure_filter_worker {
-	struct work_struct wt;
-	struct ieee80211_hw *hw;
-	unsigned int changed_flags;
-	unsigned int total_flags;
-	struct mwl8k_cmd_pkt *set_multicast_adr;
-};
+static u64 mwl8k_prepare_multicast(struct ieee80211_hw *hw,
+				   int mc_count, struct dev_addr_list *mclist)
+{
+	struct mwl8k_cmd_pkt *cmd;
+
+	cmd = __mwl8k_cmd_mac_multicast_adr(hw, mc_count, mclist);
 
-static void mwl8k_configure_filter_wt(struct work_struct *wt)
+	return (unsigned long)cmd;
+}
+
+static void mwl8k_configure_filter(struct ieee80211_hw *hw,
+				   unsigned int changed_flags,
+				   unsigned int *total_flags,
+				   u64 multicast)
 {
-	struct mwl8k_configure_filter_worker *worker =
-		(struct mwl8k_configure_filter_worker *)wt;
-	struct ieee80211_hw *hw = worker->hw;
 	struct mwl8k_priv *priv = hw->priv;
+	struct mwl8k_cmd_pkt *multicast_adr_cmd =
+		(void *)(unsigned long)multicast;
+
+	/* Clear unsupported feature flags */
+	*total_flags &= FIF_BCN_PRBRESP_PROMISC;
 
 	if (mwl8k_fw_lock(hw))
-		return;
+		goto out;
 
-	if (worker->changed_flags & FIF_BCN_PRBRESP_PROMISC) {
-		if (worker->total_flags & FIF_BCN_PRBRESP_PROMISC)
+	if (changed_flags & FIF_BCN_PRBRESP_PROMISC) {
+		if (*total_flags & FIF_BCN_PRBRESP_PROMISC)
 			mwl8k_cmd_set_pre_scan(hw);
 		else {
 			u8 *bssid;
@@ -2686,44 +2693,13 @@ static void mwl8k_configure_filter_wt(struct work_struct *wt)
 		}
 	}
 
-	if (worker->set_multicast_adr != NULL) {
-		mwl8k_post_cmd(hw, worker->set_multicast_adr);
-		kfree(worker->set_multicast_adr);
-	}
+	if (multicast_adr_cmd != NULL)
+		mwl8k_post_cmd(hw, multicast_adr_cmd);
 
 	mwl8k_fw_unlock(hw);
 
-	kfree(worker);
-}
-
-static void mwl8k_configure_filter(struct ieee80211_hw *hw,
-				   unsigned int changed_flags,
-				   unsigned int *total_flags,
-				   int mc_count,
-				   struct dev_addr_list *mclist)
-{
-	struct mwl8k_priv *priv = hw->priv;
-	struct mwl8k_configure_filter_worker *worker;
-
-	/* Clear unsupported feature flags */
-	*total_flags &= FIF_BCN_PRBRESP_PROMISC;
-
-	if (!(changed_flags & FIF_BCN_PRBRESP_PROMISC) && !mc_count)
-		return;
-
-	worker = kzalloc(sizeof(*worker), GFP_ATOMIC);
-	if (worker == NULL)
-		return;
-
-	worker->hw = hw;
-	worker->changed_flags = changed_flags;
-	worker->total_flags = *total_flags;
-	if (mc_count)
-		worker->set_multicast_adr =
-			__mwl8k_cmd_mac_multicast_adr(hw, mc_count, mclist);
-
-	INIT_WORK(&worker->wt, mwl8k_configure_filter_wt);
-	queue_work(priv->config_wq, &worker->wt);
+out:
+	kfree(multicast_adr_cmd);
 }
 
 static int mwl8k_set_rts_threshold(struct ieee80211_hw *hw, u32 value)
@@ -2787,6 +2763,7 @@ static const struct ieee80211_ops mwl8k_ops = {
 	.remove_interface	= mwl8k_remove_interface,
 	.config			= mwl8k_config,
 	.bss_info_changed	= mwl8k_bss_info_changed,
+	.prepare_multicast	= mwl8k_prepare_multicast,
 	.configure_filter	= mwl8k_configure_filter,
 	.set_rts_threshold	= mwl8k_set_rts_threshold,
 	.conf_tx		= mwl8k_conf_tx,

Re: [PATCH 10/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

[Lennert Buytenhek]

On Tue, Aug 18, 2009 at 10:13:39AM +0200, Lennert Buytenhek wrote:

> > > This will clash with:
> > > 
> > > 	http://article.gmane.org/gmane.linux.kernel.wireless.general/38141
> > > 
> > > But that is easy to fix up.  (The easiest would probably be to not
> > > apply the mwl8k part of that patch, and I'll send a followup patch
> > > to implement ->prepare_multicast() for mwl8k.)
> > 
> > It's not quite that easy because that patch changes the arguments to the
> > functions so the multicast list is no longer available in
> > ->configure_filter().
> 
> OK, if my patch set gets applied first, please use the mwl8k
> ->prepare_multicast() implementation that's below in your patch.
> 
> If your patch gets applied first, I'll fix it up in my set.

OK, your patch is already in, as you noted on IRC, so I've replaced
this by:



>From 9145398d5822cf3ed963fb155a2771c4facd1d85 Mon Sep 17 00:00:00 2001
From: Lennert Buytenhek <buytenh@wantstofly.org>
Date: Tue, 18 Aug 2009 03:55:42 +0200
Subject: [PATCH 09/29] mwl8k: fix mwl8k_configure_filter() parameter lifetime issue

mwl8k_configure_filter() passes pointers to total_flags and the
multicast address list to a workqueue function, while there is no
guarantee that those pointers will still be valid by the time the
workqueue function runs.

Solve this by passing total_flags by value, and by passing an
already built multicast address setup command packet to the workqueue
function so that we don't have to look at the multicast address list
itself outside of mwl8k_configure_filter().

Also, since ->configure_filter() can sleep now, wait synchronously
for the worker to finish.

Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
---
 drivers/net/wireless/mwl8k.c |  107 +++++++++++++++--------------------------
 1 files changed, 39 insertions(+), 68 deletions(-)

diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index a40434b..a961b69 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -1613,38 +1613,39 @@ struct mwl8k_cmd_mac_multicast_adr {
 
 #define MWL8K_ENABLE_RX_MULTICAST 0x000F
 
-static int mwl8k_cmd_mac_multicast_adr(struct ieee80211_hw *hw,
-					int mc_count,
-					struct dev_addr_list *mclist)
+static struct mwl8k_cmd_pkt *
+__mwl8k_cmd_mac_multicast_adr(struct ieee80211_hw *hw,
+			      int mc_count, struct dev_addr_list *mclist)
 {
+	struct mwl8k_priv *priv = hw->priv;
 	struct mwl8k_cmd_mac_multicast_adr *cmd;
-	int index = 0;
-	int rc;
-	int size = sizeof(*cmd) + mc_count * ETH_ALEN;
+	int size;
+	int i;
+
+	if (mc_count > priv->num_mcaddrs)
+		mc_count = priv->num_mcaddrs;
+
+	size = sizeof(*cmd) + mc_count * ETH_ALEN;
 
-	cmd = kzalloc(size, GFP_KERNEL);
+	cmd = kzalloc(size, GFP_ATOMIC);
 	if (cmd == NULL)
-		return -ENOMEM;
+		return NULL;
 
 	cmd->header.code = cpu_to_le16(MWL8K_CMD_MAC_MULTICAST_ADR);
 	cmd->header.length = cpu_to_le16(size);
 	cmd->action = cpu_to_le16(MWL8K_ENABLE_RX_MULTICAST);
 	cmd->numaddr = cpu_to_le16(mc_count);
 
-	while (index < mc_count && mclist) {
+	for (i = 0; i < mc_count && mclist; i++) {
 		if (mclist->da_addrlen != ETH_ALEN) {
-			rc = -EINVAL;
-			goto mwl8k_cmd_mac_multicast_adr_exit;
+			kfree(cmd);
+			return NULL;
 		}
-		memcpy(cmd->addr[index++], mclist->da_addr, ETH_ALEN);
+		memcpy(cmd->addr[i], mclist->da_addr, ETH_ALEN);
 		mclist = mclist->next;
 	}
 
-	rc = mwl8k_post_cmd(hw, &cmd->header);
-
-mwl8k_cmd_mac_multicast_adr_exit:
-	kfree(cmd);
-	return rc;
+	return &cmd->header;
 }
 
 /*
@@ -3091,12 +3092,21 @@ static void mwl8k_bss_info_changed(struct ieee80211_hw *hw,
 		printk(KERN_ERR "%s() timed out\n", __func__);
 }
 
+static u64 mwl8k_prepare_multicast(struct ieee80211_hw *hw,
+				   int mc_count, struct dev_addr_list *mclist)
+{
+	struct mwl8k_cmd_pkt *cmd;
+
+	cmd = __mwl8k_cmd_mac_multicast_adr(hw, mc_count, mclist);
+
+	return (unsigned long)cmd;
+}
+
 struct mwl8k_configure_filter_worker {
 	struct mwl8k_work_struct header;
 	unsigned int changed_flags;
-	unsigned int *total_flags;
-	int mc_count;
-	struct dev_addr_list *mclist;
+	unsigned int total_flags;
+	struct mwl8k_cmd_pkt *multicast_adr_cmd;
 };
 
 #define MWL8K_SUPPORTED_IF_FLAGS	FIF_BCN_PRBRESP_PROMISC
@@ -3105,18 +3115,12 @@ static int mwl8k_configure_filter_wt(struct work_struct *wt)
 {
 	struct mwl8k_configure_filter_worker *worker =
 		(struct mwl8k_configure_filter_worker *)wt;
-
 	struct ieee80211_hw *hw = worker->header.hw;
-	unsigned int changed_flags = worker->changed_flags;
-	unsigned int *total_flags = worker->total_flags;
-	int mc_count = worker->mc_count;
-	struct dev_addr_list *mclist = worker->mclist;
-
 	struct mwl8k_priv *priv = hw->priv;
 	int rc = 0;
 
-	if (changed_flags & FIF_BCN_PRBRESP_PROMISC) {
-		if (*total_flags & FIF_BCN_PRBRESP_PROMISC)
+	if (worker->changed_flags & FIF_BCN_PRBRESP_PROMISC) {
+		if (worker->total_flags & FIF_BCN_PRBRESP_PROMISC)
 			rc = mwl8k_cmd_set_pre_scan(hw);
 		else {
 			u8 *bssid;
@@ -3129,54 +3133,20 @@ static int mwl8k_configure_filter_wt(struct work_struct *wt)
 		}
 	}
 
-	if (rc)
-		goto mwl8k_configure_filter_exit;
-	if (mc_count) {
-		if (mc_count > priv->num_mcaddrs)
-			mc_count = priv->num_mcaddrs;
-
-		rc = mwl8k_cmd_mac_multicast_adr(hw, mc_count, mclist);
-		if (rc)
-			printk(KERN_ERR
-			"%s()Error setting multicast addresses\n",
-			__func__);
-	}
+	if (!rc && worker->multicast_adr_cmd != NULL)
+		rc = mwl8k_post_cmd(hw, worker->multicast_adr_cmd);
+	kfree(worker->multicast_adr_cmd);
 
-mwl8k_configure_filter_exit:
 	return rc;
 }
 
-static u64 mwl8k_prepare_multicast(struct ieee80211_hw *hw,
-				   int mc_count, struct dev_addr_list *mclist)
-{
-	struct mwl8k_configure_filter_worker *worker;
-
-	worker = kzalloc(sizeof(*worker), GFP_ATOMIC);
-
-	if (!worker)
-		return 0;
-
-	/*
-	 * XXX: This is _HORRIBLY_ broken!!
-	 *
-	 *	No locking, the mclist pointer might be invalid as soon as this
-	 *	function returns, something in the list might be invalidated
-	 *	once we get to the worker, etc...
-	 */
-	worker->mc_count = mc_count;
-	worker->mclist = mclist;
-
-	return (u64)worker;
-}
-
 static void mwl8k_configure_filter(struct ieee80211_hw *hw,
 				   unsigned int changed_flags,
 				   unsigned int *total_flags,
 				   u64 multicast)
 {
-
-	struct mwl8k_configure_filter_worker *worker = (void *)multicast;
 	struct mwl8k_priv *priv = hw->priv;
+	struct mwl8k_configure_filter_worker *worker;
 
 	/* Clear unsupported feature flags */
 	*total_flags &= MWL8K_SUPPORTED_IF_FLAGS;
@@ -3184,12 +3154,13 @@ static void mwl8k_configure_filter(struct ieee80211_hw *hw,
 	if (!(changed_flags & MWL8K_SUPPORTED_IF_FLAGS))
 		return;
 
+	worker = kzalloc(sizeof(*worker), GFP_ATOMIC);
 	if (worker == NULL)
 		return;
 
-	worker->header.options = MWL8K_WQ_QUEUE_ONLY | MWL8K_WQ_TX_WAIT_EMPTY;
 	worker->changed_flags = changed_flags;
-	worker->total_flags = total_flags;
+	worker->total_flags = *total_flags;
+	worker->multicast_adr_cmd = (void *)(unsigned long)multicast;
 
 	mwl8k_queue_work(hw, &worker->header, priv->config_wq,
 			 mwl8k_configure_filter_wt);
-- 
1.5.6.4