Document rx vs tx status concurrency requirements.
Signed-off-by: Felix Fietkau <[email protected]>
---
include/net/mac80211.h | 12 ++++++++----
net/mac80211/rate.c | 9 ++++++++-
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 04c2d46..885898a 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3043,7 +3043,8 @@ void ieee80211_napi_complete(struct ieee80211_hw *hw);
* This function may not be called in IRQ context. Calls to this function
* for a single hardware must be synchronized against each other. Calls to
* this function, ieee80211_rx_ni() and ieee80211_rx_irqsafe() may not be
- * mixed for a single hardware.
+ * mixed for a single hardware. Must not run concurrently with
+ * ieee80211_tx_status() or ieee80211_tx_status_ni().
*
* In process context use instead ieee80211_rx_ni().
*
@@ -3059,7 +3060,8 @@ void ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb);
* (internally defers to a tasklet.)
*
* Calls to this function, ieee80211_rx() or ieee80211_rx_ni() may not
- * be mixed for a single hardware.
+ * be mixed for a single hardware.Must not run concurrently with
+ * ieee80211_tx_status() or ieee80211_tx_status_ni().
*
* @hw: the hardware this frame came in on
* @skb: the buffer to receive, owned by mac80211 after this call
@@ -3073,7 +3075,8 @@ void ieee80211_rx_irqsafe(struct ieee80211_hw *hw, struct sk_buff *skb);
* (internally disables bottom halves).
*
* Calls to this function, ieee80211_rx() and ieee80211_rx_irqsafe() may
- * not be mixed for a single hardware.
+ * not be mixed for a single hardware. Must not run concurrently with
+ * ieee80211_tx_status() or ieee80211_tx_status_ni().
*
* @hw: the hardware this frame came in on
* @skb: the buffer to receive, owned by mac80211 after this call
@@ -3196,7 +3199,8 @@ void ieee80211_get_tx_rates(struct ieee80211_vif *vif,
* This function may not be called in IRQ context. Calls to this function
* for a single hardware must be synchronized against each other. Calls
* to this function, ieee80211_tx_status_ni() and ieee80211_tx_status_irqsafe()
- * may not be mixed for a single hardware.
+ * may not be mixed for a single hardware. Must not run concurrently with
+ * ieee80211_rx() or ieee80211_rx_ni().
*
* @hw: the hardware the frame was transmitted by
* @skb: the frame that was transmitted, owned by mac80211 after this call
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 0d51877..923f85b 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -688,8 +688,15 @@ int rate_control_set_rates(struct ieee80211_hw *hw,
struct ieee80211_sta *pubsta,
struct ieee80211_sta_rates *rates)
{
- struct ieee80211_sta_rates *old = rcu_dereference(pubsta->rates);
+ struct ieee80211_sta_rates *old;
+ /*
+ * mac80211 guarantees that this function will not be called
+ * concurrently, so the following RCU access is safe, even outside
+ * of a rcu_read_lock section. This can not be checked easily, so
+ * we just set the condition to true
+ */
+ old = rcu_dereference_protected(pubsta->rates, true);
rcu_assign_pointer(pubsta->rates, rates);
if (old)
kfree_rcu(old, rcu_head);
--
1.8.0.2
On Friday, May 03, 2013 10:01:03 AM Felix Fietkau wrote:
> Document rx vs tx status concurrency requirements.
>
> Signed-off-by: Felix Fietkau <[email protected]>
Thanks. As you know, I was playing with this before. I wrote
a patch, but I didn't have the change to test it yet properly.
If you find anything useful in this (now no longer needed)
take it, if not ignore it :-D. (i.e: the rcu_dereference with
sta->hnext == NULL instead of true).
Regards,
Christian
---
Subject: [RFT] mac80211: more rate control api work
In a discussion of my previous patch [1]:
"mac80211: fix spurious use of rcu_dereference"
Johannes discovered that the band-aid fix proposed in
the mail [adding rcu_read_(un)lock] was inadequate:
>+ rcu_read_lock();
> old = rcu_dereference(sta->rates);
> rcu_assign_pointer(sta->rates, new);
> if (old)
> kfree_rcu(old);
>+ rcu_read_unlock();
"The problem here is that even the rcu_read_lock() around here
that's actually there in most cases *isn't* what's protecting
this code. What's protecting this assignment is the fact that
we require drivers to not call ieee80211_tx_status()
concurrently.
If this wasn't the case, then calling the function could cause
double-free or so by having two CPUs read the old pointer and
both call kfree_rcu() on it." [2]
This patch moves the rcu-protected rates pointer into mac80211's
private station structure. This prevents drivers from modifying
the pointer (in an unsafe way). If a driver wants to access the
rates table, it should use the rate control api function:
ieee80211_get_tx_rates
Furthermore, it also adds a lock in rate_control_set_rates to
avoid double freeing old rates if the function is called
concurrently.
At last, the patch fixes suspicious RCU usage in
rate_control_set_rates during initialization by
adding an exception for stations which are not
yet added to the station hash table.
[1] Message-Id: <[email protected]>
[2] Message-Id: <[email protected]>
---
include/net/mac80211.h | 2 --
net/mac80211/rate.c | 20 ++++++++++++++++----
net/mac80211/sta_info.h | 2 ++
net/mac80211/tx.c | 2 +-
4 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 04c2d46..4457ea2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1276,7 +1276,6 @@ struct ieee80211_sta_rates {
* notifications and capabilities. The value is only valid after
* the station moves to associated state.
* @smps_mode: current SMPS mode (off, static or dynamic)
- * @tx_rates: rate control selection table
*/
struct ieee80211_sta {
u32 supp_rates[IEEE80211_NUM_BANDS];
@@ -1290,7 +1289,6 @@ struct ieee80211_sta {
u8 rx_nss;
enum ieee80211_sta_rx_bandwidth bandwidth;
enum ieee80211_smps_mode smps_mode;
- struct ieee80211_sta_rates __rcu *rates;
/* must be last */
u8 drv_priv[0] __aligned(sizeof(void *));
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index 0d51877..a049ff7 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -530,7 +530,7 @@ static void rate_fixup_ratelist(struct ieee80211_vif *vif,
}
-static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
+static void rate_control_fill_sta_table(struct ieee80211_sta *pubsta,
struct ieee80211_tx_info *info,
struct ieee80211_tx_rate *rates,
int max_rates)
@@ -538,8 +538,11 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta,
struct ieee80211_sta_rates *ratetbl = NULL;
int i;
- if (sta && !info->control.skip_table)
+ if (pubsta && !info->control.skip_table) {
+ struct sta_info *sta = container_of(pubsta, struct sta_info,
+ sta);
ratetbl = rcu_dereference(sta->rates);
+ }
/* Fill remaining rate slots with data from the sta rate table. */
max_rates = min_t(int, max_rates, IEEE80211_TX_RATE_TABLE_SIZE);
@@ -688,9 +691,18 @@ int rate_control_set_rates(struct ieee80211_hw *hw,
struct ieee80211_sta *pubsta,
struct ieee80211_sta_rates *rates)
{
- struct ieee80211_sta_rates *old = rcu_dereference(pubsta->rates);
+ struct sta_info *sta;
+ struct ieee80211_sta_rates *old;
+
+ sta = container_of(pubsta, struct sta_info, sta);
+
+ spin_lock_bh(&sta->lock);
+ old = rcu_dereference_check(sta->rates,
+ rcu_access_pointer(sta->hnext) != NULL);
+
+ rcu_assign_pointer(sta->rates, rates);
+ spin_unlock_bh(&sta->lock);
- rcu_assign_pointer(pubsta->rates, rates);
if (old)
kfree_rcu(old, rcu_head);
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index adc3004..29169f2 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -234,6 +234,7 @@ struct sta_ampdu_mlme {
* @gtk: group keys negotiated with this station, if any
* @rate_ctrl: rate control algorithm reference
* @rate_ctrl_priv: rate control private per-STA pointer
+ * @rates: rate control selection table
* @last_tx_rate: rate used for last transmit, to report to userspace as
* "the" transmit rate
* @last_rx_rate_idx: rx status rate index of the last data packet
@@ -309,6 +310,7 @@ struct sta_info {
struct ieee80211_key __rcu *ptk;
struct rate_control_ref *rate_ctrl;
void *rate_ctrl_priv;
+ struct ieee80211_sta_rates __rcu *rates;
spinlock_t lock;
struct work_struct drv_unblock_wk;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 4a5fbf8..fdf5bc4 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -693,7 +693,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
rate_control_get_rate(tx->sdata, tx->sta, &txrc);
if (tx->sta && !info->control.skip_table)
- ratetbl = rcu_dereference(tx->sta->sta.rates);
+ ratetbl = rcu_dereference(tx->sta->rates);
if (unlikely(info->control.rates[0].idx < 0)) {
if (ratetbl) {
--
1.7.10.4