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

On Thursday, May 14, 2026 11:11:04 PM Central European Summer Time Sven 
Eckelmann wrote:
> 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.

This part was/is only relevant for the own gateway (we check for the own 
primary address in the beginning of this function).

The wait_period is only set when creating the own backbone_gw, and then only 
decremented in the worker. The idea is that we wait for ~30 seconds or so 
after batman-adv initially joined the network before we allow broadcasts. 
Sometimes switches or the Linux bridge need some learning time of their own 
and would not forward our BLA announcements either, so we want to give them 
this time to get to know us.

> 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.

Yes, I think this is the mistake on Sashikos side (saw that in another review 
previously): 

 * we don't reset waiting times again. It's only set during creation
 * we don't send requests to ourselves, therefore request_sent shouldn't be 
touched by anything else. So basically the request_sent is serving a special 
case purpose for own gatways.

Cheers,
        Simon