[PATCH RFC batadv] batman-adv: bla: avoid double decrement of bla.num_requests

[PATCH RFC batadv] batman-adv: bla: avoid double decrement of bla.num_requests

[Sven Eckelmann]

The bla.num_requests is increased when a no request_sent was in progress.
And it is decremented in various places (announcemnt was received, backbone
is purged, periodic work). But the check if the request_sent is actually
set to a specific state and the atomic_dec/_inc are not safe because they
are not atomic (TOCTOU) and multiple such code portions can run
concurrently.

This can be avoided by atomically replacing the request_sent state while
checking the previous value. Only if the previous value was not actually in
the correct state then the atomic_dec/_inc can be performed on
bla.num_requests.

Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Please consider this as a note what might have to be done. This is
completely untested.

Especially think about batadv_bla_purge_backbone_gw - the old code confused
me a lot and the new one might therefore be wrong.

See
https://sashiko.dev/#/patchset/20260510-bla-cancel-work-v1-1-3654903495ef@narfation.org?part=1
---
 net/batman-adv/bridge_loop_avoidance.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/batman-adv/bridge_loop_avoidance.c b/net/batman-adv/bridge_loop_avoidance.c
index cec11f12..cc9ed2fb 100644
--- a/net/batman-adv/bridge_loop_avoidance.c
+++ b/net/batman-adv/bridge_loop_avoidance.c
@@ -544,9 +544,10 @@ batadv_bla_get_backbone_gw(struct batadv_priv *bat_priv, const u8 *orig,
 		batadv_bla_send_announce(bat_priv, entry);
 
 		/* this will be decreased in the worker thread */
-		atomic_inc(&entry->request_sent);
-		atomic_set(&entry->wait_periods, BATADV_BLA_WAIT_PERIODS);
-		atomic_inc(&bat_priv->bla.num_requests);
+		if (atomic_xchg(&entry->request_sent, 1) == 0) {
+			atomic_set(&entry->wait_periods, BATADV_BLA_WAIT_PERIODS);
+			atomic_inc(&bat_priv->bla.num_requests);
+		}
 	}
 
 	return entry;
@@ -649,10 +650,8 @@ static void batadv_bla_send_request(struct batadv_bla_backbone_gw *backbone_gw)
 			      backbone_gw->vid, BATADV_CLAIM_TYPE_REQUEST);
 
 	/* no local broadcasts should be sent or received, for now. */
-	if (!atomic_read(&backbone_gw->request_sent)) {
+	if (atomic_xchg(&backbone_gw->request_sent, 1) == 0)
 		atomic_inc(&backbone_gw->bat_priv->bla.num_requests);
-		atomic_set(&backbone_gw->request_sent, 1);
-	}
 }
 
 /**
@@ -873,10 +872,8 @@ static bool batadv_handle_announce(struct batadv_priv *bat_priv, u8 *an_addr,
 		/* if we have sent a request and the crc was OK,
 		 * we can allow traffic again.
 		 */
-		if (atomic_read(&backbone_gw->request_sent)) {
+		if (atomic_xchg(&backbone_gw->request_sent, 0) != 0)
 			atomic_dec(&backbone_gw->bat_priv->bla.num_requests);
-			atomic_set(&backbone_gw->request_sent, 0);
-		}
 	}
 
 	batadv_backbone_gw_put(backbone_gw);
@@ -1249,7 +1246,7 @@ static void batadv_bla_purge_backbone_gw(struct batadv_priv *bat_priv, int now)
 
 purge_now:
 			/* don't wait for the pending request anymore */
-			if (atomic_read(&backbone_gw->request_sent))
+			if (atomic_xchg(&backbone_gw->request_sent, 0) != 0)
 				atomic_dec(&bat_priv->bla.num_requests);
 
 			batadv_bla_del_backbone_claims(backbone_gw);
@@ -1507,8 +1504,10 @@ static void batadv_bla_periodic_work(struct work_struct *work)
 			if (!atomic_dec_and_test(&backbone_gw->wait_periods))
 				continue;
 
+			if (atomic_xchg(&backbone_gw->request_sent, 0) == 0)
+				continue;
+
 			atomic_dec(&backbone_gw->bat_priv->bla.num_requests);
-			atomic_set(&backbone_gw->request_sent, 0);
 		}
 		rcu_read_unlock();
 	}

---
base-commit: 57c6af492c1948145db835bb3ea2980472558298
change-id: 20260512-bla-atomic-request_sent-feeb9f1a08c5

Best regards,
--  
Sven Eckelmann <sven@narfation.org>

Re: [PATCH RFC batadv] batman-adv: bla: avoid double decrement of bla.num_requests

[Simon Wunderlich]

On Tuesday, May 12, 2026 9:28:52 AM Central European Summer Time Sven 
Eckelmann wrote:
> The bla.num_requests is increased when a no request_sent was in progress.
> And it is decremented in various places (announcemnt was received, backbone
> is purged, periodic work). But the check if the request_sent is actually
> set to a specific state and the atomic_dec/_inc are not safe because they
> are not atomic (TOCTOU) and multiple such code portions can run
> concurrently.
> 
> This can be avoided by atomically replacing the request_sent state while
> checking the previous value. Only if the previous value was not actually in
> the correct state then the atomic_dec/_inc can be performed on
> bla.num_requests.
> 
> Fixes: a9ce0dc43e2c ("batman-adv: add basic bridge loop avoidance code")
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Looks good to me. Please double check the first sentence of the commit message 
("... when a no request_sent was in progress").

> ---
> Please consider this as a note what might have to be done. This is
> completely untested.
> 
> Especially think about batadv_bla_purge_backbone_gw - the old code confused
> me a lot and the new one might therefore be wrong.

[...]

> @@ -1249,7 +1246,7 @@ static void batadv_bla_purge_backbone_gw(struct
> batadv_priv *bat_priv, int now)
> 
>  purge_now:
>  			/* don't wait for the pending request 
anymore */
> -			if (atomic_read(&backbone_gw-
>request_sent))
> +			if (atomic_xchg(&backbone_gw-
>request_sent, 0) != 0)
>  				atomic_dec(&bat_priv-
>bla.num_requests);

I think this change is correct. The idea was to supress broadcasts while 
requests are pending, because the claim database may be out of sync and loops 
may happen in this time otherwise. Multiple requests could be in flight to 
different backbone gateways, so this num_request counter was used to track the 
number of requests. If a backbone_gw is purged, the associated request is no 
longer interesting, and the counter can be decreased.

backbone gateways are removed on tear down or on timeout, so this is the right 
place to decrease the counter.

Your change is consistent with the rest of the patch to decrease it with a 
"real" atomic operation.

I've not tested this patch, but feel free to add:

Reviewed-by: Simon Wunderlich <sw@simonwunderlich.de>

Thank you,
      Simon