net/ipv4/fib_semantics.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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. Annotate it as well so that the value cannot be torn or
reloaded while the per-nexthop scores are being computed.
Fixes: 195374d89368 ("ipv4: fib: annotate races around nh->nh_saddr_genid and nh->nh_saddr")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
net/ipv4/fib_semantics.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 7a362f2e2c2b..885c6fae5232 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -2204,7 +2204,7 @@ 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) == saddr)
nh_score += 2;
if (hash <= nh_upper_bound)
nh_score++;
--
2.25.1On Fri, Sep 11, 2026 at 12:38 AM Linkui Xiao <xiaolinkui@126.com> 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.
32607a332cfe added the reader after 195374d89368.
> Annotate it as well so that the value cannot be torn or
> reloaded while the per-nexthop scores are being computed.
>
> Fixes: 195374d89368 ("ipv4: fib: annotate races around nh->nh_saddr_genid and nh->nh_saddr")
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> net/ipv4/fib_semantics.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 7a362f2e2c2b..885c6fae5232 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -2204,7 +2204,7 @@ 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) == saddr)
> nh_score += 2;
> if (hash <= nh_upper_bound)
> nh_score++;
> --
> 2.25.1
>
On Mon, Sep 14, 2026 at 12:51 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> On Fri, Sep 11, 2026 at 12:38 AM Linkui Xiao <xiaolinkui@126.com> 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.
>
> 32607a332cfe added the reader after 195374d89368.
>
Indeed, please put in V2:
Fixes: 32607a332cfe ("ipv4: prefer multipath nexthop that matches
source address")
pw-bot: cr
On Mon, Sep 14, 2026 at 12:57 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Mon, Sep 14, 2026 at 12:51 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >
> > On Fri, Sep 11, 2026 at 12:38 AM Linkui Xiao <xiaolinkui@126.com> 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.
> >
> > 32607a332cfe added the reader after 195374d89368.
> >
>
> Indeed, please put in V2:
>
> Fixes: 32607a332cfe ("ipv4: prefer multipath nexthop that matches
> source address")
Adding Willem
It seems that this code also lacks a check against genid?
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 50e96f86ca59ab164f764f5d2185d158c14a4a6a..5a8bbcb65daaada1c3ae96339cabc2e801653dac
100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -2185,6 +2185,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);
@@ -2193,6 +2194,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;
@@ -2205,7 +2207,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++;
On 2026/9/15 04:21, Eric Dumazet wrote:
> On Mon, Sep 14, 2026 at 12:57 PM Eric Dumazet <edumazet@google.com> wrote:
>>
>> On Mon, Sep 14, 2026 at 12:51 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>>>
>>> On Fri, Sep 11, 2026 at 12:38 AM Linkui Xiao <xiaolinkui@126.com> 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.
>>>
>>> 32607a332cfe added the reader after 195374d89368.
>>>
>>
>> Indeed, please put in V2:
>>
>> Fixes: 32607a332cfe ("ipv4: prefer multipath nexthop that matches
>> source address")
>
> Adding Willem
>
> It seems that this code also lacks a check against genid?
Thanks Eric and Kuniyuki for the correction. I'll send a V2 with Fixes:
32607a332cfe.
Kuniyuki, good catch on the missing genid check. nh_saddr is only
meaningful when nh_saddr_genid matches dev_addr_genid, and
fib_select_multipath() currently skips that validation. I'll fold the
genid check into V2 along with the READ_ONCE annotations, unless you'd
prefer to send it as a separate patch. Let me know.
Best regards,
Linkui Xiao
>
> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> index 50e96f86ca59ab164f764f5d2185d158c14a4a6a..5a8bbcb65daaada1c3ae96339cabc2e801653dac
> 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -2185,6 +2185,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);
> @@ -2193,6 +2194,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;
> @@ -2205,7 +2207,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++;
On Mon, Sep 14, 2026 at 7:37 PM Linkui Xiao <xiaolinkui@126.com> wrote:
>
>
>
> On 2026/9/15 04:21, Eric Dumazet wrote:
> > On Mon, Sep 14, 2026 at 12:57 PM Eric Dumazet <edumazet@google.com> wrote:
> >>
> >> On Mon, Sep 14, 2026 at 12:51 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
> >>>
> >>> On Fri, Sep 11, 2026 at 12:38 AM Linkui Xiao <xiaolinkui@126.com> 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.
> >>>
> >>> 32607a332cfe added the reader after 195374d89368.
> >>>
> >>
> >> Indeed, please put in V2:
> >>
> >> Fixes: 32607a332cfe ("ipv4: prefer multipath nexthop that matches
> >> source address")
> >
> > Adding Willem
> >
> > It seems that this code also lacks a check against genid?
> Thanks Eric and Kuniyuki for the correction. I'll send a V2 with Fixes:
> 32607a332cfe.
>
> Kuniyuki, good catch on the missing genid check. nh_saddr is only
> meaningful when nh_saddr_genid matches dev_addr_genid, and
> fib_select_multipath() currently skips that validation. I'll fold the
> genid check into V2 along with the READ_ONCE annotations, unless you'd
> prefer to send it as a separate patch. Let me know.
>
I (Eric) was the one who mentioned the genid thing :)
Send a V2 with both bugs fixed.
Thanks
> Best regards,
> Linkui Xiao
> >
> > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
> > index 50e96f86ca59ab164f764f5d2185d158c14a4a6a..5a8bbcb65daaada1c3ae96339cabc2e801653dac
> > 100644
> > --- a/net/ipv4/fib_semantics.c
> > +++ b/net/ipv4/fib_semantics.c
> > @@ -2185,6 +2185,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);
> > @@ -2193,6 +2194,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;
> > @@ -2205,7 +2207,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++;
>
© 2016 - 2026 Red Hat, Inc.