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 <[email protected]>
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 <[email protected]>
Thank you,
Simon
