[ath9k-devel] [PATCH] mac80211: ath9k: Use RCU protection calling ieee80211_get_tx_rates

[ath9k-devel] [PATCH] mac80211: ath9k: Use RCU protection calling ieee80211_get_tx_rates

[Calvin Owens]

Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
---
 drivers/net/wireless/ath/ath9k/xmit.c | 2 ++
 drivers/net/wireless/mac80211_hwsim.c | 6 ++++++
 include/net/mac80211.h                | 3 ++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 1c9b1ba..1d57015 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -142,8 +142,10 @@ static void ath_send_bar(struct ath_atx_tid *tid, u16 seqno)
 static void ath_set_rates(struct ieee80211_vif *vif, struct ieee80211_sta *sta,
 			  struct ath_buf *bf)
 {
+	rcu_read_lock();
 	ieee80211_get_tx_rates(vif, sta, bf->bf_mpdu, bf->rates,
 			       ARRAY_SIZE(bf->rates));
+	rcu_read_unlock();
 }
 
 static void ath_tx_flush_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index cb34c78..468a5a6 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -900,9 +900,11 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
 		hwsim_check_sta_magic(control->sta);
 
 	if (rctbl)
+		rcu_read_lock();
 		ieee80211_get_tx_rates(txi->control.vif, control->sta, skb,
 				       txi->control.rates,
 				       ARRAY_SIZE(txi->control.rates));
+		rcu_read_unlock();
 
 	txi->rate_driver_data[0] = channel;
 	mac80211_hwsim_monitor_rx(hw, skb, channel);
@@ -1008,9 +1010,11 @@ static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
 
 	if (rctbl) {
 		struct ieee80211_tx_info *txi = IEEE80211_SKB_CB(skb);
+		rcu_read_lock();
 		ieee80211_get_tx_rates(txi->control.vif, NULL, skb,
 				       txi->control.rates,
 				       ARRAY_SIZE(txi->control.rates));
+		rcu_read_unlock();
 	}
 
 	mac80211_hwsim_monitor_rx(hw, skb, chan);
@@ -1044,9 +1048,11 @@ static void mac80211_hwsim_beacon_tx(void *arg, u8 *mac,
 		return;
 	info = IEEE80211_SKB_CB(skb);
 	if (rctbl)
+		rcu_read_lock();
 		ieee80211_get_tx_rates(vif, NULL, skb,
 				       info->control.rates,
 				       ARRAY_SIZE(info->control.rates));
+		rcu_read_unlock();
 
 	txrate = ieee80211_get_tx_rate(hw, info);
 
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 885898a..df345c1 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3175,7 +3175,8 @@ void ieee80211_sta_set_buffered(struct ieee80211_sta *sta,
  *
  * Call this function in a driver with per-packet rate selection support
  * to combine the rate info in the packet tx info with the most recent
- * rate selection table for the station entry.
+ * rate selection table for the station entry. Must be called with RCU
+ * protection.
  *
  * @vif: &struct ieee80211_vif pointer from the add_interface callback.
  * @sta: the receiver station to which this packet is sent.
-- 
1.8.2.1

[ath9k-devel] [PATCH] mac80211: ath9k: Use RCU protection calling ieee80211_get_tx_rates

[Johannes Berg]

Now the subject should no longer say "mac80211:" :)

However... given that ieee80211_get_tx_rates() actually *copies* the
rates into the parameter, I guess it should do the rcu_read_lock()
internally. I guess I wasn't paying attention previously. Felix?

johannes

[ath9k-devel] [PATCH] mac80211: ath9k: Use RCU protection calling ieee80211_get_tx_rates

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> Now the subject should no longer say "mac80211:" :)

It did have a change to mac80211.h as well, but IMHO that should be in a
separate patch.

-- 
Kalle Valo

[ath9k-devel] [PATCH] mac80211: ath9k: Use RCU protection calling ieee80211_get_tx_rates

[Calvin Owens]

On Tuesday 06/11 at 21:55 +0200, Johannes Berg wrote:
> Now the subject should no longer say "mac80211:" :)
> 
> However... given that ieee80211_get_tx_rates() actually *copies* the
> rates into the parameter, I guess it should do the rcu_read_lock()
> internally. I guess I wasn't paying attention previously. Felix?

I thought about that, but it seemed too simple, so I assumed I was
missing something. ;)

I'll send a patch per above. That makes the header file update
unnecessary too.

> johannes

[ath9k-devel] [PATCH] mac80211: Use RCU protection in ieee80211_get_tx_rates()

[Calvin Owens]

Copying the rate table should be done in an RCU read-side critical
section.

Signed-off-by: Calvin Owens <jcalvinowens@gmail.com>
---
 net/mac80211/rate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index d3f414f..090d9b0 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -538,6 +538,8 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
 	struct ieee80211_sta_rates *ratetbl = NULL;
 	int i;
 
+	rcu_read_lock();
+
 	if (sta && !info->control.skip_table)
 		ratetbl = rcu_dereference(sta->rates);
 
@@ -566,6 +568,8 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
 		if (rates[i].idx < 0 || !rates[i].count)
 			break;
 	}
+
+	rcu_read_unlock();
 }
 
 static void rate_control_apply_mask(struct ieee80211_sub_if_data *sdata,
-- 
1.8.2.1

[ath9k-devel] [PATCH] mac80211: Use RCU protection in ieee80211_get_tx_rates()

[Felix Fietkau]

On 2013-06-12 10:00 AM, Calvin Owens wrote:
> Copying the rate table should be done in an RCU read-side critical
> section.
I think this approach is wrong. The sta entry is also under RCU
protection (no locking for read access in that part of the code.
In a normal driver tx path, no extra rcu_read_lock/rcu_read_unlock is
needed. Only if the driver does some scheduling outside of the tx
function (which ath9k does), this RCU warning appears.

How about this change instead:
---
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -1570,6 +1570,8 @@ void ath_txq_schedule(struct ath_softc *
 	    txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH)
 		return;
 
+	rcu_read_lock();
+
 	ac = list_first_entry(&txq->axq_acq, struct ath_atx_ac, list);
 	last_ac = list_entry(txq->axq_acq.prev, struct ath_atx_ac, list);
 
@@ -1608,8 +1610,10 @@ void ath_txq_schedule(struct ath_softc *
 
 		if (ac == last_ac ||
 		    txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH)
-			return;
+			break;
 	}
+
+	rcu_read_unlock();
 }
 
 /***********/

[ath9k-devel] [PATCH] mac80211: Use RCU protection in ieee80211_get_tx_rates()

[Calvin Owens]

On Wednesday 06/12 at 11:47 +0200, Felix Fietkau wrote:
> On 2013-06-12 10:00 AM, Calvin Owens wrote:
> > Copying the rate table should be done in an RCU read-side critical
> > section.
> I think this approach is wrong. The sta entry is also under RCU
> protection (no locking for read access in that part of the code.
> In a normal driver tx path, no extra rcu_read_lock/rcu_read_unlock is
> needed. Only if the driver does some scheduling outside of the tx
> function (which ath9k does), this RCU warning appears.
> 
> How about this change instead:
> ---
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -1570,6 +1570,8 @@ void ath_txq_schedule(struct ath_softc *
>  	    txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH)
>  		return;
>  
> +	rcu_read_lock();
> +
>  	ac = list_first_entry(&txq->axq_acq, struct ath_atx_ac, list);
>  	last_ac = list_entry(txq->axq_acq.prev, struct ath_atx_ac, list);
>  
> @@ -1608,8 +1610,10 @@ void ath_txq_schedule(struct ath_softc *
>  
>  		if (ac == last_ac ||
>  		    txq->axq_ampdu_depth >= ATH_AGGR_MIN_QDEPTH)
> -			return;
> +			break;
>  	}
> +
> +	rcu_read_unlock();
>  }
>  
>  /***********/
>

Yep, that stops the RCU warning for me.

Tested-by: Calvin Owens <jcalvinowens@gmail.com>