Hi,
I mark this as RFC, because I'm not sure if iwlwifi needs to extend
ieee80211_mark_rx_ba_filtered_frames() to support mew hardware that
hw.max_rx_aggregation_subframes is larger than 64. If not, we can just
add some conditions to avoid UBSAN warning like this patch. Otherwise,
this RFC can't entirely resolve the problem.
Summarize possible cases
use
case hw.max_rx_agg mark_rx_ba w/o this with this
---- ------------- ---------- ----------- ---------
1 > 64 o UBSAN warn UBSAN warn (no change still)
WARN_ONCE() to note people don't use mark_rx_ba()
2 > 64 x UBSAN warn no UBSAN warn
3 <= 64 o work no change
4 <= 64 x work no change
* hw.max_rx_agg is short for hw.max_rx_aggregation_subframes
* mark_rx_ba is short for ieee80211_mark_rx_ba_filtered_frames()
This RFC doesn't try to fix case 1, because the case may be not existing.
Also, a possible implementation could be to declare 1024 bitmap for EHT
hardware, and that has extra cost to shift this bitmap (compare with
original u64). So, just give it a warning.
Following is candidate of commit message if this RFC can be accepted:
The commit 06470f7468c8 ("mac80211: add API to allow filtering frames in BA sessions")
adds reorder_buf_filtered to mark frames filtered by firmware, but new
hardware can support hw.max_rx_aggregation_subframes more than 64 frames.
Then, it leads UBSAN warning:
UBSAN: shift-out-of-bounds in net/mac80211/rx.c:1129:39
shift exponent 215 is too large for 64-bit type 'long long unsigned int'
Call Trace:
<IRQ>
dump_stack_lvl+0x48/0x70
dump_stack+0x10/0x20
__ubsan_handle_shift_out_of_bounds+0x1ac/0x360
ieee80211_release_reorder_frame.constprop.0.cold+0x64/0x69 [mac80211]
ieee80211_sta_reorder_release+0x9c/0x400 [mac80211]
ieee80211_prepare_and_rx_handle+0x1234/0x1420 [mac80211]
? __pfx_jhash+0x10/0x10
? rht_key_get_hash.isra.0+0x19/0x30 [mac80211]
ieee80211_rx_list+0xaef/0xf60 [mac80211]
? kfree_skbmem+0x58/0xb0
? rtw89_vif_rx_stats_iter+0x2bb/0x2e1 [rtw89_core]
ieee80211_rx_napi+0x53/0xd0 [mac80211]
Since only old hardware with 64 or less RX aggregation frames uses
ieee80211_mark_rx_ba_filtered_frames(), add a WARN_ONCE() and comment to
note to avoid using this function if hardware capability is not suitable.
Cc: <[email protected]>
Signed-off-by: Ping-Ke Shih <[email protected]>
---
include/net/mac80211.h | 1 +
net/mac80211/rx.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 3a8a2d2c58c3..2a55ae932c56 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -6612,6 +6612,7 @@ void ieee80211_stop_rx_ba_session(struct ieee80211_vif *vif, u16 ba_rx_bitmap,
* marks frames marked in the bitmap as having been filtered. Afterwards, it
* checks if any frames in the window starting from @ssn can now be released
* (in case they were only waiting for frames that were filtered.)
+ * (Only work correctly if @max_rx_aggregation_subframes <= 64 frames)
*/
void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
u16 ssn, u64 filtered,
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 4f707d2a160f..0af2599c17e8 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1083,7 +1083,8 @@ static inline bool ieee80211_rx_reorder_ready(struct tid_ampdu_rx *tid_agg_rx,
struct sk_buff *tail = skb_peek_tail(frames);
struct ieee80211_rx_status *status;
- if (tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
+ if (tid_agg_rx->reorder_buf_filtered &&
+ tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
return true;
if (!tail)
@@ -1124,7 +1125,8 @@ static void ieee80211_release_reorder_frame(struct ieee80211_sub_if_data *sdata,
}
no_frame:
- tid_agg_rx->reorder_buf_filtered &= ~BIT_ULL(index);
+ if (tid_agg_rx->reorder_buf_filtered)
+ tid_agg_rx->reorder_buf_filtered &= ~BIT_ULL(index);
tid_agg_rx->head_seq_num = ieee80211_sn_inc(tid_agg_rx->head_seq_num);
}
@@ -4264,6 +4266,7 @@ void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
u16 ssn, u64 filtered,
u16 received_mpdus)
{
+ struct ieee80211_local *local;
struct sta_info *sta;
struct tid_ampdu_rx *tid_agg_rx;
struct sk_buff_head frames;
@@ -4281,6 +4284,11 @@ void ieee80211_mark_rx_ba_filtered_frames(struct ieee80211_sta *pubsta, u8 tid,
sta = container_of(pubsta, struct sta_info, sta);
+ local = sta->sdata->local;
+ WARN_ONCE(local->hw.max_rx_aggregation_subframes > 64,
+ "RX BA marker can't support max_rx_aggregation_subframes %u > 64\n",
+ local->hw.max_rx_aggregation_subframes);
+
if (!ieee80211_rx_data_set_sta(&rx, sta, -1))
return;
--
2.25.1
On Thu, 2023-08-17 at 09:03 +0200, Johannes Berg wrote:
>
> >
> > If not, we can just
> > add some conditions to avoid UBSAN warning like this patch. Otherwise,
> > this RFC can't entirely resolve the problem.
>
> Seems fine. I'd kind of probably not word it as "fix UBSAN" since really
> it's just more along the lines of "warn if API is misused"? :)
Maybe, combine two? Because I want to avoid UBSAN warning initially.
>
> > Since only old hardware with 64 or less RX aggregation frames uses
> > ieee80211_mark_rx_ba_filtered_frames(), add a WARN_ONCE() and comment to
> > note to avoid using this function if hardware capability is not suitable.
> >
> > Cc: <[email protected]>
>
> I don't really think this is stable material - if there's a driver
> that's calling this when >64 frames is supported then it's a driver bug
> that should be fixed, and if not then there's no bug?
I'll remove this.
>
> > +++ b/net/mac80211/rx.c
> > @@ -1083,7 +1083,8 @@ static inline bool ieee80211_rx_reorder_ready(struct tid_ampdu_rx
> > *tid_agg_rx,
> > struct sk_buff *tail = skb_peek_tail(frames);
> > struct ieee80211_rx_status *status;
> >
> > - if (tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
> > + if (tid_agg_rx->reorder_buf_filtered &&
> > + tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
> > return true;
>
> Or maybe no - this part is what you think should be
This function will be called by all drivers that rely on mac80211's rx reordering.
The UBSAN find `index` of `BIT_ULL(index)` could be over 64, for example
index=215 when I set hw.max_rx_aggregation_subframes to 256. This is initial
problem I want to fix.
The `tid_agg_rx->reorder_buf_filtered != 0` means hw.max_rx_aggregation_subframes <= 64
(as well as index <= 64) implicitly, because only this kind of hardware can call
ieee80211_mark_rx_ba_filtered_frames(). Maybe, below would be intuitive, but
I worry it hides problem that UBSAN can't find.
if (index < 64 && tid_agg_rx->reorder_buf_filtered & BIT_ULL(index))
return true;
Any suggestion? or prefer?
>
> > if (!tail)
> > @@ -1124,7 +1125,8 @@ static void ieee80211_release_reorder_frame(struct ieee80211_sub_if_data
> > *sdata,
> > }
> >
> > no_frame:
> > - tid_agg_rx->reorder_buf_filtered &= ~BIT_ULL(index);
> > + if (tid_agg_rx->reorder_buf_filtered)
> > + tid_agg_rx->reorder_buf_filtered &= ~BIT_ULL(index);
>
> And this.
The same as above.
Thank you
Ping-Ke