netdev - Re: [RFC PATCH] mac80211: clip ADDBA instead of bailing out

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1560b292-6366-4588-ad4d-654377613b84@gmail.com>
Date: Wed, 19 Mar 2025 06:21:51 -0700
From: James Prestwood <prestwoj@...il.com>
To: Alexandre Ferrieux <alexandre.ferrieux@...il.com>,
 linux-wireless@...r.kernel.org
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
 Johannes Berg <johannes@...solutions.net>
Subject: Re: [RFC PATCH] mac80211: clip ADDBA instead of bailing out

Hi Alexandre,

On 3/19/25 3:58 AM, Alexandre Ferrieux wrote:
> When a Linux Wifi{4,5} device talks to a Wifi6 AP, if the AP proposes a Block
> Acknowledgement aggregation size (ADDBA) exceeding its expectations, the code in
> mac80211 just bails out, rejecting the aggregation. This yields a big
> performance penalty on the ack path, which is observable in comparison with
> other OSes (Windows and MacOS) which "play smarter" and accept the proposal with
> a "clipped" size.
Out of curiosity do you have any performance numbers for this, like 
Linux vs Windows vs MacOS? We ran into a significant performance hit 
after I added multicast RX support on ath10k (after ~30 clients were on 
the same channel). After looking into the pcaps we saw many ADDBA 
failures and ultimately had to disable multicast RX. I want to give this 
patch a try either way, but I was curious if you had any data on 
performance improvements.
>
> A typical scenario would be:
>
>    AP -> Device : ADDBA_request(size=256)
>
> Current Linux reaction:
>
>    Device -> AP : ADDBA_reply(failure)
>
> Other OSes reaction:
>
>    Device -> AP : ADDBA_reply(size=64)
>
> Note that the IEEE802.11 standard allows for both reactions, but it sounds
> really suboptimal to be bailing out instead of clipping. The patch below does
> the latter.
>
> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@...il.com>
> ---
>
> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
> index f3fbe5a4395e..264dad847842 100644
> --- a/net/mac80211/agg-rx.c
> +++ b/net/mac80211/agg-rx.c
> @@ -317,18 +317,20 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta,
>                  max_buf_size = IEEE80211_MAX_AMPDU_BUF_HT;
>
>          /* sanity check for incoming parameters:
> -        * check if configuration can support the BA policy
> -        * and if buffer size does not exceeds max value */
> +        * check if configuration can support the BA policy */
>          /* XXX: check own ht delayed BA capability?? */
>          if (((ba_policy != 1) &&
> -            (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA))) ||
> -           (buf_size > max_buf_size)) {
> -               status = WLAN_STATUS_INVALID_QOS_PARAM;
> +            (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA)))) {
> +               status = WLAN_STATUS_INVALID_QOS_PARAM;
>                  ht_dbg_ratelimited(sta->sdata,
>                                     "AddBA Req with bad params from %pM on tid
> %u. policy %d, buffer size %d\n",
>                                     sta->sta.addr, tid, ba_policy, buf_size);
>                  goto end;
>          }
> +       if (buf_size > max_buf_size) {
> +         buf_size = max_buf_size ; // Clip instead of bailing out
> +       }
> +
>          /* determine default buffer size */
>          if (buf_size == 0)
>                  buf_size = max_buf_size;
>
>