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

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

[Dmitry Antipov]

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

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 =
-- 
2.43.0

Re: [PATCH] wifi: mac80211: don't ask rate control with zero rate mask if scanning

[Johannes Berg]

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

Re: [PATCH] wifi: mac80211: don't ask rate control with zero rate mask if scanning

[Dmitry Antipov]

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

Re: [PATCH] wifi: mac80211: don't ask rate control with zero rate mask if scanning

[Johannes Berg]

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

Re: [PATCH] wifi: mac80211: don't ask rate control with zero rate mask if scanning

[Dmitry Antipov]

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

[PATCH 0/1] Re: wifi: mac80211: don't ask rate control with zero rate mask if scanning

[Dmitry Antipov]

Well, the literal approach to "not even use the bitrate mask for scanning"
may look like the following. It fixes an original WARN_ONCE() as reported
by syzkaller, but I'm definitely interesting on how to check whether it
doesn't break something else.

Dmitry Antipov (1):
  wifi: mac80211: ignore the bitrate mask when scanning

 include/net/mac80211.h | 3 +++
 net/mac80211/rate.c    | 4 +++-
 net/mac80211/scan.c    | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/1] wifi: mac80211: ignore the bitrate mask when scanning

[Dmitry Antipov]

When the frame is transmitted due to scanning rather than
regular operation on the interface, ignore the bitrate mask.

Reported-by: syzbot+fdc5123366fb9c3fdc6d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fdc5123366fb9c3fdc6d
Suggested-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 include/net/mac80211.h | 3 +++
 net/mac80211/rate.c    | 4 +++-
 net/mac80211/scan.c    | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index d400fe2e8668..df9b578e58bb 100644
--- 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,
 };
 
diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index d5ea5f5bcf3a..6878fe454c36 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -351,6 +351,7 @@ static void __rate_control_send_low(struct ieee80211_hw *hw,
 	int i;
 	u32 rate_flags =
 		ieee80211_chandef_rate_flags(&hw->conf.chandef);
+	bool scanning = !!(info->control.flags & IEEE80211_TX_CTRL_SCAN_TX);
 
 	if (sband->band == NL80211_BAND_S1GHZ) {
 		info->control.rates[0].flags |= IEEE80211_TX_RC_S1G_MCS;
@@ -364,7 +365,8 @@ static void __rate_control_send_low(struct ieee80211_hw *hw,
 
 	info->control.rates[0].idx = 0;
 	for (i = 0; i < sband->n_bitrates; i++) {
-		if (!(rate_mask & BIT(i)))
+		/* Do not use the bitrate mask when scanning. */
+		if (!scanning && !(rate_mask & BIT(i)))
 			continue;
 
 		if ((rate_flags & sband->bitrates[i].flags) != rate_flags)
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 645355e5f1bc..dd68dfe46e0e 100644
--- 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);
 	}
 }
-- 
2.43.0

Re: [PATCH 0/1] Re: wifi: mac80211: don't ask rate control with zero rate mask if scanning

[Johannes Berg]

On Thu, 2024-01-25 at 12:07 +0300, Dmitry Antipov wrote:
> Well, the literal approach to "not even use the bitrate mask for scanning"
> may look like the following. It fixes an original WARN_ONCE() as reported
> by syzkaller, but I'm definitely interesting on how to check whether it
> doesn't break something else.

I will always run the hwsim tests in hostap, but I'm not really
concerned it would break anything - this was never really _intended_ to
cover scanning in the first place.

johannes

Re: [PATCH 1/1] wifi: mac80211: ignore the bitrate mask when scanning

[Johannes Berg]

On Thu, 2024-01-25 at 12:07 +0300, Dmitry Antipov wrote:
> 
> +++ b/net/mac80211/rate.c
> @@ -351,6 +351,7 @@ static void __rate_control_send_low(struct ieee80211_hw *hw,
>  	int i;
>  	u32 rate_flags =
>  		ieee80211_chandef_rate_flags(&hw->conf.chandef);
> +	bool scanning = !!(info->control.flags & IEEE80211_TX_CTRL_SCAN_TX);
>  
>  	if (sband->band == NL80211_BAND_S1GHZ) {
>  		info->control.rates[0].flags |= IEEE80211_TX_RC_S1G_MCS;
> @@ -364,7 +365,8 @@ static void __rate_control_send_low(struct ieee80211_hw *hw,
>  
>  	info->control.rates[0].idx = 0;
>  	for (i = 0; i < sband->n_bitrates; i++) {
> -		if (!(rate_mask & BIT(i)))
> +		/* Do not use the bitrate mask when scanning. */
> +		if (!scanning && !(rate_mask & BIT(i)))
>  			continue;
> 

I know my patch didn't work, but I think I'd still prefer if we didn't
pass a 0 rate mask around to ignore it - maybe we can just make it ~0 in
the outer place that looks at the sdata mask(s)?

I'm not sure why we fill "BIT(sband->n_bitrates) - 1" rather than ~0 in
the first place, we always (have to) check anyway, so wouldn't really
care about having more bits set than rates exist ...

johannes