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

[Sven Eckelmann]

On Thursday, 14 May 2026 23:00:45 CEST sashiko-...@kernel.org 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

signature.asc
Description: This is a digitally signed message part.