| Message ID | 1456954113-4682-1-git-send-email-emmanuel.grumbach@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, Mar 02, 2016 at 11:28:31PM +0200, Emmanuel Grumbach wrote: > Even if the current chandef width is equal to the station's max-BW, it > doesn't mean it's a valid width for TDLS. Make sure to always check > regulatory constraints in these cases. I'm not sure this change is the trigger for this issue, but since I noticed this for the first time today and this commit went just in into wireless-testing.git, it sounds quite likely that this was indeed behind the busy loop I saw here: > diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c > @@ -332,7 +332,7 @@ ieee80211_tdls_chandef_vht_upgrade(struct ieee80211_sub_if_data *sdata, > /* proceed to downgrade the chandef until usable or the same */ > - while (uc.width > max_width && > + while (uc.width > max_width || > !cfg80211_reg_can_beacon_relax(sdata->local->hw.wiphy, &uc, > sdata->wdev.iftype)) > ieee80211_chandef_downgrade(&uc); I'm not sure what caused the chandef in uc (sta->tdls_chandef) to be invalid (actually, I do know now; see below), but when running the ap_open_tdls_vht80plus80 test case, the VM got stuck in a busy loop printing warnings about that chandef being invalid and with this while loop being here, that never stopped.. Well, until couple of hours later when I noticed this and stopped in manually. That left 1.6 GB of kernel log entries (and that would have been way more had printk not refused to print so much, but even the "589 printk messages dropped" prints were enough to make this huge).. So if uc is invalid here, it looks like this loop can get into a state where it never terminates. The iteration hits these warnings: WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:336 cfg80211_chandef_dfs_required+0xf6/0x100() WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:608 cfg80211_chandef_usable+0x188/0x190() WARNING: CPU: 2 PID: 623 at net/mac80211/util.c:2860 ieee80211_chandef_downgrade+0x138/0x170() The last entry here seems to imply that c->width downgrade happened once successfully since no other WARN_ON_ONCE() were printed within ieee80211_chandef_downgrade(). This is followed by: WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:336 cfg80211_chandef_dfs_required+0xf6/0x100() WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:608 cfg80211_chandef_usable+0x188/0x190() WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:336 cfg80211_chandef_dfs_required+0xf6/0x100() WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:608 cfg80211_chandef_usable+0x188/0x190() WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:336 cfg80211_chandef_dfs_required+0xf6/0x100() WARNING: CPU: 2 PID: 623 at net/wireless/chan.c:608 cfg80211_chandef_usable+0x188/0x190() WARNING: CPU: 2 PID: 623 at net/mac80211/util.c:2848 ieee80211_chandef_downgrade+0x39/0x170() The last one here is hitting the WARN_ON_ONCE(1) in the default case, so it looks like there were two more successful downgrades (total three starting from 80+80) and then we are in a busy loop with every new iteration hitting that default case warning at util.c:2848. This continues indefinitely. Regardless of what caused the chandef to be invalid, this while loop would benefit of some added robustness to avoid the possibility of an infinite loop where the channel width cannot be downgraded anymore. The console log from that run is available here: http://w1.fi/a/tdls-downgrade-warning-loop.txt It looks like I can now reproduce this easily with ./vm-run.sh ap_open_tdls_vht80plus80 Reverting this one-liner patch removes that loop and all of the chandef invalid warnings. With this patch applied, ieee80211_tdls_chandef_vht_upgrade() shows uc.width == 4, max_width == 4, and chandef valid at the beginning of the function. Just before the while loop, uc.width == 3, max_width == 3, chandef is now invalid. On loop iterations, uc.width is dropped to 2, 1, and finally 0 where it remains while the loop continues running. The chandef becomes invalid when going through the centers_80mhz loop and setting uc.center_freq1 = 5210, uc.width = NL80211_CHAN_WIDTH_80. The AP was configured with seg0_idx 42 + seg1_idx 155.
On Sun, Mar 6, 2016 at 5:58 PM, Jouni Malinen <j@w1.fi> wrote: > > On Wed, Mar 02, 2016 at 11:28:31PM +0200, Emmanuel Grumbach wrote: > > Even if the current chandef width is equal to the station's max-BW, it > > doesn't mean it's a valid width for TDLS. Make sure to always check > > regulatory constraints in these cases. > > I'm not sure this change is the trigger for this issue, but since I > noticed this for the first time today and this commit went just in into > wireless-testing.git, it sounds quite likely that this was indeed behind > the busy loop I saw here: > > > diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c > > @@ -332,7 +332,7 @@ ieee80211_tdls_chandef_vht_upgrade(struct ieee80211_sub_if_data *sdata, > > /* proceed to downgrade the chandef until usable or the same */ > > - while (uc.width > max_width && > > + while (uc.width > max_width || > > !cfg80211_reg_can_beacon_relax(sdata->local->hw.wiphy, &uc, > > sdata->wdev.iftype)) > > ieee80211_chandef_downgrade(&uc); Good catch :) We actually just noticed this as well and have a suggested fix already - basically the code was trying to *upgrade* a 80p80 channel to a 80 one. I guess it will be out in a couple days after some internal testing. Arik -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c index c9eeb3f..43f13abe 100644 --- a/net/mac80211/tdls.c +++ b/net/mac80211/tdls.c @@ -332,7 +332,7 @@ ieee80211_tdls_chandef_vht_upgrade(struct ieee80211_sub_if_data *sdata, return; /* proceed to downgrade the chandef until usable or the same */ - while (uc.width > max_width && + while (uc.width > max_width || !cfg80211_reg_can_beacon_relax(sdata->local->hw.wiphy, &uc, sdata->wdev.iftype)) ieee80211_chandef_downgrade(&uc);