Re: [PATCH net-next 03/15] batman-adv: bat_v: fix bonding candidate selection

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=3

> > diff --git a/net/batman-adv/bat_v.c b/net/batman-adv/bat_v.c
> > index 0c27447cf688..2d3045733db1 100644
> > --- a/net/batman-adv/bat_v.c
> > +++ b/net/batman-adv/bat_v.c
> > @@ -519,8 +519,8 @@ static int batadv_v_neigh_cmp(struct batadv_neigh_node *neigh1,
> >   * @neigh2: second neighbour to compare
> >   * @if_outgoing2: outgoing interface to use for @neigh2
> >   *
> > - * Return: true if the throughput of @neigh2 is at least 3/4 of the
> > - *  @neigh1 throughput
> > + * Return: true if the throughput of @neigh1 is at least 3/4 of the
> > + *  @neigh2 throughput
> >   */
> 
> Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
> This isn't an issue introduced by this patch, the "at least" wording was
> already there before the swap, but does the rewritten kernel-doc for
> batadv_v_neigh_is_sob() still describe the boundary case incorrectly?
> 
> The implementation uses a strict comparison:
> 
> net/batman-adv/bat_v.c:batadv_v_neigh_is_sob() {
> 	threshold = ifinfo2->bat_v.throughput / 4;
> 	threshold = ifinfo2->bat_v.throughput - threshold;
> 
> 	ret = ifinfo1->bat_v.throughput > threshold;
> }
> 
> So a candidate whose throughput is exactly tp2 - tp2/4 returns false, as
> does the tp1 == tp2 == 0 case, while "at least 3/4" reads as inclusive.

Out-of-scope for this patch and adjusted in a later patch.

> 
> The remaining inclusive wording is the pre-existing ops-table description
> in net/batman-adv/types.h, which still documents
> batadv_algo_neigh_ops::is_similar_or_better as "check if neigh1 is equally
> similar or better than neigh2"; that text is untouched by this series.
> Should it be reworded too?

Out-of-scope for this patch.

> 
> > @@ -540,10 +540,10 @@ static bool batadv_v_neigh_is_sob(struct batadv_neigh_node *neigh1,
> >  	if (!ifinfo2)
> >  		goto err_ifinfo2;
> >  
> > -	threshold = ifinfo1->bat_v.throughput / 4;
> > -	threshold = ifinfo1->bat_v.throughput - threshold;
> > +	threshold = ifinfo2->bat_v.throughput / 4;
> > +	threshold = ifinfo2->bat_v.throughput - threshold;
> >  
> > -	ret = ifinfo2->bat_v.throughput > threshold;
> > +	ret = ifinfo1->bat_v.throughput > threshold;
> 
> Discovered by: gpt-5-6-sol, opus-5 · Missed by: sashiko-gemini
> Should this carry a Fixes: tag and a stable indication?
[...]
> That means every released kernel containing 9786906022eb keeps accepting
> bonding candidates with arbitrarily low throughput whenever
> bat_priv->bonding is enabled.  Without a Fixes: tag the change is not
> visible to stable/AUTOSEL tooling.


I am not allowed to add Fixes: lines for patches targeting net-next. See
https://lore.kernel.org/all/7d73cd74-b040-4a3d-9d78-4ad8fc0a01f4@redhat.com/

Regards,
Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]