Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jean-Pierre TOSONI <jp.tosoni@acksys.fr>
To:     "Johannes Berg (johannes@sipsolutions.net)" 
        <johannes@sipsolutions.net>,
        "'linux-wireless@vger.kernel.org'" <linux-wireless@vger.kernel.org>
Subject: RE: [RFC v3] mac80211: fix rx blockack session race condition
Thread-Topic: [RFC v3] mac80211: fix rx blockack session race condition
Thread-Index: AdfL0tUGplDdZUWUTvSEilo32QEGpQAELkXg
Date:   Thu, 28 Oct 2021 10:08:09 +0000
Message-ID: <DB9PR01MB73545D195C51A3DDACE38E7CE4869@DB9PR01MB7354.eurprd01.prod.exchangelabs.com>
References: <DB9PR01MB7354781F66D4D59611D256A6E4869@DB9PR01MB7354.eurprd01.prod.exchangelabs.com>
In-Reply-To: <DB9PR01MB7354781F66D4D59611D256A6E4869@DB9PR01MB7354.eurprd01.prod.exchangelabs.com>
Accept-Language: fr-FR, en-US
Content-Language: fr-FR
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?B2SLA6+m2crCdyqF331g5p+2AM6y+8MUZR2x7+IKJnKCIAfPrubFqzp7/z?=
 =?iso-8859-1?Q?iIFZYoHrveih244xzvvUz+AazUDFeTQoQRv9A+qF4954M8zabC32hQ/T6O?=
 =?iso-8859-1?Q?UE1S0xgvgG42VVCq16bmHJdB6g1+mP/SZyQ6dSJ7cT2vGWvTp/bR5OpwZX?=
 =?iso-8859-1?Q?IveXjHakb6h1Uc3G7KWDJLiNaxjm9VOVMgoSOUlZBgDJeDPCgOA0OCY8Bk?=
 =?iso-8859-1?Q?uZu8MkEmlwc35awiaANz+9uKA1CpKo25+7qdUdJy8VGGM17NMNVB3R3Ere?=
 =?iso-8859-1?Q?Q0seR2WbybfcpRO/GxJpMmXfZKVemHjxi+9itM1zk3kkYvm4AyJEcqHCBI?=
 =?iso-8859-1?Q?QG4dMIzDyvvtZ2UQXEQKjZiqtjCkV3Scql7JL9/jLtIn+bYPOGHJzmrrF6?=
 =?iso-8859-1?Q?IFc70Hjg0JZlDwin5W91jTvcJraqA4M4yUn3TRkLJvgcBKiTNPfUCfULsA?=
 =?iso-8859-1?Q?YfrkjJ05WsHWMNFiVYVie93pnOG90Ugq4ytiln9DHwqeJApEt2fISQoZEM?=
 =?iso-8859-1?Q?vMXIkZ8oP/y/XwkXQGAX0PNfrgUWQOtxiF03OcF/LFA/CizkpX82WPcT7s?=
 =?iso-8859-1?Q?g9ikxg9UI7HWspq3sMtBtVWfPESwz8G4pfrwAFS1r3clBKiq3KdKiDLfZo?=
 =?iso-8859-1?Q?rlwDu5xbozBx5zZKHEXA47M8f5u84wp27LnwGfai1gpRYh5YhwRNnWEWU3?=
 =?iso-8859-1?Q?ZWSBAg1HEOgsaYGWDDI9Bcy6vJk1Gboj2k+EoRnzFrfLyAkIt/SEKrEfTa?=
 =?iso-8859-1?Q?cumtFcUkJ09PdZRTKuvNYN8B84JKnltJbCuWuPQm2Ddddn0C47qufNjWR0?=
 =?iso-8859-1?Q?+mWQAgAFFn1sYVyfdURjvkH2IeY9rz0aU1SkUERn1jnVyK5Fx4dXmlmnkn?=
 =?iso-8859-1?Q?I9W4BAwNuWQYDIwJ6KmDTP6vG61X1VCpDB5DOPev3A1xxYUsI12wMNOcKh?=
 =?iso-8859-1?Q?VJjS37XXJesudgJExey+vRs1tsJhmttvZMJe0n+bUFEMJ/ErME5j8VGJfR?=
 =?iso-8859-1?Q?R51dhse749I1k791wb/e1FGW/YkcotuYm+bzGcxo56EjA8zrfTusxk45Bs?=
 =?iso-8859-1?Q?DCu/QPxdbq5X/btKmj3weoKemwlLvE+YQDi5DfpqzSPqB+5/gGWG6INebj?=
 =?iso-8859-1?Q?7Hj1KOnlUohdo8OblILKutldHsk8PtE8loiFhhRY5PW+1JGRnVl2aKKvE4?=
 =?iso-8859-1?Q?RKVZfJqXq+vX7UA3D/YXj+dhKu7Bxt08On4dPyQ8X4pfSUWGBDBl5/UrHZ?=
 =?iso-8859-1?Q?aJ8AHf4y97sE1UpXgwk1hOFGfimXMiYvuNQbV4PcrAdPDO5MEywK6x45x2?=
 =?iso-8859-1?Q?8PRz3fL7mSFLAH5CngPsMCcg4OOWC+yYyqTREzHfvbrG+z5SzecV7N6Mfc?=
 =?iso-8859-1?Q?KMUDTciR0+ivEOx0QjMFdPD1l9PcyPknGwX00AWrPPJrmrueEYjCDs65As?=
 =?iso-8859-1?Q?vV/KiLUcurZdtJbSSK535cce4DwfForHySy3sJ9//E2nLrXF5H80v8scuX?=
 =?iso-8859-1?Q?v0teC2iwkZcK6yHop0xz55F3TFrOjgsYxvY70fsZWsQWX9UOyBadyyNp/u?=
 =?iso-8859-1?Q?h8d/Y6kAzWG5PCpmTKUu+MOyJrI2LK7g2AM2Uhmq6QGQum82w+Rcpi21BF?=
 =?iso-8859-1?Q?X5arXorW3iZsFwYEfchrCLZS53utaqwLsgp69PHb+7cq1kJkLWDSZukw?=
 =?iso-8859-1?Q?=3D=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: acksys.fr
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DB9PR01MB7354.eurprd01.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 240eb837-ea5d-462b-939f-08d999fad7dd
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Oct 2021 10:08:09.0559
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: f18a6414-d5f3-4b5c-9345-f30c01d87e32
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: bD5p31GT0BxfwSrPr6pLj6CiuOtnZ6AYewzyd+v2C6SnMLKBA03GLRb5J4o2J3K8cZe3arKAY/YOQM6yyXENEA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR01MB6438
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

Please disregard this v3. The spinlock is misplaced in ht.c.
Sorry.

-----Message d'origine-----
De=A0: Jean-Pierre TOSONI <jp.tosoni@acksys.fr>=20
Envoy=E9=A0: jeudi 28 octobre 2021 11:15
=C0=A0: Johannes Berg (johannes@sipsolutions.net) <johannes@sipsolutions.ne=
t>; 'linux-wireless@vger.kernel.org' <linux-wireless@vger.kernel.org>
Objet=A0: [RFC v3] mac80211: fix rx blockack session race condition

When the mac80211 layer is used with ath10k, the following may happen:

a) radio card firmware receives ADDBA-REQ from peer
b) radio sends back ADDBA-RESP to peer and signals the ath10k driver
c) ath10k calls back ieee80211_manage_rx_ba_offl() in mac80211
   to signal rx ba offloading
d) mac80211::agg-rx.c::ieee80211_manage_rx_ba_offl()
  d1) sets a flag: sta->ampdu_mlme.tid_rx_manage_offl
  d2) queues a call to ht.c::ieee80211_ba_session_work()
e) ...scheduler runs...
f) ht.c::ieee80211_ba_session_work() checks the flag, clears it
   and sets up the rx ba session.

During (e), a fast peer may already have sent a BAREQ which is
propagated to rx.c::ieee80211_rx_h_ctrl(). Since the session is not
yet established, mac80211 sends back a DELBA to the peer, which can
hang the BA session.

The phenomenon can be observed between two QCA988X fw 10.2.4 radios,
using a loop of associate/arping from client to AP/disconnect. After
a few thousand loops, arping does not get a response and a sniffer
detects a DELBA action frame from the client, following an ADDBA.

Fix:
1) check the offload flag in addition to the check for a valid
   aggregation session
2) protect the paired checks of (offload flag, valid aggregation session)
   with a spinlock against interference from ieee80211_ba_session_work().

Note 1: there is another dubious DELBA generation in
ieee80211_rx_reorder_ampdu(), where the same kind of fix should fit,
but I did not fix it since I knew no easy way to test.

Note 2: this fix applies to wireless backports from 5.4-rc8.
---
V2: remove debugging code leftovers, sorry for that
V3: use spin_lock_bh instead of a mutex

Index: b/net/mac80211/rx.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -3085,11 +3085,18 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_
=20
 		tid =3D le16_to_cpu(bar_data.control) >> 12;
=20
+		spin_lock_bh(&rx->sta->ampdu_mlme.rx_offl_lock);
 		if (!test_bit(tid, rx->sta->ampdu_mlme.agg_session_valid) &&
-		    !test_and_set_bit(tid, rx->sta->ampdu_mlme.unexpected_agg))
+		    /* back_req is allowed if the fw just received addba */
+		    !test_bit(tid, rx->sta->ampdu_mlme.tid_rx_manage_offl) &&
+		    !test_and_set_bit(tid, rx->sta->ampdu_mlme.unexpected_agg)) {
+			spin_unlock_bh(&rx->sta->ampdu_mlme.rx_offl_lock);
 			ieee80211_send_delba(rx->sdata, rx->sta->sta.addr, tid,
 					     WLAN_BACK_RECIPIENT,
 					     WLAN_REASON_QSTA_REQUIRE_SETUP);
+		} else {
+			spin_unlock_bh(&rx->sta->ampdu_mlme.rx_offl_lock);
+		}
=20
 		tid_agg_rx =3D rcu_dereference(rx->sta->ampdu_mlme.tid_rx[tid]);
 		if (!tid_agg_rx)
Index: b/net/mac80211/ht.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c
@@ -367,11 +367,13 @@ void ieee80211_ba_session_work(struct wo
 							 IEEE80211_MAX_AMPDU_BUF_HT,
 							 false, true, NULL);
=20
+		spin_lock_bh(&sta->ampdu_mlme.rx_offl_lock);
 		if (test_and_clear_bit(tid + IEEE80211_NUM_TIDS,
 				       sta->ampdu_mlme.tid_rx_manage_offl))
 			___ieee80211_stop_rx_ba_session(
 				sta, tid, WLAN_BACK_RECIPIENT,
 				0, false);
+		spin_unlock_bh(&sta->ampdu_mlme.rx_offl_lock);
=20
 		spin_lock_bh(&sta->lock);
=20
Index: b/net/mac80211/sta_info.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -354,6 +354,7 @@ struct sta_info *sta_info_alloc(struct i
=20
 	spin_lock_init(&sta->lock);
 	spin_lock_init(&sta->ps_lock);
+	spin_lock_init(&sta->ampdu_mlme.rx_offl_lock);
 	INIT_WORK(&sta->drv_deliver_wk, sta_deliver_ps_frames);
 	INIT_WORK(&sta->ampdu_mlme.work, ieee80211_ba_session_work);
 	mutex_init(&sta->ampdu_mlme.mtx);
Index: b/net/mac80211/sta_info.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -266,6 +266,7 @@ struct tid_ampdu_rx {
  * @mtx: mutex to protect all TX data (except non-NULL assignments
  *	to tid_tx[idx], which are protected by the sta spinlock)
  *	tid_start_tx is also protected by sta->lock.
+ * @rx_offl_lock: protects transfer from tid_rx_manage_offl to agg_session=
_valid
  * @tid_rx: aggregation info for Rx per TID -- RCU protected
  * @tid_rx_token: dialog tokens for valid aggregation sessions
  * @tid_rx_timer_expired: bitmap indicating on which TIDs the
@@ -287,6 +288,7 @@ struct tid_ampdu_rx {
 struct sta_ampdu_mlme {
 	struct mutex mtx;
 	/* rx */
+	spinlock_t rx_offl_lock;
 	struct tid_ampdu_rx __rcu *tid_rx[IEEE80211_NUM_TIDS];
 	u8 tid_rx_token[IEEE80211_NUM_TIDS];
 	unsigned long tid_rx_timer_expired[BITS_TO_LONGS(IEEE80211_NUM_TIDS)];
--
quilt 0.63