[PATCH] mac80211: stop Rx during HW reconfig

[PATCH] mac80211: stop Rx during HW reconfig

[Arik Nemtsov]

While HW reconfig is in progress, drop all incoming Rx. This prevents
incoming packets from changing the internal state of the driver or
calling callbacks of the low level driver while it is in inconsistent
state.

Signed-off-by: Arik Nemtsov <arik@wizery.com>
---
 net/mac80211/ieee80211_i.h |    3 +++
 net/mac80211/rx.c          |    7 +++++++
 net/mac80211/util.c        |   10 ++++++++++
 3 files changed, 20 insertions(+)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 1975f35..9b915c6 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -912,6 +912,9 @@ struct ieee80211_local {
 	/* device is started */
 	bool started;
 
+	/* device is during a HW reconfig */
+	bool in_reconfig;
+
 	/* wowlan is enabled -- don't reconfig on resume */
 	bool wowlan;
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 489093b..d08b431 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3023,6 +3023,9 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	if (WARN_ON(!sband))
 		goto drop;
 
+	/* make sure we get the latest values for the next variable checks */
+	smp_rmb();
+
 	/*
 	 * If we're suspending, it is possible although not too likely
 	 * that we'd be receiving frames after having already partially
@@ -3033,6 +3036,10 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
 	if (unlikely(local->quiescing || local->suspended))
 		goto drop;
 
+	/* We might be during a HW reconfig, prevent Rx for the same reason */
+	if (unlikely(local->in_reconfig))
+		goto drop;
+
 	/*
 	 * The same happens when we're not even started,
 	 * but that's worth a warning.
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 7c7e571..f369e5e 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1233,6 +1233,13 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 		return res;
 	}
 
+	/*
+	 * Stop all Rx during the reconfig. We don't want state changes
+	 * or driver callbacks while this is in progress.
+	 */
+	local->in_reconfig = true;
+	smp_wmb();
+
 	/* setup fragmentation threshold */
 	drv_set_frag_threshold(local, hw->wiphy->frag_threshold);
 
@@ -1376,6 +1383,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
 		if (ieee80211_sdata_running(sdata))
 			ieee80211_enable_keys(sdata);
 
+	local->in_reconfig = false;
+	smp_wmb();
+
  wake_up:
 	/*
 	 * Clear the WLAN_STA_BLOCK_BA flag so new aggregation
-- 
1.7.9.5

Re: [PATCH] mac80211: stop Rx during HW reconfig

[Johannes Berg]

On Sun, 2012-06-03 at 23:30 +0300, Arik Nemtsov wrote:

> +++ b/net/mac80211/rx.c
> @@ -3023,6 +3023,9 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
>  	if (WARN_ON(!sband))
>  		goto drop;
>  
> +	/* make sure we get the latest values for the next variable checks */
> +	smp_rmb();
> +

That doesn't make a lot of sense I think? This entire thing is
inherently racy, you could be processing RX already even while just
setting the variable in the other code. I'm not really sure what the
effect of this here is, but it's certainly a hot-path.

> +       /*
> +        * Stop all Rx during the reconfig. We don't want state changes
> +        * or driver callbacks while this is in progress.
> +        */
> +       local->in_reconfig = true;
> +       smp_wmb();


If anything, you should probably put this right into
ieee80211_restart_hw() rather than the reconfig function? It's not
needed for suspend/resume anyway since we check that separately.

johannes

Re: [PATCH] mac80211: stop Rx during HW reconfig

[Arik Nemtsov]

On Mon, Jun 4, 2012 at 9:16 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2012-06-03 at 23:30 +0300, Arik Nemtsov wrote:
>
>> +++ b/net/mac80211/rx.c
>> @@ -3023,6 +3023,9 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb)
>>       if (WARN_ON(!sband))
>>               goto drop;
>>
>> +     /* make sure we get the latest values for the next variable checks */
>> +     smp_rmb();
>> +
>
> That doesn't make a lot of sense I think? This entire thing is
> inherently racy, you could be processing RX already even while just
> setting the variable in the other code. I'm not really sure what the
> effect of this here is, but it's certainly a hot-path.

Yea I wondered about that myself, but I saw spinlocks are being used
later inside ieee80211_rx_handlers(), so there's a memory barrier
anyway IIUC.

You're right that we still can get very old RX from before the
reconfig, but this is not the one I was worried about (and it doesn't
happen in practice).
This one worries me (we actually had something similar)

1. reconfig starts
2. AP starts beaconing
3. remote sta sends ADDBA
4. remote sta is added by mac80211

Since in_reconfig is set before the start of the AP, in this sense we
won't get state changes during the reconfig.

>
>> +       /*
>> +        * Stop all Rx during the reconfig. We don't want state changes
>> +        * or driver callbacks while this is in progress.
>> +        */
>> +       local->in_reconfig = true;
>> +       smp_wmb();
>
>
> If anything, you should probably put this right into
> ieee80211_restart_hw() rather than the reconfig function? It's not
> needed for suspend/resume anyway since we check that separately.

Sure, even earlier is better I guess.

Arik