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

On Thursday, 14 May 2026 23:00:45 CEST [email protected] wrote:
> [ ... ]
> 
> > @@ -1517,14 +1529,21 @@ static void batadv_bla_periodic_work(struct work_struct *work)
> >  			 * some grace time.
> >  			 */
> >  
> > -			if (atomic_read(&backbone_gw->request_sent) == 0)
> > -				continue;
> > +			spin_lock_bh(&bat_priv->bla.num_requests_lock);
> > +			if (backbone_gw->state != BATADV_BLA_BACKBONE_GW_UNSYNCED)
> > +				goto unlock_next;
> >  
> > -			if (!atomic_dec_and_test(&backbone_gw->wait_periods))
> > -				continue;
> > +			if (backbone_gw->wait_periods > 0)
> > +				backbone_gw->wait_periods--;
> > +
> > +			if (backbone_gw->wait_periods > 0)
> > +				goto unlock_next;
> >  
> > +			backbone_gw->state = BATADV_BLA_BACKBONE_GW_SYNCED;
> >  			atomic_dec(&backbone_gw->bat_priv->bla.num_requests);
> > -			atomic_set(&backbone_gw->request_sent, 0);
> > +
> > +unlock_next:
> > +			spin_unlock_bh(&bat_priv->bla.num_requests_lock);
> 
> Will this prematurely reset the bridge loop avoidance sync state when a request
> is sent?
> 
> In the previous implementation, batadv_bla_send_request() set request_sent to 1
> but left wait_periods at 0. When batadv_bla_periodic_work() ran,
> atomic_dec_and_test() on wait_periods would decrement it to -1 (returning
> false), which correctly kept the gateway in the out-of-sync state until a valid
> ANNOUNCE frame arrived.
> 
> With wait_periods now being a u8, if batadv_bla_send_request() sets the state
> to BATADV_BLA_BACKBONE_GW_UNSYNCED but leaves wait_periods at 0, the next run
> of batadv_bla_periodic_work() will evaluate if (backbone_gw->wait_periods > 0)
> as false.
> 
> Because the code falls through when wait_periods is 0, it appears to
> unconditionally transition back to BATADV_BLA_BACKBONE_GW_SYNCED.
> 
> Could this spontaneously drop the out-of-sync block without receiving a valid
> ANNOUNCE frame, potentially allowing broadcast loops?

@Simon, can you double check this? My understanding is that this code is only 
relevant for own_backbone (see batadv_bla_get_backbone_gw()). And for
own_backbone, we are always setting wait_periods to BATADV_BLA_WAIT_PERIODS.

An underflow was never expected and this code should not run for 
batadv_handle_announce() with CRC error (see batadv_bla_send_request()). Only
in this codepath, we don't set wait_periods to anything.

Regards,
	Sven

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQS81G/PswftH/OW8cVND3cr0xT1ywUCagY6aAAKCRBND3cr0xT1
y9QHAP9H81QkqhvptMrvk5FQMi1BFFa5NetSBjry3A8yB9+bfwEAn4xPCV7MOOB8
PXVudWO0UrI60g97tiw3tSooZ3ibRwo=
=x+NR
-----END PGP SIGNATURE-----