[PATCH] mac80211: clear ifmgd->bssid only after building DELBA

[PATCH] mac80211: clear ifmgd->bssid only after building DELBA

[Eliad Peller]

ieee80211_set_disassoc() clears ifmgd->bssid before
building DELBA frames, resulting in frames with invalid
bssid ("00:00:00:00:00:00").

Fix it by clearing ifmgd->bssid only after building
all the needed frames.

After this change, we no longer need to save the
bssid (before clearing it), so remove the local array.

Reported-by: Ido Yariv <ido@wizery.com>
Cc: stable@vger.kernel.org
Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 net/mac80211/mlme.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index da52587..96a7f64 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1327,7 +1327,6 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	struct sta_info *sta;
 	u32 changed = 0;
-	u8 bssid[ETH_ALEN];
 
 	ASSERT_MGD_MTX(ifmgd);
 
@@ -1337,10 +1336,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	if (WARN_ON(!ifmgd->associated))
 		return;
 
-	memcpy(bssid, ifmgd->associated->bssid, ETH_ALEN);
-
 	ifmgd->associated = NULL;
-	memset(ifmgd->bssid, 0, ETH_ALEN);
 
 	/*
 	 * we need to commit the associated = NULL change because the
@@ -1360,7 +1356,7 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	netif_carrier_off(sdata->dev);
 
 	mutex_lock(&local->sta_mtx);
-	sta = sta_info_get(sdata, bssid);
+	sta = sta_info_get(sdata, ifmgd->bssid);
 	if (sta) {
 		set_sta_flag(sta, WLAN_STA_BLOCK_BA);
 		ieee80211_sta_tear_down_BA_sessions(sta, tx);
@@ -1369,13 +1365,16 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 
 	/* deauthenticate/disassociate now */
 	if (tx || frame_buf)
-		ieee80211_send_deauth_disassoc(sdata, bssid, stype, reason,
-					       tx, frame_buf);
+		ieee80211_send_deauth_disassoc(sdata, ifmgd->bssid, stype,
+					       reason, tx, frame_buf);
 
 	/* flush out frame */
 	if (tx)
 		drv_flush(local, false);
 
+	/* clear bssid only after building the needed mgmt frames */
+	memset(ifmgd->bssid, 0, ETH_ALEN);
+
 	/* remove AP and TDLS peers */
 	sta_info_flush(local, sdata);
 
-- 
1.7.6.401.g6a319

Re: [PATCH] mac80211: clear ifmgd->bssid only after building DELBA

[Johannes Berg]

On Fri, 2012-06-01 at 11:14 +0300, Eliad Peller wrote:
> ieee80211_set_disassoc() clears ifmgd->bssid before
> building DELBA frames, resulting in frames with invalid
> bssid ("00:00:00:00:00:00").
> 
> Fix it by clearing ifmgd->bssid only after building
> all the needed frames.
> 
> After this change, we no longer need to save the
> bssid (before clearing it), so remove the local array.

I'm not convinced clearing the BSSID this late is correct, there's a lot
of code using it, for example comparing it on RX, that we may not want
to have it at this point any more.

It may well be that this is no longer a concern, but I wouldn't dismiss
it right away.

johannes

Re: [PATCH] mac80211: clear ifmgd->bssid only after building DELBA

[Eliad Peller]

On Fri, Jun 1, 2012 at 11:21 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2012-06-01 at 11:14 +0300, Eliad Peller wrote:
>> ieee80211_set_disassoc() clears ifmgd->bssid before
>> building DELBA frames, resulting in frames with invalid
>> bssid ("00:00:00:00:00:00").
>>
>> Fix it by clearing ifmgd->bssid only after building
>> all the needed frames.
>>
>> After this change, we no longer need to save the
>> bssid (before clearing it), so remove the local array.
>
> I'm not convinced clearing the BSSID this late is correct, there's a lot
> of code using it, for example comparing it on RX, that we may not want
> to have it at this point any more.
>
> It may well be that this is no longer a concern, but I wouldn't dismiss
> it right away.
>
i can't really spot any significant difference between these 2 locations.
i only deferred it latter in the same function (after stopping the
queues, setting carrier off, and sending DELBA + deauth). do you see
anything that might go wrong?

Eliad.

Re: [PATCH] mac80211: clear ifmgd->bssid only after building DELBA

[Johannes Berg]

On Fri, 2012-06-01 at 11:40 +0300, Eliad Peller wrote:
> On Fri, Jun 1, 2012 at 11:21 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Fri, 2012-06-01 at 11:14 +0300, Eliad Peller wrote:
> >> ieee80211_set_disassoc() clears ifmgd->bssid before
> >> building DELBA frames, resulting in frames with invalid
> >> bssid ("00:00:00:00:00:00").
> >>
> >> Fix it by clearing ifmgd->bssid only after building
> >> all the needed frames.
> >>
> >> After this change, we no longer need to save the
> >> bssid (before clearing it), so remove the local array.
> >
> > I'm not convinced clearing the BSSID this late is correct, there's a lot
> > of code using it, for example comparing it on RX, that we may not want
> > to have it at this point any more.
> >
> > It may well be that this is no longer a concern, but I wouldn't dismiss
> > it right away.
> >
> i can't really spot any significant difference between these 2 locations.
> i only deferred it latter in the same function (after stopping the
> queues, setting carrier off, and sending DELBA + deauth). do you see
> anything that might go wrong?

I took another look, and I guess I don't see anything now either. Let's
try this patch :-)

One side effect from clearing the BSSID early was also that we sometimes
(if there was pending traffic while disassociating) got the message
"dropped from to unauthorized port 00:00:00:00:00:00" or so ... so that
should go away too.

johannes

Re: [PATCH] mac80211: clear ifmgd->bssid only after building DELBA

[Johannes Berg]

On Fri, 2012-06-01 at 11:14 +0300, Eliad Peller wrote:
> ieee80211_set_disassoc() clears ifmgd->bssid before
> building DELBA frames, resulting in frames with invalid
> bssid ("00:00:00:00:00:00").
> 
> Fix it by clearing ifmgd->bssid only after building
> all the needed frames.
> 
> After this change, we no longer need to save the
> bssid (before clearing it), so remove the local array.

Applied to mac80211.git, sorry I missed this, I thought John had picked
it up. I guess I'll have to send a pull request with this patch soon :)

johannes