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: <Stable@xxxxxxxxxxxxxxx>
>
> 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
