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

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

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

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()
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@kylinos.cn>
---
V1: https://lore.kernel.org/all/CANn89iJhFL2LNQCXTANQAx8B1GVdGTJgrDwPTZOjbH8k1ygtsg@mail.gmail.com/

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++;
-- 
2.25.1

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

[Ido Schimmel]

On Tue, Sep 15, 2026 at 03:15:03PM +0800, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> 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@kylinos.cn>
> ---
> V1: https://lore.kernel.org/all/CANn89iJhFL2LNQCXTANQAx8B1GVdGTJgrDwPTZOjbH8k1ygtsg@mail.gmail.com/
> 
> 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++;

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@kylinos.cn>
>>
>> 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@kylinos.cn>
>> ---
>> V1: https://lore.kernel.org/all/CANn89iJhFL2LNQCXTANQAx8B1GVdGTJgrDwPTZOjbH8k1ygtsg@mail.gmail.com/
>>
>> 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++;

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] In fib_select_multipath() (net/ipv4/fib_semantics.c), the newly
  added `READ_ONCE(nexthop_nh->nh_saddr_genid) == genid` test treats a…

--

ipv4: fib: fix data-race and stale genid check around nh->nh_saddr

This adds READ_ONCE() to the two lockless reads of nh->nh_saddr and
nh->nh_saddr_genid in fib_select_multipath(), and additionally requires the
cached nh_saddr_genid to equal a snapshot of net->ipv4.dev_addr_genid before
the source-address match can add its scoring bonus.  The stated intent is to
mirror fib_result_prefsrc().

> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 7a362f2e2c2bd..e2bfc11aea09c 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -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;

[Severity: High]
Does this genid check turn a cache miss into a permanent disqualification,
so that the source-address preference from 32607a332cfe stops working after
any address event in the netns?

The function the commit message says is being mirrored treats a genid
mismatch as a miss and repopulates the cache:

net/ipv4/fib_semantics.c:fib_result_prefsrc() {
	...
		if (READ_ONCE(nh->nh_saddr_genid) ==
		    atomic_read(&net->ipv4.dev_addr_genid))
			return READ_ONCE(nh->nh_saddr);
	}

	return fib_info_update_nhc_saddr(net, nhc, res->fi->fib_scope);
}

fib_select_multipath() has no such fallback: on mismatch the nexthop simply
loses nh_score += 2, and nothing recomputes nh_saddr for it.

nh_saddr_genid is only stamped in fib_create_info() (once per fib_info at
route creation, for all nexthops), at nexthop object creation in
net/ipv4/nexthop.c, and by fib_result_prefsrc() for res->nhc alone.
Meanwhile dev_addr_genid is a per-netns counter bumped for any IPv4 address
event on any device:

net/ipv4/fib_frontend.c:fib_inetaddr_event() {
	...
		atomic_inc(&net->ipv4.dev_addr_genid);
		rt_cache_flush(net);

The same pattern appears for NETDEV_DOWN and in fib_netdev_event().  So after
one unrelated ip addr add/del, DHCP renew or NETDEV_UP, every fib_nh in the
netns has a stale nh_saddr_genid, and the accompanying rt_cache_flush()
forces established sockets into fresh lookups that already carry
fl4->saddr != 0.

Is there anything on that path that would refresh the cache?  The only lazy
refresher reachable from here is gated on the opposite condition:

net/ipv4/fib_semantics.c:fib_select_path() {
	...
check_saddr:
	if (!fl4->saddr) {
		...
			fl4->saddr = fib_result_prefsrc(net, res);

That block is skipped exactly when saddr is non-zero, which is the only case
the new check governs, and even when it does run it refreshes res->nhc only,
not the sibling nexthops walked by the scoring loop.  The other caller,
ip_mkroute_input() in net/ipv4/route.c, passes fl4 == NULL, so saddr is 0
there and the check has no effect on forwarding.

With that in place, for every output lookup that already has a source
address (socket bound before connect(), UDP or raw sendmsg with a fixed
source, and the post-flush re-lookups), does the +2 bonus become
unreachable, making nh_score == 3 impossible and reducing selection to plain
hash ECMP?  That would let a flow sourced from veth0's address egress via
veth1, which is the behaviour 32607a332cfe was written to avoid.

Would calling fib_info_update_nhc_saddr() on mismatch, or restricting this
patch to the READ_ONCE() annotations that the KCSAN report actually
justifies, be preferable here?  As it stands the Fixes: tag would carry this
behavioural change into stable.

[Severity: Low]
On the commit message wording:

    fib_info_update_nhc_saddr() stores a new value from another CPU as soon
    as the preferred source address of the egress device changes.

Is that accurate?  fib_info_update_nhc_saddr() is only called on demand
(from fib_result_prefsrc() on a genid mismatch, and at route creation), not
from the address-change notifier itself, which only bumps dev_addr_genid.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915071503.465875-1-xiaolinkui%40126.com