* [PATCH 0/3] mac80211: fix csa counters
@ 2014-05-22 13:28 Michal Kazior
2014-05-22 13:28 ` [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp Michal Kazior
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Michal Kazior @ 2014-05-22 13:28 UTC (permalink / raw)
To: linux-wireless; +Cc: johannes, Michal Kazior
Hi,
I was seeing strange WARN_ON splat loops and
crashes while I was testing my multi-vif CSA.
CSA counters weren't properly reset and on some
failpaths this caused a mess. There were also
possible SMP inconsitencies/races.
Michal Kazior (3):
mac80211: move csa counters from sdata to beacon/presp
mac80211: use csa counter offsets instead of csa_active
mac80211: make csa_currnet_counter atomic
net/mac80211/cfg.c | 72 +++++++++++++++++++----------
net/mac80211/ibss.c | 2 +-
net/mac80211/ieee80211_i.h | 6 +--
net/mac80211/mesh.c | 2 +-
net/mac80211/tx.c | 112 +++++++++++++++++++++++++--------------------
5 files changed, 114 insertions(+), 80 deletions(-)
--
1.8.5.3
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp 2014-05-22 13:28 [PATCH 0/3] mac80211: fix csa counters Michal Kazior @ 2014-05-22 13:28 ` Michal Kazior 2014-05-22 13:43 ` Johannes Berg 2014-05-22 13:28 ` [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active Michal Kazior 2014-05-22 13:28 ` [PATCH 3/3] mac80211: make csa_currnet_counter atomic Michal Kazior 2 siblings, 1 reply; 14+ messages in thread From: Michal Kazior @ 2014-05-22 13:28 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior Having csa counters part of beacon and probe_resp structures makes it easier to get rid of possible reaces between setting a beacon and updating counters on SMP systems by guaranteeing counters are always consistent against given beacon struct. While at it relax WARN_ON into WARN_ON_ONCE to prevent spamming logs and racing. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- net/mac80211/cfg.c | 68 +++++++++++++++++++----------- net/mac80211/ibss.c | 2 +- net/mac80211/ieee80211_i.h | 6 +-- net/mac80211/mesh.c | 2 +- net/mac80211/tx.c | 102 ++++++++++++++++++++++++--------------------- 5 files changed, 104 insertions(+), 76 deletions(-) diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index ac45304..0518268 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -862,7 +862,9 @@ static int ieee80211_set_monitor_channel(struct wiphy *wiphy, } static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, - const u8 *resp, size_t resp_len) + const u8 *resp, size_t resp_len, + const u16 *csa_counter_offset, + int n_csa_counter_offset) { struct probe_resp *new, *old; @@ -877,6 +879,8 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, new->len = resp_len; memcpy(new->data, resp, resp_len); + memcpy(new->csa_counter_offset, csa_counter_offset, + n_csa_counter_offset * sizeof(*new->csa_counter_offset)); rcu_assign_pointer(sdata->u.ap.probe_resp, new); if (old) @@ -886,7 +890,12 @@ static int ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata, } static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, - struct cfg80211_beacon_data *params) + struct cfg80211_beacon_data *params, + const u16 *csa_counter_offset_beacon, + int n_csa_counter_offset_beacon, + const u16 *csa_counter_offset_presp, + int n_csa_counter_offset_presp, + u8 csa_count) { struct beacon_data *new, *old; int new_head_len, new_tail_len; @@ -929,6 +938,9 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, new->tail = new->head + new_head_len; new->head_len = new_head_len; new->tail_len = new_tail_len; + new->csa_current_counter = csa_count; + memcpy(new->csa_counter_offset, csa_counter_offset_beacon, + n_csa_counter_offset_beacon * sizeof(*new->csa_counter_offset)); /* copy in head */ if (params->head) @@ -944,7 +956,9 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, memcpy(new->tail, old->tail, new_tail_len); err = ieee80211_set_probe_resp(sdata, params->probe_resp, - params->probe_resp_len); + params->probe_resp_len, + csa_counter_offset_presp, + n_csa_counter_offset_presp); if (err < 0) return err; if (err == 0) @@ -1029,7 +1043,8 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, sdata->vif.bss_conf.p2p_noa_attr.oppps_ctwindow |= IEEE80211_P2P_OPPPS_ENABLE_BIT; - err = ieee80211_assign_beacon(sdata, ¶ms->beacon); + err = ieee80211_assign_beacon(sdata, ¶ms->beacon, + NULL, 0, NULL, 0, 0); if (err < 0) { ieee80211_vif_release_channel(sdata); return err; @@ -1077,7 +1092,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, if (!old) return -ENOENT; - err = ieee80211_assign_beacon(sdata, params); + err = ieee80211_assign_beacon(sdata, params, NULL, 0, NULL, 0, 0); if (err < 0) return err; ieee80211_bss_info_change_notify(sdata, err); @@ -3060,7 +3075,8 @@ static int ieee80211_set_after_csa_beacon(struct ieee80211_sub_if_data *sdata, switch (sdata->vif.type) { case NL80211_IFTYPE_AP: - err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon); + err = ieee80211_assign_beacon(sdata, sdata->u.ap.next_beacon, + NULL, 0, NULL, 0, 0); kfree(sdata->u.ap.next_beacon); sdata->u.ap.next_beacon = NULL; @@ -3197,20 +3213,12 @@ static int ieee80211_set_csa_beacon(struct ieee80211_sub_if_data *sdata, IEEE80211_MAX_CSA_COUNTERS_NUM)) return -EINVAL; - /* make sure we don't have garbage in other counters */ - memset(sdata->csa_counter_offset_beacon, 0, - sizeof(sdata->csa_counter_offset_beacon)); - memset(sdata->csa_counter_offset_presp, 0, - sizeof(sdata->csa_counter_offset_presp)); - - memcpy(sdata->csa_counter_offset_beacon, - params->counter_offsets_beacon, - params->n_counter_offsets_beacon * sizeof(u16)); - memcpy(sdata->csa_counter_offset_presp, - params->counter_offsets_presp, - params->n_counter_offsets_presp * sizeof(u16)); - - err = ieee80211_assign_beacon(sdata, ¶ms->beacon_csa); + err = ieee80211_assign_beacon(sdata, ¶ms->beacon_csa, + params->counter_offsets_beacon, + params->n_counter_offsets_beacon, + params->counter_offsets_presp, + params->n_counter_offsets_presp, + params->count); if (err < 0) { kfree(sdata->u.ap.next_beacon); return err; @@ -3354,7 +3362,6 @@ __ieee80211_channel_switch(struct wiphy *wiphy, struct net_device *dev, sdata->csa_radar_required = params->radar_required; sdata->csa_chandef = params->chandef; sdata->csa_block_tx = params->block_tx; - sdata->csa_current_counter = params->count; sdata->vif.csa_active = true; if (sdata->csa_block_tx) @@ -3502,10 +3509,23 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, sdata->vif.type == NL80211_IFTYPE_ADHOC) && params->n_csa_offsets) { int i; - u8 c = sdata->csa_current_counter; + struct beacon_data *beacon = NULL; + + rcu_read_lock(); + + if (sdata->vif.type == NL80211_IFTYPE_AP) + beacon = rcu_dereference(sdata->u.ap.beacon); + else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) + beacon = rcu_dereference(sdata->u.ibss.presp); + else if (ieee80211_vif_is_mesh(&sdata->vif)) + beacon = rcu_dereference(sdata->u.mesh.beacon); - for (i = 0; i < params->n_csa_offsets; i++) - data[params->csa_offsets[i]] = c; + if (beacon) + for (i = 0; i < params->n_csa_offsets; i++) + data[params->csa_offsets[i]] = + beacon->csa_current_counter; + + rcu_read_unlock(); } IEEE80211_SKB_CB(skb)->flags = flags; diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c index 1bbac94..0e5b7c0 100644 --- a/net/mac80211/ibss.c +++ b/net/mac80211/ibss.c @@ -143,7 +143,7 @@ ieee80211_ibss_build_presp(struct ieee80211_sub_if_data *sdata, *pos++ = csa_settings->block_tx ? 1 : 0; *pos++ = ieee80211_frequency_to_channel( csa_settings->chandef.chan->center_freq); - sdata->csa_counter_offset_beacon[0] = (pos - presp->head); + presp->csa_counter_offset[0] = (pos - presp->head); *pos++ = csa_settings->count; } diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index ed2b817..71915a2 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -233,12 +233,15 @@ struct beacon_data { u8 *head, *tail; int head_len, tail_len; struct ieee80211_meshconf_ie *meshconf; + u16 csa_counter_offset[IEEE80211_MAX_CSA_COUNTERS_NUM]; + u8 csa_current_counter; struct rcu_head rcu_head; }; struct probe_resp { struct rcu_head rcu_head; int len; + u16 csa_counter_offset[IEEE80211_MAX_CSA_COUNTERS_NUM]; u8 data[0]; }; @@ -753,8 +756,6 @@ struct ieee80211_sub_if_data { struct mac80211_qos_map __rcu *qos_map; struct work_struct csa_finalize_work; - u16 csa_counter_offset_beacon[IEEE80211_MAX_CSA_COUNTERS_NUM]; - u16 csa_counter_offset_presp[IEEE80211_MAX_CSA_COUNTERS_NUM]; bool csa_radar_required; bool csa_block_tx; /* write-protected by sdata_lock and local->mtx */ struct cfg80211_chan_def csa_chandef; @@ -766,7 +767,6 @@ struct ieee80211_sub_if_data { struct ieee80211_chanctx *reserved_chanctx; struct cfg80211_chan_def reserved_chandef; bool reserved_radar_required; - u8 csa_current_counter; /* used to reconfigure hardware SM PS */ struct work_struct recalc_smps; diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index 6495a3f..a9d4ba6 100644 --- a/net/mac80211/mesh.c +++ b/net/mac80211/mesh.c @@ -679,7 +679,7 @@ ieee80211_mesh_build_beacon(struct ieee80211_if_mesh *ifmsh) *pos++ = 0x0; *pos++ = ieee80211_frequency_to_channel( csa->settings.chandef.chan->center_freq); - sdata->csa_counter_offset_beacon[0] = hdr_len + 6; + bcn->csa_counter_offset[0] = hdr_len + 6; *pos++ = csa->settings.count; *pos++ = WLAN_EID_CHAN_SWITCH_PARAM; *pos++ = 6; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 5214686..7d96a27 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2423,7 +2423,7 @@ static void ieee80211_set_csa(struct ieee80211_sub_if_data *sdata, u8 *beacon_data; size_t beacon_data_len; int i; - u8 count = sdata->csa_current_counter; + u8 count = beacon->csa_current_counter; switch (sdata->vif.type) { case NL80211_IFTYPE_AP: @@ -2442,46 +2442,54 @@ static void ieee80211_set_csa(struct ieee80211_sub_if_data *sdata, return; } + rcu_read_lock(); for (i = 0; i < IEEE80211_MAX_CSA_COUNTERS_NUM; ++i) { - u16 counter_offset_beacon = - sdata->csa_counter_offset_beacon[i]; - u16 counter_offset_presp = sdata->csa_counter_offset_presp[i]; - - if (counter_offset_beacon) { - if (WARN_ON(counter_offset_beacon >= beacon_data_len)) - return; - - beacon_data[counter_offset_beacon] = count; - } - - if (sdata->vif.type == NL80211_IFTYPE_AP && - counter_offset_presp) { - rcu_read_lock(); - resp = rcu_dereference(sdata->u.ap.probe_resp); + resp = rcu_dereference(sdata->u.ap.probe_resp); - /* If nl80211 accepted the offset, this should - * not happen. - */ - if (WARN_ON(!resp)) { + if (beacon->csa_counter_offset[i]) { + if (WARN_ON_ONCE(beacon->csa_counter_offset[i] >= + beacon_data_len)) { rcu_read_unlock(); return; } - resp->data[counter_offset_presp] = count; - rcu_read_unlock(); + + beacon_data[beacon->csa_counter_offset[i]] = count; } + + if (sdata->vif.type == NL80211_IFTYPE_AP && resp && + resp->csa_counter_offset) + resp->data[resp->csa_counter_offset[i]] = count; } + rcu_read_unlock(); } u8 ieee80211_csa_update_counter(struct ieee80211_vif *vif) { struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif); + struct beacon_data *beacon = NULL; + u8 count = 0; - sdata->csa_current_counter--; + rcu_read_lock(); + + if (sdata->vif.type == NL80211_IFTYPE_AP) + beacon = rcu_dereference(sdata->u.ap.beacon); + else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) + beacon = rcu_dereference(sdata->u.ibss.presp); + else if (ieee80211_vif_is_mesh(&sdata->vif)) + beacon = rcu_dereference(sdata->u.mesh.beacon); + + if (!beacon) + goto unlock; + + beacon->csa_current_counter--; /* the counter should never reach 0 */ - WARN_ON(!sdata->csa_current_counter); + WARN_ON_ONCE(!beacon->csa_current_counter); + count = beacon->csa_current_counter; - return sdata->csa_current_counter; +unlock: + rcu_read_unlock(); + return count; } EXPORT_SYMBOL(ieee80211_csa_update_counter); @@ -2491,7 +2499,6 @@ bool ieee80211_csa_is_complete(struct ieee80211_vif *vif) struct beacon_data *beacon = NULL; u8 *beacon_data; size_t beacon_data_len; - int counter_beacon = sdata->csa_counter_offset_beacon[0]; int ret = false; if (!ieee80211_sdata_running(sdata)) @@ -2529,10 +2536,10 @@ bool ieee80211_csa_is_complete(struct ieee80211_vif *vif) goto out; } - if (WARN_ON(counter_beacon > beacon_data_len)) + if (WARN_ON_ONCE(beacon->csa_counter_offset[0] > beacon_data_len)) goto out; - if (beacon_data[counter_beacon] == 1) + if (beacon_data[beacon->csa_counter_offset[0]] == 1) ret = true; out: rcu_read_unlock(); @@ -2548,6 +2555,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, bool is_template) { struct ieee80211_local *local = hw_to_local(hw); + struct beacon_data *beacon = NULL; struct sk_buff *skb = NULL; struct ieee80211_tx_info *info; struct ieee80211_sub_if_data *sdata = NULL; @@ -2569,8 +2577,8 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, if (sdata->vif.type == NL80211_IFTYPE_AP) { struct ieee80211_if_ap *ap = &sdata->u.ap; - struct beacon_data *beacon = rcu_dereference(ap->beacon); + beacon = rcu_dereference(ap->beacon); if (beacon) { if (sdata->vif.csa_active) { if (!is_template) @@ -2613,34 +2621,34 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, } else if (sdata->vif.type == NL80211_IFTYPE_ADHOC) { struct ieee80211_if_ibss *ifibss = &sdata->u.ibss; struct ieee80211_hdr *hdr; - struct beacon_data *presp = rcu_dereference(ifibss->presp); - if (!presp) + beacon = rcu_dereference(ifibss->presp); + if (!beacon) goto out; if (sdata->vif.csa_active) { if (!is_template) ieee80211_csa_update_counter(vif); - ieee80211_set_csa(sdata, presp); + ieee80211_set_csa(sdata, beacon); } - skb = dev_alloc_skb(local->tx_headroom + presp->head_len + + skb = dev_alloc_skb(local->tx_headroom + beacon->head_len + local->hw.extra_beacon_tailroom); if (!skb) goto out; skb_reserve(skb, local->tx_headroom); - memcpy(skb_put(skb, presp->head_len), presp->head, - presp->head_len); + memcpy(skb_put(skb, beacon->head_len), beacon->head, + beacon->head_len); hdr = (struct ieee80211_hdr *) skb->data; hdr->frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT | IEEE80211_STYPE_BEACON); } else if (ieee80211_vif_is_mesh(&sdata->vif)) { struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh; - struct beacon_data *bcn = rcu_dereference(ifmsh->beacon); - if (!bcn) + beacon = rcu_dereference(ifmsh->beacon); + if (!beacon) goto out; if (sdata->vif.csa_active) { @@ -2652,40 +2660,40 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, */ ieee80211_csa_update_counter(vif); - ieee80211_set_csa(sdata, bcn); + ieee80211_set_csa(sdata, beacon); } if (ifmsh->sync_ops) - ifmsh->sync_ops->adjust_tbtt(sdata, bcn); + ifmsh->sync_ops->adjust_tbtt(sdata, beacon); skb = dev_alloc_skb(local->tx_headroom + - bcn->head_len + + beacon->head_len + 256 + /* TIM IE */ - bcn->tail_len + + beacon->tail_len + local->hw.extra_beacon_tailroom); if (!skb) goto out; skb_reserve(skb, local->tx_headroom); - memcpy(skb_put(skb, bcn->head_len), bcn->head, bcn->head_len); + memcpy(skb_put(skb, beacon->head_len), beacon->head, beacon->head_len); ieee80211_beacon_add_tim(sdata, &ifmsh->ps, skb, is_template); if (offs) { - offs->tim_offset = bcn->head_len; - offs->tim_length = skb->len - bcn->head_len; + offs->tim_offset = beacon->head_len; + offs->tim_length = skb->len - beacon->head_len; } - memcpy(skb_put(skb, bcn->tail_len), bcn->tail, bcn->tail_len); + memcpy(skb_put(skb, beacon->tail_len), beacon->tail, beacon->tail_len); } else { WARN_ON(1); goto out; } /* CSA offsets */ - if (offs) { + if (offs && beacon) { int i; for (i = 0; i < IEEE80211_MAX_CSA_COUNTERS_NUM; i++) { - u16 csa_off = sdata->csa_counter_offset_beacon[i]; + u16 csa_off = beacon->csa_counter_offset[i]; if (!csa_off) continue; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp 2014-05-22 13:28 ` [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp Michal Kazior @ 2014-05-22 13:43 ` Johannes Berg 2014-05-22 13:53 ` Michal Kazior 0 siblings, 1 reply; 14+ messages in thread From: Johannes Berg @ 2014-05-22 13:43 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: > Having csa counters part of beacon and probe_resp > structures makes it easier to get rid of possible > reaces between setting a beacon and updating > counters on SMP systems by guaranteeing counters > are always consistent against given beacon struct. This makes a lot of sense. > static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, > - struct cfg80211_beacon_data *params) > + struct cfg80211_beacon_data *params, > + const u16 *csa_counter_offset_beacon, > + int n_csa_counter_offset_beacon, > + const u16 *csa_counter_offset_presp, > + int n_csa_counter_offset_presp, > + u8 csa_count) But that seems overkill. Maybe those CSA-related arguments could be in some new struct so you don't have to pass "..., NULL, 0, NULL, 0, 0"? johannes ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp 2014-05-22 13:43 ` Johannes Berg @ 2014-05-22 13:53 ` Michal Kazior 2014-05-22 14:37 ` Johannes Berg 0 siblings, 1 reply; 14+ messages in thread From: Michal Kazior @ 2014-05-22 13:53 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On 22 May 2014 15:43, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: >> Having csa counters part of beacon and probe_resp >> structures makes it easier to get rid of possible >> reaces between setting a beacon and updating >> reaces -_- grr >> counters on SMP systems by guaranteeing counters >> are always consistent against given beacon struct. > > This makes a lot of sense. > >> static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, >> - struct cfg80211_beacon_data *params) >> + struct cfg80211_beacon_data *params, >> + const u16 *csa_counter_offset_beacon, >> + int n_csa_counter_offset_beacon, >> + const u16 *csa_counter_offset_presp, >> + int n_csa_counter_offset_presp, >> + u8 csa_count) > > But that seems overkill. Maybe those CSA-related arguments could be in > some new struct so you don't have to pass "..., NULL, 0, NULL, 0, 0"? I didn't want to invent a structure just to pass a bunch of arguments. Hmm.. maybe we can move counter offsets into cfg80211_beacon_data? This would apply to cfg80211_csa_settings as well. Michał ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp 2014-05-22 13:53 ` Michal Kazior @ 2014-05-22 14:37 ` Johannes Berg 2014-05-22 14:38 ` Johannes Berg 0 siblings, 1 reply; 14+ messages in thread From: Johannes Berg @ 2014-05-22 14:37 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2014-05-22 at 15:53 +0200, Michal Kazior wrote: > >> static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, > >> - struct cfg80211_beacon_data *params) > >> + struct cfg80211_beacon_data *params, > >> + const u16 *csa_counter_offset_beacon, > >> + int n_csa_counter_offset_beacon, > >> + const u16 *csa_counter_offset_presp, > >> + int n_csa_counter_offset_presp, > >> + u8 csa_count) > > > > But that seems overkill. Maybe those CSA-related arguments could be in > > some new struct so you don't have to pass "..., NULL, 0, NULL, 0, 0"? > > I didn't want to invent a structure just to pass a bunch of arguments. I don't see any problem with that? > Hmm.. maybe we can move counter offsets into cfg80211_beacon_data? > This would apply to cfg80211_csa_settings as well. I don't think that's a good idea, since the cfg80211_beacon_data is used in places where that wouldn't make sense, that would confuse the API. You could use "struct cfg80211_csa_settings" here I suppose, but I think a new struct is perfectly reasonable. johannes ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp 2014-05-22 14:37 ` Johannes Berg @ 2014-05-22 14:38 ` Johannes Berg 0 siblings, 0 replies; 14+ messages in thread From: Johannes Berg @ 2014-05-22 14:38 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2014-05-22 at 16:37 +0200, Johannes Berg wrote: > You could use "struct cfg80211_csa_settings" here I suppose, but I think > a new struct is perfectly reasonable. Actually I don't think that's a terribly good idea - there are a lot of temporary pointers so you can't copy the struct, and it has a lot of unnecessary baggage too. johannes ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active 2014-05-22 13:28 [PATCH 0/3] mac80211: fix csa counters Michal Kazior 2014-05-22 13:28 ` [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp Michal Kazior @ 2014-05-22 13:28 ` Michal Kazior 2014-05-22 13:45 ` Johannes Berg 2014-05-22 13:28 ` [PATCH 3/3] mac80211: make csa_currnet_counter atomic Michal Kazior 2 siblings, 1 reply; 14+ messages in thread From: Michal Kazior @ 2014-05-22 13:28 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior vif->csa_active is protected by mutexes only. This means it is unreliable to depend on it on codeflow in non-sleepable beacon and CSA code. There was no guarantee to have vif->csa_active update be visible before beacons are updated on SMP systems. Using csa counter offsets which are embedded in beacon struct (and thus are protected with single RCU assignment) is much safer. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- net/mac80211/tx.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 7d96a27..eeeafeb 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2536,6 +2536,9 @@ bool ieee80211_csa_is_complete(struct ieee80211_vif *vif) goto out; } + if (!beacon->csa_counter_offset[0]) + goto out; + if (WARN_ON_ONCE(beacon->csa_counter_offset[0] > beacon_data_len)) goto out; @@ -2580,7 +2583,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, beacon = rcu_dereference(ap->beacon); if (beacon) { - if (sdata->vif.csa_active) { + if (beacon->csa_counter_offset[0]) { if (!is_template) ieee80211_csa_update_counter(vif); @@ -2626,7 +2629,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, if (!beacon) goto out; - if (sdata->vif.csa_active) { + if (beacon->csa_counter_offset[0]) { if (!is_template) ieee80211_csa_update_counter(vif); @@ -2651,7 +2654,7 @@ __ieee80211_beacon_get(struct ieee80211_hw *hw, if (!beacon) goto out; - if (sdata->vif.csa_active) { + if (beacon->csa_counter_offset[0]) { if (!is_template) /* TODO: For mesh csa_counter is in TU, so * decrementing it by one isn't correct, but -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active 2014-05-22 13:28 ` [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active Michal Kazior @ 2014-05-22 13:45 ` Johannes Berg 2014-05-22 13:59 ` Michal Kazior 0 siblings, 1 reply; 14+ messages in thread From: Johannes Berg @ 2014-05-22 13:45 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: > vif->csa_active is protected by mutexes only. This > means it is unreliable to depend on it on codeflow > in non-sleepable beacon and CSA code. There was no > guarantee to have vif->csa_active update be > visible before beacons are updated on SMP systems. > > Using csa counter offsets which are embedded in > beacon struct (and thus are protected with single > RCU assignment) is much safer. This seems reasonable, but many uses of csa_active remain, no? hwsim for example seems to access it without locking as well. johannes ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active 2014-05-22 13:45 ` Johannes Berg @ 2014-05-22 13:59 ` Michal Kazior 2014-05-22 14:38 ` Johannes Berg 0 siblings, 1 reply; 14+ messages in thread From: Michal Kazior @ 2014-05-22 13:59 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On 22 May 2014 15:45, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: >> vif->csa_active is protected by mutexes only. This >> means it is unreliable to depend on it on codeflow >> in non-sleepable beacon and CSA code. There was no >> guarantee to have vif->csa_active update be >> visible before beacons are updated on SMP systems. >> >> Using csa counter offsets which are embedded in >> beacon struct (and thus are protected with single >> RCU assignment) is much safer. > > This seems reasonable, but many uses of csa_active remain, no? hwsim for > example seems to access it without locking as well. Since the problem could be split I went with fixing things step-by-step. I was trying to tackle csa_active issue as well but I'm yet to arrive at a satisfactory solution. It's probably best to kill it and introduce a drv_switch_beacon_done(). With my patchset csa_active isn't as critical for decision making in the code. Michał ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active 2014-05-22 13:59 ` Michal Kazior @ 2014-05-22 14:38 ` Johannes Berg 0 siblings, 0 replies; 14+ messages in thread From: Johannes Berg @ 2014-05-22 14:38 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2014-05-22 at 15:59 +0200, Michal Kazior wrote: > On 22 May 2014 15:45, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: > >> vif->csa_active is protected by mutexes only. This > >> means it is unreliable to depend on it on codeflow > >> in non-sleepable beacon and CSA code. There was no > >> guarantee to have vif->csa_active update be > >> visible before beacons are updated on SMP systems. > >> > >> Using csa counter offsets which are embedded in > >> beacon struct (and thus are protected with single > >> RCU assignment) is much safer. > > > > This seems reasonable, but many uses of csa_active remain, no? hwsim for > > example seems to access it without locking as well. > > Since the problem could be split I went with fixing things step-by-step. > > I was trying to tackle csa_active issue as well but I'm yet to arrive > at a satisfactory solution. It's probably best to kill it and > introduce a drv_switch_beacon_done(). > > With my patchset csa_active isn't as critical for decision making in the code. Ok, thanks for the explanation. johannes ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] mac80211: make csa_currnet_counter atomic 2014-05-22 13:28 [PATCH 0/3] mac80211: fix csa counters Michal Kazior 2014-05-22 13:28 ` [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp Michal Kazior 2014-05-22 13:28 ` [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active Michal Kazior @ 2014-05-22 13:28 ` Michal Kazior 2014-05-22 13:50 ` Johannes Berg 2 siblings, 1 reply; 14+ messages in thread From: Michal Kazior @ 2014-05-22 13:28 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior Even it ieee80211_beacon_get() and ieee80211_csa_update_counter() were to be guaranteed to be serialized by drivers it still couldn't be guaranteed to be safe on SMP systems. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- net/mac80211/cfg.c | 12 +++++++----- net/mac80211/ieee80211_i.h | 2 +- net/mac80211/tx.c | 11 ++++++----- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 0518268..aab258b 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -938,7 +938,7 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata, new->tail = new->head + new_head_len; new->head_len = new_head_len; new->tail_len = new_tail_len; - new->csa_current_counter = csa_count; + atomic_set(&new->csa_current_counter, csa_count); memcpy(new->csa_counter_offset, csa_counter_offset_beacon, n_csa_counter_offset_beacon * sizeof(*new->csa_counter_offset)); @@ -3508,7 +3508,6 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, (sdata->vif.type == NL80211_IFTYPE_AP || sdata->vif.type == NL80211_IFTYPE_ADHOC) && params->n_csa_offsets) { - int i; struct beacon_data *beacon = NULL; rcu_read_lock(); @@ -3520,10 +3519,13 @@ static int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, else if (ieee80211_vif_is_mesh(&sdata->vif)) beacon = rcu_dereference(sdata->u.mesh.beacon); - if (beacon) + if (beacon) { + u8 count = atomic_read(&beacon->csa_current_counter); + int i; + for (i = 0; i < params->n_csa_offsets; i++) - data[params->csa_offsets[i]] = - beacon->csa_current_counter; + data[params->csa_offsets[i]] = count; + } rcu_read_unlock(); } diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 71915a2..69da2d6 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -234,7 +234,7 @@ struct beacon_data { int head_len, tail_len; struct ieee80211_meshconf_ie *meshconf; u16 csa_counter_offset[IEEE80211_MAX_CSA_COUNTERS_NUM]; - u8 csa_current_counter; + atomic_t csa_current_counter; struct rcu_head rcu_head; }; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index eeeafeb..af59877 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2423,7 +2423,7 @@ static void ieee80211_set_csa(struct ieee80211_sub_if_data *sdata, u8 *beacon_data; size_t beacon_data_len; int i; - u8 count = beacon->csa_current_counter; + u8 count = atomic_read(&beacon->csa_current_counter); switch (sdata->vif.type) { case NL80211_IFTYPE_AP: @@ -2481,11 +2481,12 @@ u8 ieee80211_csa_update_counter(struct ieee80211_vif *vif) if (!beacon) goto unlock; - beacon->csa_current_counter--; - + count = atomic_dec_return(&beacon->csa_current_counter); /* the counter should never reach 0 */ - WARN_ON_ONCE(!beacon->csa_current_counter); - count = beacon->csa_current_counter; + if (WARN_ON_ONCE(count <= 0)) { + /* prevent it from going negative */ + atomic_inc(&beacon->csa_current_counter); + } unlock: rcu_read_unlock(); -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] mac80211: make csa_currnet_counter atomic 2014-05-22 13:28 ` [PATCH 3/3] mac80211: make csa_currnet_counter atomic Michal Kazior @ 2014-05-22 13:50 ` Johannes Berg 2014-05-22 14:03 ` Michal Kazior 0 siblings, 1 reply; 14+ messages in thread From: Johannes Berg @ 2014-05-22 13:50 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless typo in subject. On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: > Even it ieee80211_beacon_get() and > ieee80211_csa_update_counter() were to be > guaranteed to be serialized by drivers it still > couldn't be guaranteed to be safe on SMP systems. We had this debate internally as well, but I don't see where issues should be. The only place updating the counter is ieee80211_csa_update_counter(), and that should only be called either by the driver, or by ieee80211_beacon_get[_tim]() [*]. If the driver wants to have the counter update by ieee80211_beacon_get() then it must call the function once every beacon interval, if it uses the _template() version then it must call ieee80211_csa_update_counter() every beacon interval, so no races seem possible. johannes [*] the driver shouldn't call ieee80211_beacon_get[_tim] and ieee80211_csa_update_counter() in a mixed fashion anyway as that'd lead to CSA counter bugs, and ieee80211_beacon_get_template() doesn't change the counter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] mac80211: make csa_currnet_counter atomic 2014-05-22 13:50 ` Johannes Berg @ 2014-05-22 14:03 ` Michal Kazior 2014-05-22 14:36 ` Johannes Berg 0 siblings, 1 reply; 14+ messages in thread From: Michal Kazior @ 2014-05-22 14:03 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On 22 May 2014 15:50, Johannes Berg <johannes@sipsolutions.net> wrote: > typo in subject. > > On Thu, 2014-05-22 at 15:28 +0200, Michal Kazior wrote: >> Even it ieee80211_beacon_get() and >> ieee80211_csa_update_counter() were to be >> guaranteed to be serialized by drivers it still >> couldn't be guaranteed to be safe on SMP systems. > > We had this debate internally as well, but I don't see where issues > should be. The only place updating the counter is > ieee80211_csa_update_counter(), and that should only be called either by > the driver, or by ieee80211_beacon_get[_tim]() [*]. If the driver wants > to have the counter update by ieee80211_beacon_get() then it must call > the function once every beacon interval, if it uses the _template() > version then it must call ieee80211_csa_update_counter() every beacon > interval, so no races seem possible. > > johannes > > [*] the driver shouldn't call ieee80211_beacon_get[_tim] and > ieee80211_csa_update_counter() in a mixed fashion anyway as that'd lead > to CSA counter bugs, and ieee80211_beacon_get_template() doesn't change > the counter I was thinking the same. The problem is without an atomic variable the code is simply oblivious to SMP. What if your beacon interrupts are spread across different CPU and your caches haven't synced yet? I suppose this is practically unlikely considering beacon intervals but still.. Michał ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] mac80211: make csa_currnet_counter atomic 2014-05-22 14:03 ` Michal Kazior @ 2014-05-22 14:36 ` Johannes Berg 0 siblings, 0 replies; 14+ messages in thread From: Johannes Berg @ 2014-05-22 14:36 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2014-05-22 at 16:03 +0200, Michal Kazior wrote: > I was thinking the same. The problem is without an atomic variable the > code is simply oblivious to SMP. What if your beacon interrupts are > spread across different CPU and your caches haven't synced yet? I > suppose this is practically unlikely considering beacon intervals but > still.. No, this can't happen - Linux only runs on cache-coherent SMP architectures. The only race that could happen is the "lost update" etc. but that's obviously avoided by the driver synchronising/spacing the updates. johannes ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-05-22 14:39 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-22 13:28 [PATCH 0/3] mac80211: fix csa counters Michal Kazior 2014-05-22 13:28 ` [PATCH 1/3] mac80211: move csa counters from sdata to beacon/presp Michal Kazior 2014-05-22 13:43 ` Johannes Berg 2014-05-22 13:53 ` Michal Kazior 2014-05-22 14:37 ` Johannes Berg 2014-05-22 14:38 ` Johannes Berg 2014-05-22 13:28 ` [PATCH 2/3] mac80211: use csa counter offsets instead of csa_active Michal Kazior 2014-05-22 13:45 ` Johannes Berg 2014-05-22 13:59 ` Michal Kazior 2014-05-22 14:38 ` Johannes Berg 2014-05-22 13:28 ` [PATCH 3/3] mac80211: make csa_currnet_counter atomic Michal Kazior 2014-05-22 13:50 ` Johannes Berg 2014-05-22 14:03 ` Michal Kazior 2014-05-22 14:36 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).