Re: [PATCH net v2] ipv4: fib: fix data-race and stale genid check around nh->nh_saddr

[Linkui Xiao]

Hi Ido,

Thanks a lot for the review. Both points are valid, and I'll address
them in v3.

1. Commit reference

You're right — the rule that nh_saddr is only meaningful when
nh_saddr_genid matches dev_addr_genid was established by commit
436c3b66ec98 ("ipv4: Invalidate nexthop cache nh_saddr more
correctly."), not by 195374d89368. The latter only added the
READ_ONCE()/WRITE_ONCE() annotations. I'll correct the commit message
accordingly.

2. Refresh on genid mismatch instead of skipping

I agree that skipping the comparison on a genid mismatch would
silently downgrade ECMP selection to hash-based for as long as the
cached genid remains stale, which defeats the purpose of the
source-address matching introduced by 32607a332cfe.

I'll adopt your fib_nh_saddr() helper in v3:

    static __be32 fib_nh_saddr(struct net *net, const struct fib_info *fi,
                               struct fib_nh *nh, int genid)
    {
        if (READ_ONCE(nh->nh_saddr_genid) == genid)
            return READ_ONCE(nh->nh_saddr);

        return fib_info_update_nhc_saddr(net, &nh->nh_common,
                                         fi->fib_scope);
    }

and simplify the check to:

    if (saddr && fib_nh_saddr(net, fi, nexthop_nh, genid) == saddr)
        nh_score += 2;

This keeps the fast path free of extra work when the genid matches,
while refreshing the cached source address on a mismatch, mirroring
fib_result_prefsrc().

I'll send v3 shortly. Thanks again for the guidance.

Best regards,
Linkui Xiao

On 2026/9/16 01:53, Ido Schimmel wrote:
> On Tue, Sep 15, 2026 at 03:15:03PM +0800, Linkui Xiao wrote:
> > From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> >
> > fib_select_multipath() compares nexthop_nh->nh_saddr against the flow
> > source address with no lock held, while fib_info_update_nhc_saddr()
> > stores a new value from another CPU as soon as the preferred source
> > address of the egress device changes.
> >
> > Commit 195374d89368 ("ipv4: fib: annotate races around nh->nh_saddr_genid
> > and nh->nh_saddr") added WRITE_ONCE() on the store side and READ_ONCE()
> > in fib_result_prefsrc() after syzbot reported
> >
> > 	BUG: KCSAN: data-race in fib_select_path / fib_select_path
> >
> > but it only covered that reader. fib_select_multipath(), reached from
> > fib_select_path(), is a second lockless reader of nh->nh_saddr and was
> > left bare.
> >
> > Moreover, nh_saddr is only meaningful when nh_saddr_genid matches
> > dev_addr_genid, as established by 195374d89368. fib_select_multipath()
>
> It's established by commit 436c3b66ec98 ("ipv4: Invalidate nexthop cache
> nh_saddr more correctly.")
>
> > skips that validation, so it can score a nexthop using a stale source
> > address and skew the ECMP selection.
> >
> > Annotate both reads with READ_ONCE() and add the missing genid check,
> > mirroring fib_result_prefsrc().
> >
> > Fixes: 32607a332cfe ("ipv4: prefer multipath nexthop that matches source address")
> > Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> > ---
> > V1: https://lore.kernel.org/all/CANn89iJhFL2LNQCXTANQAx8B1GVdGTJgrDwPTZOjbH8k1ygtsg@xxxxxxxxxxxxxx/
> >
> > V2:
> > - point Fixes: at 32607a332cfe, which introduced the bare reader
> > - add the missing genid check as suggested by Eric Dumazet
> >
> >   net/ipv4/fib_semantics.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> > index 7a362f2e2c2b..e2bfc11aea09 100644
> > --- a/net/ipv4/fib_semantics.c
> > +++ b/net/ipv4/fib_semantics.c
> > @@ -2184,6 +2184,7 @@ void fib_select_multipath(struct fib_result *res, int hash,
> >   	bool use_neigh;
> >   	int score = -1;
> >   	__be32 saddr;
> > +	int genid;
> >   
> >   	if (unlikely(res->fi->nh)) {
> >   		nexthop_path_fib_result(res, hash);
> > @@ -2192,6 +2193,7 @@ void fib_select_multipath(struct fib_result *res, int hash,
> >   
> >   	use_neigh = READ_ONCE(net->ipv4.sysctl_fib_multipath_use_neigh);
> >   	saddr = fl4 ? fl4->saddr : 0;
> > +	genid = saddr ? atomic_read(&net->ipv4.dev_addr_genid) : 0;
> >   
> >   	change_nexthops(fi) {
> >   		int nh_upper_bound, nh_score = 0;
> > @@ -2204,7 +2206,9 @@ void fib_select_multipath(struct fib_result *res, int hash,
> >   		    (use_neigh && !fib_good_nh(nexthop_nh)))
> >   			continue;
> >   
> > -		if (saddr && nexthop_nh->nh_saddr == saddr)
> > +		if (saddr &&
> > +		    READ_ONCE(nexthop_nh->nh_saddr_genid) == genid &&
> > +		    READ_ONCE(nexthop_nh->nh_saddr) == saddr)
> >   			nh_score += 2;
> >   		if (hash <= nh_upper_bound)
> >   			nh_score++;
>
> The cached generation ID can remain stale for a long time and during
> this time we will silently downgrade the nexthop selection to be hash
> based. We need to refresh it in case there is a mismatch. Something
> like:
>
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index e2bfc11aea09..5c9021ea3a79 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -2176,6 +2176,15 @@ static bool fib_good_nh(const struct fib_nh *nh)
>   	return !!(state & NUD_VALID);
>   }
>   
> +static __be32 fib_nh_saddr(struct net *net, const struct fib_info *fi,
> +			   struct fib_nh *nh, int genid)
> +{
> +	if (READ_ONCE(nh->nh_saddr_genid) == genid)
> +		return READ_ONCE(nh->nh_saddr);
> +
> +	return fib_info_update_nhc_saddr(net, &nh->nh_common, fi->fib_scope);
> +}
> +
>   void fib_select_multipath(struct fib_result *res, int hash,
>   			  const struct flowi4 *fl4)
>   {
> @@ -2206,9 +2215,7 @@ void fib_select_multipath(struct fib_result *res, int hash,
>   		    (use_neigh && !fib_good_nh(nexthop_nh)))
>   			continue;
>   
> -		if (saddr &&
> -		    READ_ONCE(nexthop_nh->nh_saddr_genid) == genid &&
> -		    READ_ONCE(nexthop_nh->nh_saddr) == saddr)
> +		if (saddr && fib_nh_saddr(net, fi, nexthop_nh, genid) == saddr)
>   			nh_score += 2;
>   		if (hash <= nh_upper_bound)
>   			nh_score++;