Re: [PATCH] ipv4: fib: annotate data-race around nh->nh_saddr

[Linkui Xiao]

On 2026/9/15 04:21, Eric Dumazet wrote:
> On Mon, Sep 14, 2026 at 12:57 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > On Mon, Sep 14, 2026 at 12:51 PM Kuniyuki Iwashima <kuniyu@xxxxxxxxxx> wrote:
> > > On Fri, Sep 11, 2026 at 12:38 AM Linkui Xiao <xiaolinkui@xxxxxxx> 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.
> > >
> > > 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++;