* [PATCH] mac80211: fix AP_VLAN crypto tailroom calculation @ 2015-05-11 13:05 Michal Kazior 2015-05-11 13:11 ` Johannes Berg 2015-05-13 9:16 ` [PATCH v2 1/2] " Michal Kazior 0 siblings, 2 replies; 12+ messages in thread From: Michal Kazior @ 2015-05-11 13:05 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior AP_VLANs may inherit crypto keys from parent AP. Moreover AP_VLANs may have PTK keys of their own. Hence both AP_VLAN sdata and AP sdata must be inspected. Some splats I was seeing: (a) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wep.c:102 ieee80211_wep_add_iv (b) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:73 ieee80211_tx_h_michael_mic_add (c) WARNING: CPU: 3 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:433 ieee80211_crypto_ccmp_encrypt I've seen (a) and (b) with ath9k hw crypto and (c) with ath9k sw crypto. All of them were related to insufficient skb tailroom and I was able to trigger these with ping6 program. This patch effectively fixes Tx when using AP_VLANs with WEP and WPA in some setups. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- net/mac80211/tx.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 8df134213adf..0887d6e5c424 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -1593,6 +1593,25 @@ static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata, /* device xmit handlers */ +static bool +ieee80211_need_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_sub_if_data *parent_sdata; + + if (sdata->crypto_tx_tailroom_needed_cnt) + return true; + + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN && sdata->bss) { + parent_sdata = container_of(sdata->bss, + struct ieee80211_sub_if_data, + u.ap); + if (parent_sdata->crypto_tx_tailroom_needed_cnt) + return true; + } + + return false; +} + static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb, int head_need, bool may_encrypt) @@ -1600,7 +1619,7 @@ static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata, struct ieee80211_local *local = sdata->local; int tail_need = 0; - if (may_encrypt && sdata->crypto_tx_tailroom_needed_cnt) { + if (may_encrypt && ieee80211_need_crypto_tx_tailroom(sdata)) { tail_need = IEEE80211_ENCRYPT_TAILROOM; tail_need -= skb_tailroom(skb); tail_need = max_t(int, tail_need, 0); -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] mac80211: fix AP_VLAN crypto tailroom calculation 2015-05-11 13:05 [PATCH] mac80211: fix AP_VLAN crypto tailroom calculation Michal Kazior @ 2015-05-11 13:11 ` Johannes Berg 2015-05-11 13:33 ` Michal Kazior 2015-05-13 9:16 ` [PATCH v2 1/2] " Michal Kazior 1 sibling, 1 reply; 12+ messages in thread From: Johannes Berg @ 2015-05-11 13:11 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Mon, 2015-05-11 at 13:05 +0000, Michal Kazior wrote: > AP_VLANs may inherit crypto keys from parent AP. > Moreover AP_VLANs may have PTK keys of their own. > Hence both AP_VLAN sdata and AP sdata must be > inspected. > > Some splats I was seeing: > > (a) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wep.c:102 ieee80211_wep_add_iv > (b) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:73 ieee80211_tx_h_michael_mic_add > (c) WARNING: CPU: 3 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:433 ieee80211_crypto_ccmp_encrypt > > I've seen (a) and (b) with ath9k hw crypto and (c) > with ath9k sw crypto. All of them were related to > insufficient skb tailroom and I was able to > trigger these with ping6 program. > > This patch effectively fixes Tx when using > AP_VLANs with WEP and WPA in some setups. > > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > --- > net/mac80211/tx.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c > index 8df134213adf..0887d6e5c424 100644 > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -1593,6 +1593,25 @@ static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata, > > /* device xmit handlers */ > > +static bool > +ieee80211_need_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) > +{ > + struct ieee80211_sub_if_data *parent_sdata; > + > + if (sdata->crypto_tx_tailroom_needed_cnt) > + return true; > + > + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN && sdata->bss) { > + parent_sdata = container_of(sdata->bss, > + struct ieee80211_sub_if_data, > + u.ap); > + if (parent_sdata->crypto_tx_tailroom_needed_cnt) > + return true; > + } > + > + return false; > +} > + > static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata, > struct sk_buff *skb, > int head_need, bool may_encrypt) > @@ -1600,7 +1619,7 @@ static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata, > struct ieee80211_local *local = sdata->local; > int tail_need = 0; > > - if (may_encrypt && sdata->crypto_tx_tailroom_needed_cnt) { > + if (may_encrypt && ieee80211_need_crypto_tx_tailroom(sdata)) { This makes that check far more inefficient - I think you should write it differently and have the management code copy the value to the VLAN interfaces so the existing check here is sufficient. johannes ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mac80211: fix AP_VLAN crypto tailroom calculation 2015-05-11 13:11 ` Johannes Berg @ 2015-05-11 13:33 ` Michal Kazior 0 siblings, 0 replies; 12+ messages in thread From: Michal Kazior @ 2015-05-11 13:33 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On 11 May 2015 at 15:11, Johannes Berg <johannes@sipsolutions.net> wrote: > On Mon, 2015-05-11 at 13:05 +0000, Michal Kazior wrote: [...] >> +static bool >> +ieee80211_need_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) >> +{ >> + struct ieee80211_sub_if_data *parent_sdata; >> + >> + if (sdata->crypto_tx_tailroom_needed_cnt) >> + return true; >> + >> + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN && sdata->bss) { >> + parent_sdata = container_of(sdata->bss, >> + struct ieee80211_sub_if_data, >> + u.ap); >> + if (parent_sdata->crypto_tx_tailroom_needed_cnt) >> + return true; >> + } >> + >> + return false; >> +} >> + >> static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata, >> struct sk_buff *skb, >> int head_need, bool may_encrypt) >> @@ -1600,7 +1619,7 @@ static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata, >> struct ieee80211_local *local = sdata->local; >> int tail_need = 0; >> >> - if (may_encrypt && sdata->crypto_tx_tailroom_needed_cnt) { >> + if (may_encrypt && ieee80211_need_crypto_tx_tailroom(sdata)) { > > This makes that check far more inefficient - I think you should write it > differently and have the management code copy the value to the VLAN > interfaces so the existing check here is sufficient. I didn't want to pre-optimize but you're probably right. I'll look into it more. Thanks! Michał ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] mac80211: fix AP_VLAN crypto tailroom calculation 2015-05-11 13:05 [PATCH] mac80211: fix AP_VLAN crypto tailroom calculation Michal Kazior 2015-05-11 13:11 ` Johannes Berg @ 2015-05-13 9:16 ` Michal Kazior 2015-05-13 9:16 ` [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption Michal Kazior 2015-05-20 13:12 ` [PATCH v2 1/2] mac80211: fix AP_VLAN crypto tailroom calculation Johannes Berg 1 sibling, 2 replies; 12+ messages in thread From: Michal Kazior @ 2015-05-13 9:16 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior Some splats I was seeing: (a) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wep.c:102 ieee80211_wep_add_iv (b) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:73 ieee80211_tx_h_michael_mic_add (c) WARNING: CPU: 3 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:433 ieee80211_crypto_ccmp_encrypt I've seen (a) and (b) with ath9k hw crypto and (c) with ath9k sw crypto. All of them were related to insufficient skb tailroom and I was able to trigger these with ping6 program. AP_VLANs may inherit crypto keys from parent AP. This wasn't considered and yielded problems in some setups resulting in inability to transmit data because mac80211 wouldn't resize skbs when necessary and subsequently drop some packets due to insufficient tailroom. For efficiency purposes don't inspect both AP_VLAN and AP sdata looking for tailroom counter. Instead update AP_VLAN tailroom counters whenever their master AP tailroom counter changes. Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- Notes: v2: * update AP_VLAN tailroom counters on AP tailroom changes instead of checking both AP_VLAN and AP sdatas on Tx path for efficiency purposes [Johannes] * refactor commit log a bit net/mac80211/iface.c | 6 ++++ net/mac80211/key.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++------ net/mac80211/key.h | 1 + net/mac80211/util.c | 3 ++ 4 files changed, 83 insertions(+), 9 deletions(-) diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c index dc2d7133c4f6..3f58d37ea057 100644 --- a/net/mac80211/iface.c +++ b/net/mac80211/iface.c @@ -522,6 +522,12 @@ int ieee80211_do_open(struct wireless_dev *wdev, bool coming_up) memcpy(sdata->vif.hw_queue, master->vif.hw_queue, sizeof(sdata->vif.hw_queue)); sdata->vif.bss_conf.chandef = master->vif.bss_conf.chandef; + + mutex_lock(&local->key_mtx); + sdata->crypto_tx_tailroom_needed_cnt += + master->crypto_tx_tailroom_needed_cnt; + mutex_unlock(&local->key_mtx); + break; } case NL80211_IFTYPE_AP: diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 2e677376c958..577a11a13cdf 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -58,6 +58,22 @@ static void assert_key_lock(struct ieee80211_local *local) lockdep_assert_held(&local->key_mtx); } +static void +update_vlan_tailroom_need_count(struct ieee80211_sub_if_data *sdata, int delta) +{ + struct ieee80211_sub_if_data *vlan; + + if (sdata->vif.type != NL80211_IFTYPE_AP) + return; + + mutex_lock(&sdata->local->mtx); + + list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) + vlan->crypto_tx_tailroom_needed_cnt += delta; + + mutex_unlock(&sdata->local->mtx); +} + static void increment_tailroom_need_count(struct ieee80211_sub_if_data *sdata) { /* @@ -79,6 +95,8 @@ static void increment_tailroom_need_count(struct ieee80211_sub_if_data *sdata) * http://mid.gmane.org/1308590980.4322.19.camel@jlt3.sipsolutions.net */ + update_vlan_tailroom_need_count(sdata, 1); + if (!sdata->crypto_tx_tailroom_needed_cnt++) { /* * Flush all XMIT packets currently using HW encryption or no @@ -88,6 +106,15 @@ static void increment_tailroom_need_count(struct ieee80211_sub_if_data *sdata) } } +static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata, + int delta) +{ + WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt < delta); + + update_vlan_tailroom_need_count(sdata, -delta); + sdata->crypto_tx_tailroom_needed_cnt -= delta; +} + static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) { struct ieee80211_sub_if_data *sdata; @@ -144,7 +171,7 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) || (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM))) - sdata->crypto_tx_tailroom_needed_cnt--; + decrease_tailroom_need_count(sdata, 1); WARN_ON((key->conf.flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE) && (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)); @@ -545,7 +572,7 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key, schedule_delayed_work(&sdata->dec_tailroom_needed_wk, HZ/2); } else { - sdata->crypto_tx_tailroom_needed_cnt--; + decrease_tailroom_need_count(sdata, 1); } } @@ -635,6 +662,7 @@ void ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom) void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) { struct ieee80211_key *key; + struct ieee80211_sub_if_data *vlan; ASSERT_RTNL(); @@ -643,7 +671,14 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) mutex_lock(&sdata->local->key_mtx); - sdata->crypto_tx_tailroom_needed_cnt = 0; + WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt || + sdata->crypto_tx_tailroom_pending_dec); + + if (sdata->vif.type == NL80211_IFTYPE_AP) { + list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) + WARN_ON_ONCE(vlan->crypto_tx_tailroom_needed_cnt || + vlan->crypto_tx_tailroom_pending_dec); + } list_for_each_entry(key, &sdata->key_list, list) { increment_tailroom_need_count(sdata); @@ -653,6 +688,22 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) mutex_unlock(&sdata->local->key_mtx); } +void ieee80211_reset_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_sub_if_data *vlan; + + mutex_lock(&sdata->local->key_mtx); + + sdata->crypto_tx_tailroom_needed_cnt = 0; + + if (sdata->vif.type == NL80211_IFTYPE_AP) { + list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) + vlan->crypto_tx_tailroom_needed_cnt = 0; + } + + mutex_unlock(&sdata->local->key_mtx); +} + void ieee80211_iter_keys(struct ieee80211_hw *hw, struct ieee80211_vif *vif, void (*iter)(struct ieee80211_hw *hw, @@ -692,8 +743,8 @@ static void ieee80211_free_keys_iface(struct ieee80211_sub_if_data *sdata, { struct ieee80211_key *key, *tmp; - sdata->crypto_tx_tailroom_needed_cnt -= - sdata->crypto_tx_tailroom_pending_dec; + decrease_tailroom_need_count(sdata, + sdata->crypto_tx_tailroom_pending_dec); sdata->crypto_tx_tailroom_pending_dec = 0; ieee80211_debugfs_key_remove_mgmt_default(sdata); @@ -713,6 +764,7 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata, { struct ieee80211_local *local = sdata->local; struct ieee80211_sub_if_data *vlan; + struct ieee80211_sub_if_data *master; struct ieee80211_key *key, *tmp; LIST_HEAD(keys); @@ -732,8 +784,20 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata, list_for_each_entry_safe(key, tmp, &keys, list) __ieee80211_key_destroy(key, false); - WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt || - sdata->crypto_tx_tailroom_pending_dec); + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN) { + if (sdata->bss) { + master = container_of(sdata->bss, + struct ieee80211_sub_if_data, + u.ap); + + WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt != + master->crypto_tx_tailroom_needed_cnt); + } + } else { + WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt || + sdata->crypto_tx_tailroom_pending_dec); + } + if (sdata->vif.type == NL80211_IFTYPE_AP) { list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) WARN_ON_ONCE(vlan->crypto_tx_tailroom_needed_cnt || @@ -797,8 +861,8 @@ void ieee80211_delayed_tailroom_dec(struct work_struct *wk) */ mutex_lock(&sdata->local->key_mtx); - sdata->crypto_tx_tailroom_needed_cnt -= - sdata->crypto_tx_tailroom_pending_dec; + decrease_tailroom_need_count(sdata, + sdata->crypto_tx_tailroom_pending_dec); sdata->crypto_tx_tailroom_pending_dec = 0; mutex_unlock(&sdata->local->key_mtx); } diff --git a/net/mac80211/key.h b/net/mac80211/key.h index df430a618764..2119526db2f4 100644 --- a/net/mac80211/key.h +++ b/net/mac80211/key.h @@ -160,6 +160,7 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata, void ieee80211_free_sta_keys(struct ieee80211_local *local, struct sta_info *sta); void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata); +void ieee80211_reset_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata); #define key_mtx_dereference(local, ref) \ rcu_dereference_protected(ref, lockdep_is_held(&((local)->key_mtx))) diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 79412f16b61d..b864ebc6ab8f 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -2023,6 +2023,9 @@ int ieee80211_reconfig(struct ieee80211_local *local) /* add back keys */ list_for_each_entry(sdata, &local->interfaces, list) + ieee80211_reset_crypto_tx_tailroom(sdata); + + list_for_each_entry(sdata, &local->interfaces, list) if (ieee80211_sdata_running(sdata)) ieee80211_enable_keys(sdata); -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption 2015-05-13 9:16 ` [PATCH v2 1/2] " Michal Kazior @ 2015-05-13 9:16 ` Michal Kazior 2015-05-20 13:14 ` Johannes Berg 2015-05-22 8:22 ` [PATCH v3] " Michal Kazior 2015-05-20 13:12 ` [PATCH v2 1/2] mac80211: fix AP_VLAN crypto tailroom calculation Johannes Berg 1 sibling, 2 replies; 12+ messages in thread From: Michal Kazior @ 2015-05-13 9:16 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior There was a possible race between ieee80211_reconfig() and ieee80211_delayed_tailroom_dec(). This could result in inability to transmit data if driver crashed during roaming or rekeying and subsequent skbs with insufficient tailroom appeared. This race was probably never seen in the wild because a device driver would have to crash AND recover within 0.5s which is very unlikely. I was able to prove this race exists after changing the delay to 10s locally and crashing ath10k via debugfs immediately after GTK rekeying. In case of ath10k the counter went below 0. This was harmless but other drivers which actually require tailroom (e.g. for WEP ICV or MMIC) could end up with the counter at 0 instead of >0 and introduce insufficient skb tailroom failures because mac80211 would not resize skbs appropriately anymore. Fixes: 8d1f7ecd2af5 ("mac80211: defer tailroom counter manipulation when roaming") Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- Notes: While doing PATCH v2 [1/2] I've noticed a subtle bug in the delayed tailroom counter logic. Since this touches the codepaths [1/2] does I'm posting this as a pair. net/mac80211/key.c | 5 ++++- net/mac80211/main.c | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 577a11a13cdf..4c6f8c97d11a 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -695,10 +695,13 @@ void ieee80211_reset_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) mutex_lock(&sdata->local->key_mtx); sdata->crypto_tx_tailroom_needed_cnt = 0; + sdata->crypto_tx_tailroom_pending_dec = 0; if (sdata->vif.type == NL80211_IFTYPE_AP) { - list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) + list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) { vlan->crypto_tx_tailroom_needed_cnt = 0; + vlan->crypto_tx_tailroom_pending_dec = 0; + } } mutex_unlock(&sdata->local->key_mtx); diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 3c956c5f99b2..d8e1cbdcbc43 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -246,6 +246,7 @@ static void ieee80211_restart_work(struct work_struct *work) { struct ieee80211_local *local = container_of(work, struct ieee80211_local, restart_work); + struct ieee80211_sub_if_data *sdata; /* wait for scan work complete */ flush_workqueue(local->workqueue); @@ -254,6 +255,8 @@ static void ieee80211_restart_work(struct work_struct *work) "%s called with hardware scan in progress\n", __func__); rtnl_lock(); + list_for_each_entry(sdata, &local->interfaces, list) + cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk); ieee80211_scan_cancel(local); ieee80211_reconfig(local); rtnl_unlock(); -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption 2015-05-13 9:16 ` [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption Michal Kazior @ 2015-05-20 13:14 ` Johannes Berg 2015-05-21 8:16 ` Michal Kazior 2015-05-22 8:22 ` [PATCH v3] " Michal Kazior 1 sibling, 1 reply; 12+ messages in thread From: Johannes Berg @ 2015-05-20 13:14 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Wed, 2015-05-13 at 09:16 +0000, Michal Kazior wrote: > There was a possible race between > ieee80211_reconfig() and > ieee80211_delayed_tailroom_dec(). This could > result in inability to transmit data if driver > crashed during roaming or rekeying and subsequent > skbs with insufficient tailroom appeared. > > This race was probably never seen in the wild > because a device driver would have to crash AND > recover within 0.5s which is very unlikely. > > I was able to prove this race exists after > changing the delay to 10s locally and crashing > ath10k via debugfs immediately after GTK > rekeying. In case of ath10k the counter went below > 0. This was harmless but other drivers which > actually require tailroom (e.g. for WEP ICV or > MMIC) could end up with the counter at 0 instead > of >0 and introduce insufficient skb tailroom > failures because mac80211 would not resize skbs > appropriately anymore. > > Fixes: 8d1f7ecd2af5 ("mac80211: defer tailroom counter manipulation when roaming") > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > --- > > Notes: > While doing PATCH v2 [1/2] I've noticed a subtle bug in the > delayed tailroom counter logic. Since this touches the > codepaths [1/2] does I'm posting this as a pair. > > net/mac80211/key.c | 5 ++++- > net/mac80211/main.c | 3 +++ > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/net/mac80211/key.c b/net/mac80211/key.c > index 577a11a13cdf..4c6f8c97d11a 100644 > --- a/net/mac80211/key.c > +++ b/net/mac80211/key.c > @@ -695,10 +695,13 @@ void ieee80211_reset_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) > mutex_lock(&sdata->local->key_mtx); > > sdata->crypto_tx_tailroom_needed_cnt = 0; > + sdata->crypto_tx_tailroom_pending_dec = 0; > > if (sdata->vif.type == NL80211_IFTYPE_AP) { > - list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) > + list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) { > vlan->crypto_tx_tailroom_needed_cnt = 0; > + vlan->crypto_tx_tailroom_pending_dec = 0; > + } > } > > mutex_unlock(&sdata->local->key_mtx); > diff --git a/net/mac80211/main.c b/net/mac80211/main.c > index 3c956c5f99b2..d8e1cbdcbc43 100644 > --- a/net/mac80211/main.c > +++ b/net/mac80211/main.c > @@ -246,6 +246,7 @@ static void ieee80211_restart_work(struct work_struct *work) > { > struct ieee80211_local *local = > container_of(work, struct ieee80211_local, restart_work); > + struct ieee80211_sub_if_data *sdata; > > /* wait for scan work complete */ > flush_workqueue(local->workqueue); > @@ -254,6 +255,8 @@ static void ieee80211_restart_work(struct work_struct *work) > "%s called with hardware scan in progress\n", __func__); > > rtnl_lock(); > + list_for_each_entry(sdata, &local->interfaces, list) > + cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk); Would it make sense to just flush the work here? That way we don't have to do all the other things. johannes ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption 2015-05-20 13:14 ` Johannes Berg @ 2015-05-21 8:16 ` Michal Kazior 2015-05-21 8:30 ` Johannes Berg 0 siblings, 1 reply; 12+ messages in thread From: Michal Kazior @ 2015-05-21 8:16 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On 20 May 2015 at 15:14, Johannes Berg <johannes@sipsolutions.net> wrote: > On Wed, 2015-05-13 at 09:16 +0000, Michal Kazior wrote: >> There was a possible race between >> ieee80211_reconfig() and >> ieee80211_delayed_tailroom_dec(). This could >> result in inability to transmit data if driver >> crashed during roaming or rekeying and subsequent >> skbs with insufficient tailroom appeared. >> >> This race was probably never seen in the wild >> because a device driver would have to crash AND >> recover within 0.5s which is very unlikely. >> >> I was able to prove this race exists after >> changing the delay to 10s locally and crashing >> ath10k via debugfs immediately after GTK >> rekeying. In case of ath10k the counter went below >> 0. This was harmless but other drivers which >> actually require tailroom (e.g. for WEP ICV or >> MMIC) could end up with the counter at 0 instead >> of >0 and introduce insufficient skb tailroom >> failures because mac80211 would not resize skbs >> appropriately anymore. >> >> Fixes: 8d1f7ecd2af5 ("mac80211: defer tailroom counter manipulation when roaming") >> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> >> --- >> >> Notes: >> While doing PATCH v2 [1/2] I've noticed a subtle bug in the >> delayed tailroom counter logic. Since this touches the >> codepaths [1/2] does I'm posting this as a pair. >> >> net/mac80211/key.c | 5 ++++- >> net/mac80211/main.c | 3 +++ >> 2 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/net/mac80211/key.c b/net/mac80211/key.c >> index 577a11a13cdf..4c6f8c97d11a 100644 >> --- a/net/mac80211/key.c >> +++ b/net/mac80211/key.c >> @@ -695,10 +695,13 @@ void ieee80211_reset_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata) >> mutex_lock(&sdata->local->key_mtx); >> >> sdata->crypto_tx_tailroom_needed_cnt = 0; >> + sdata->crypto_tx_tailroom_pending_dec = 0; >> >> if (sdata->vif.type == NL80211_IFTYPE_AP) { >> - list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) >> + list_for_each_entry(vlan, &sdata->u.ap.vlans, u.vlan.list) { >> vlan->crypto_tx_tailroom_needed_cnt = 0; >> + vlan->crypto_tx_tailroom_pending_dec = 0; >> + } >> } >> >> mutex_unlock(&sdata->local->key_mtx); >> diff --git a/net/mac80211/main.c b/net/mac80211/main.c >> index 3c956c5f99b2..d8e1cbdcbc43 100644 >> --- a/net/mac80211/main.c >> +++ b/net/mac80211/main.c >> @@ -246,6 +246,7 @@ static void ieee80211_restart_work(struct work_struct *work) >> { >> struct ieee80211_local *local = >> container_of(work, struct ieee80211_local, restart_work); >> + struct ieee80211_sub_if_data *sdata; >> >> /* wait for scan work complete */ >> flush_workqueue(local->workqueue); >> @@ -254,6 +255,8 @@ static void ieee80211_restart_work(struct work_struct *work) >> "%s called with hardware scan in progress\n", __func__); >> >> rtnl_lock(); >> + list_for_each_entry(sdata, &local->interfaces, list) >> + cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk); > > Would it make sense to just flush the work here? That way we don't have > to do all the other things. Hmm.. dec_tailroom_needed_wk is queued on system workqueue now so there's no feasible way of flushing it (restart_work is on a system workqueue as well). It'd need to be moved to local->workqueue. I guess that would work too. Michał ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption 2015-05-21 8:16 ` Michal Kazior @ 2015-05-21 8:30 ` Johannes Berg 2015-05-21 8:44 ` Michal Kazior 0 siblings, 1 reply; 12+ messages in thread From: Johannes Berg @ 2015-05-21 8:30 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Thu, 2015-05-21 at 10:16 +0200, Michal Kazior wrote: > >> rtnl_lock(); > >> + list_for_each_entry(sdata, &local->interfaces, list) > >> + cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk); > > > > Would it make sense to just flush the work here? That way we don't have > > to do all the other things. > > Hmm.. dec_tailroom_needed_wk is queued on system workqueue now so > there's no feasible way of flushing it (restart_work is on a system > workqueue as well). It'd need to be moved to local->workqueue. I guess > that would work too. flush_work()? johannes ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption 2015-05-21 8:30 ` Johannes Berg @ 2015-05-21 8:44 ` Michal Kazior 0 siblings, 0 replies; 12+ messages in thread From: Michal Kazior @ 2015-05-21 8:44 UTC (permalink / raw) To: Johannes Berg; +Cc: linux-wireless On 21 May 2015 at 10:30, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2015-05-21 at 10:16 +0200, Michal Kazior wrote: > >> >> rtnl_lock(); >> >> + list_for_each_entry(sdata, &local->interfaces, list) >> >> + cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk); >> > >> > Would it make sense to just flush the work here? That way we don't have >> > to do all the other things. >> >> Hmm.. dec_tailroom_needed_wk is queued on system workqueue now so >> there's no feasible way of flushing it (restart_work is on a system >> workqueue as well). It'd need to be moved to local->workqueue. I guess >> that would work too. > > flush_work()? Oh. I wasn't aware of this call.. Thanks for pointing out :-) Michał ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] mac80211: prevent possible crypto tx tailroom corruption 2015-05-13 9:16 ` [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption Michal Kazior 2015-05-20 13:14 ` Johannes Berg @ 2015-05-22 8:22 ` Michal Kazior 2015-05-29 11:05 ` Johannes Berg 1 sibling, 1 reply; 12+ messages in thread From: Michal Kazior @ 2015-05-22 8:22 UTC (permalink / raw) To: linux-wireless; +Cc: johannes, Michal Kazior There was a possible race between ieee80211_reconfig() and ieee80211_delayed_tailroom_dec(). This could result in inability to transmit data if driver crashed during roaming or rekeying and subsequent skbs with insufficient tailroom appeared. This race was probably never seen in the wild because a device driver would have to crash AND recover within 0.5s which is very unlikely. I was able to prove this race exists after changing the delay to 10s locally and crashing ath10k via debugfs immediately after GTK rekeying. In case of ath10k the counter went below 0. This was harmless but other drivers which actually require tailroom (e.g. for WEP ICV or MMIC) could end up with the counter at 0 instead of >0 and introduce insufficient skb tailroom failures because mac80211 would not resize skbs appropriately anymore. Fixes: 8d1f7ecd2af5 ("mac80211: defer tailroom counter manipulation when roaming") Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- Notes: v3: * use flush_work() [Johannes] net/mac80211/main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 99d27babd9f0..674164fe5cdb 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -246,6 +246,7 @@ static void ieee80211_restart_work(struct work_struct *work) { struct ieee80211_local *local = container_of(work, struct ieee80211_local, restart_work); + struct ieee80211_sub_if_data *sdata; /* wait for scan work complete */ flush_workqueue(local->workqueue); @@ -254,6 +255,8 @@ static void ieee80211_restart_work(struct work_struct *work) "%s called with hardware scan in progress\n", __func__); rtnl_lock(); + list_for_each_entry(sdata, &local->interfaces, list) + flush_delayed_work(&sdata->dec_tailroom_needed_wk); ieee80211_scan_cancel(local); ieee80211_reconfig(local); rtnl_unlock(); -- 2.1.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] mac80211: prevent possible crypto tx tailroom corruption 2015-05-22 8:22 ` [PATCH v3] " Michal Kazior @ 2015-05-29 11:05 ` Johannes Berg 0 siblings, 0 replies; 12+ messages in thread From: Johannes Berg @ 2015-05-29 11:05 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Fri, 2015-05-22 at 10:22 +0200, Michal Kazior wrote: > There was a possible race between > ieee80211_reconfig() and > ieee80211_delayed_tailroom_dec(). This could > result in inability to transmit data if driver > crashed during roaming or rekeying and subsequent > skbs with insufficient tailroom appeared. > > This race was probably never seen in the wild > because a device driver would have to crash AND > recover within 0.5s which is very unlikely. > > I was able to prove this race exists after > changing the delay to 10s locally and crashing > ath10k via debugfs immediately after GTK > rekeying. In case of ath10k the counter went below > 0. This was harmless but other drivers which > actually require tailroom (e.g. for WEP ICV or > MMIC) could end up with the counter at 0 instead > of >0 and introduce insufficient skb tailroom > failures because mac80211 would not resize skbs > appropriately anymore. > > Fixes: 8d1f7ecd2af5 ("mac80211: defer tailroom counter manipulation when roaming") > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> Applied. johannes ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] mac80211: fix AP_VLAN crypto tailroom calculation 2015-05-13 9:16 ` [PATCH v2 1/2] " Michal Kazior 2015-05-13 9:16 ` [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption Michal Kazior @ 2015-05-20 13:12 ` Johannes Berg 1 sibling, 0 replies; 12+ messages in thread From: Johannes Berg @ 2015-05-20 13:12 UTC (permalink / raw) To: Michal Kazior; +Cc: linux-wireless On Wed, 2015-05-13 at 09:16 +0000, Michal Kazior wrote: > Some splats I was seeing: > > (a) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wep.c:102 ieee80211_wep_add_iv > (b) WARNING: CPU: 1 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:73 ieee80211_tx_h_michael_mic_add > (c) WARNING: CPU: 3 PID: 0 at /devel/src/linux/net/mac80211/wpa.c:433 ieee80211_crypto_ccmp_encrypt > > I've seen (a) and (b) with ath9k hw crypto and (c) > with ath9k sw crypto. All of them were related to > insufficient skb tailroom and I was able to > trigger these with ping6 program. > > AP_VLANs may inherit crypto keys from parent AP. > This wasn't considered and yielded problems in > some setups resulting in inability to transmit > data because mac80211 wouldn't resize skbs when > necessary and subsequently drop some packets due > to insufficient tailroom. > > For efficiency purposes don't inspect both AP_VLAN > and AP sdata looking for tailroom counter. Instead > update AP_VLAN tailroom counters whenever their > master AP tailroom counter changes. Applied. johannes ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-05-29 11:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-05-11 13:05 [PATCH] mac80211: fix AP_VLAN crypto tailroom calculation Michal Kazior 2015-05-11 13:11 ` Johannes Berg 2015-05-11 13:33 ` Michal Kazior 2015-05-13 9:16 ` [PATCH v2 1/2] " Michal Kazior 2015-05-13 9:16 ` [PATCH v2 2/2] mac80211: prevent possible crypto tx tailroom corruption Michal Kazior 2015-05-20 13:14 ` Johannes Berg 2015-05-21 8:16 ` Michal Kazior 2015-05-21 8:30 ` Johannes Berg 2015-05-21 8:44 ` Michal Kazior 2015-05-22 8:22 ` [PATCH v3] " Michal Kazior 2015-05-29 11:05 ` Johannes Berg 2015-05-20 13:12 ` [PATCH v2 1/2] mac80211: fix AP_VLAN crypto tailroom calculation 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).