On Thu, 2023-08-17 at 12:06 +0000, Ping-Ke Shih wrote: > 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. Yeah I understood later - never mind. > > > 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. Actually I don't know - do you have a driver that's actually supporting >64 BlockAck on stable? I initially missed entirely the fact that the UBSAN happens on the normal RX path, not (only) on the "mark as filtered" path. Sorry I didn't make that clear. > > > > > > +++ 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. Yeah. > 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. Right, which I had missed initially in review. > 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? I think what you did is fine. Just send the patch as it was? johannes
