| Message ID | 20240117124848.120438-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: don't ask rate control with zero rate mask if scanning | expand |
On Wed, 2024-01-17 at 15:48 +0300, Dmitry Antipov wrote: > If we're scanning and got the control frame with zero rate mask, drop > the frame before '__rate_control_send_low()' getting stuck attempting > to select supported rate. But why drop the frame? I'm still thinking that it just doesn't really make sense to apply the rate mask to scanning at all? The most common use case for this is probably P2P-style things where you just don't want to use CCK, but for scanning we have NL80211_ATTR_TX_NO_CCK_RATE for this, so there's really no need to apply the rate mask? johannes
On 1/18/24 16:46, Johannes Berg wrote: > On Wed, 2024-01-17 at 15:48 +0300, Dmitry Antipov wrote: >> If we're scanning and got the control frame with zero rate mask, drop >> the frame before '__rate_control_send_low()' getting stuck attempting >> to select supported rate. > > But why drop the frame? I'm still thinking that it just doesn't really > make sense to apply the rate mask to scanning at all? Hm. It seems that I'm still missing something important, but I don't realize why ieee80211_scan_state_set_channel() advances to the (next) channel even after ieee80211_set_bitrate_mask() resets this channel's rate mask to 0. Note that the comment in ieee80211_set_bitrate_mask() explicitly states that there should be at least one usable rate for the band we're currently operating on. Why this is not applicable to other band(s) we might probe next? > The most common use case for this is probably P2P-style things where you > just don't want to use CCK, but for scanning we have > NL80211_ATTR_TX_NO_CCK_RATE for this, so there's really no need to apply > the rate mask? Does NL80211_ATTR_TX_NO_CCK_RATE makes an effect on bands other than 2.4GHz? Note original reproducer triggers WARN_ONCE() after switching to 5GHz. Dmitry
On Fri, 2024-01-19 at 11:07 +0300, Dmitry Antipov wrote: > Hm. It seems that I'm still missing something important, but I don't > realize why ieee80211_scan_state_set_channel() advances to the (next) > channel even after ieee80211_set_bitrate_mask() resets this channel's > rate mask to 0. Note that the comment in ieee80211_set_bitrate_mask() > explicitly states that there should be at least one usable rate for > the band we're currently operating on. Why this is not applicable to > other band(s) we might probe next? I guess it depends on the definition of 'operating'? Scanning isn't really 'operating' in this context, I'd argue. > > > The most common use case for this is probably P2P-style things where you > > just don't want to use CCK, but for scanning we have > > NL80211_ATTR_TX_NO_CCK_RATE for this, so there's really no need to apply > > the rate mask? > > Does NL80211_ATTR_TX_NO_CCK_RATE makes an effect on bands other than > 2.4GHz? No. > Note original reproducer triggers WARN_ONCE() after switching > to 5GHz. Sure, that's unrelated. I'm guessing the reproducer sets up a (limited) rate set only for 2.4 GHz and that's OK because that's where it's connected, or so, but then (software) scanning interferes. My reason for mentioning was just that NL80211_ATTR_TX_NO_CCK_RATE is meant to aid restricting rates while scanning, so I'm saying that we should probably not even _use_ the bitrate mask for scanning at all. I was trying to let you come up with a patch for learning, but I think at this point just making one illustrates better what I'm thinking ... Here's one, but I haven't even compiled this: --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -932,6 +932,8 @@ enum mac80211_tx_info_flags { * of their QoS TID or other priority field values. * @IEEE80211_TX_CTRL_MCAST_MLO_FIRST_TX: first MLO TX, used mostly internally * for sequence number assignment + * @IEEE80211_TX_CTRL_SCAN_TX: Indicates that this frame is transmitted + * due to scanning, not in normal operation on the interface. * @IEEE80211_TX_CTRL_MLO_LINK: If not @IEEE80211_LINK_UNSPECIFIED, this * frame should be transmitted on the specific link. This really is * only relevant for frames that do not have data present, and is @@ -952,6 +954,7 @@ enum mac80211_tx_control_flags { IEEE80211_TX_CTRL_NO_SEQNO = BIT(7), IEEE80211_TX_CTRL_DONT_REORDER = BIT(8), IEEE80211_TX_CTRL_MCAST_MLO_FIRST_TX = BIT(9), + IEEE80211_TX_CTRL_SCAN_TX = BIT(10), IEEE80211_TX_CTRL_MLO_LINK = 0xf0000000, }; --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -632,6 +632,8 @@ static void ieee80211_send_scan_probe_req(struct ieee80211_sub_if_data *sdata, cpu_to_le16(IEEE80211_SN_TO_SEQ(sn)); } IEEE80211_SKB_CB(skb)->flags |= tx_flags; + IEEE80211_SKB_CB(skb)->control.flags |= + IEEE80211_TX_CTRL_SCAN_TX; ieee80211_tx_skb_tid_band(sdata, skb, 7, channel->band); } } --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -701,7 +701,9 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) txrc.bss_conf = &tx->sdata->vif.bss_conf; txrc.skb = tx->skb; txrc.reported_rate.idx = -1; - txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[info->band]; + + if (!(info->control.flags & IEEE80211_TX_CTRL_SCAN_TX)) + txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[info->band]; if (tx->sdata->rc_has_mcs_mask[info->band]) txrc.rate_idx_mcs_mask = What do you think? johannes
On 1/19/24 20:19, Johannes Berg wrote: > I was trying to let you come up with a patch for learning, but I think > at this point just making one illustrates better what I'm thinking ... > Here's one, but I haven't even compiled this: Hmm... this: > --- a/net/mac80211/tx.c > +++ b/net/mac80211/tx.c > @@ -701,7 +701,9 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) > txrc.bss_conf = &tx->sdata->vif.bss_conf; > txrc.skb = tx->skb; > txrc.reported_rate.idx = -1; > - txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[info->band]; > + > + if (!(info->control.flags & IEEE80211_TX_CTRL_SCAN_TX)) > + txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[info->band]; > > if (tx->sdata->rc_has_mcs_mask[info->band]) > txrc.rate_idx_mcs_mask = leaves 'rate_idx_mask' unset (i.e. zero) even for band 0 and then causes the same WARN_ONCE() even earlier (before switching to band 1). Dmitry
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 314998fdb1a5..53a473a2f8dd 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -701,7 +701,12 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) txrc.bss_conf = &tx->sdata->vif.bss_conf; txrc.skb = tx->skb; txrc.reported_rate.idx = -1; - txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[info->band]; + + if (tx->sdata->rc_rateidx_mask[info->band]) + txrc.rate_idx_mask = tx->sdata->rc_rateidx_mask[info->band]; + else if (test_bit(SCAN_SW_SCANNING, &tx->local->scanning)) + /* we're scanning but have no usable rates */ + return TX_DROP; if (tx->sdata->rc_has_mcs_mask[info->band]) txrc.rate_idx_mcs_mask =
If we're scanning and got the control frame with zero rate mask, drop the frame before '__rate_control_send_low()' getting stuck attempting to select supported rate. Reported-by: syzbot+fdc5123366fb9c3fdc6d@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=fdc5123366fb9c3fdc6d Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- net/mac80211/tx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)