wifi: mac80211: don't ask rate control with zero rate mask if scanning

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(-)

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