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. Oh, good catch and question, but no. This firmware notification cannot appear in newer devices, and in fact we don't use mac80211 reordering at all for a long time (since RSS was introduced, basically) and should probably prevent handling of this notification. > 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"? :) > 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? > +++ 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 > > 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. > @@ -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); > + >From your description I was only thinking about this. So yeah I guess it does make some sense to actually call it a fix - for the parts about using the filtered value with >64 subframes supported ... Looks fine to me then! johannes
