Re: [PATCH net] ipv6: fix data race in fib6_metric_set() using cmpxchg

[Eric Dumazet]

On Thu, Mar 26, 2026 at 6:44 AM Jiayuan Chen <jiayuan.chen@xxxxxxxxx> wrote:
>
>
> On 3/26/26 9:13 PM, Hangbin Liu wrote:
> > On Thu, Mar 26, 2026 at 05:05:57AM -0700, Eric Dumazet wrote:
> >>> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> >>> index dd26657b6a4a..64de761f40d5 100644
> >>> --- a/net/ipv6/ip6_fib.c
> >>> +++ b/net/ipv6/ip6_fib.c
> >>> @@ -730,14 +730,16 @@ void fib6_metric_set(struct fib6_info *f6i, int metric, u32 val)
> >>>          if (!f6i)
> >>>                  return;
> >>>
> >>> -       if (f6i->fib6_metrics == &dst_default_metrics) {
> >>> +       if (READ_ONCE(f6i->fib6_metrics) == &dst_default_metrics) {
> >>> +               struct dst_metrics *dflt = (struct dst_metrics *)&dst_default_metrics;
> >>>                  struct dst_metrics *p = kzalloc_obj(*p, GFP_ATOMIC);
> >>>
> >>>                  if (!p)
> >>>                          return;
> >>>
> >>>                  refcount_set(&p->refcnt, 1);
> >>> -               f6i->fib6_metrics = p;
> >>> +               if (cmpxchg(&f6i->fib6_metrics, dflt, p) != dflt)
> >>> +                       kfree(p);
> >>>          }
> >>>
> >> The following line should happen before the cmpxchg(),
> >>   ->metrics[X] accesses also need READ_ONCE()/WRITE_ONCE() annotations.
> > Hi Eric,
> >
> > Jiayuan also suggested to using READ_ONCE()/WRITE_ONCE() for metrics[X]
> > accesses. But I don't get why this line should happen before the cmpxchg(),
> > Would you please help explain?
>
>
> I think what Eric means is something like this:
>
>
> ...
>          struct dst_metrics *p = kzalloc_obj(*p, GFP_ATOMIC);
>
>          if (!p)
>              return;
>
>          p->metrics[metric - 1] = val;
>          refcount_set(&p->refcnt, 1);
>          if (cmpxchg(&f6i->fib6_metrics, dflt, p) != dflt)
>              kfree(p);
>          else
>              return;
>      }
> }
>
> m = READ_ONCE(f6i->fib6_metrics);
> WRITE_ONCE(m->metrics[metric - 1], val);
>
>
> Since p is private data before being published via cmpxchg(), we can
> safely initialize its metrics beforehand. This way we don't need to
> worry about concurrent access to f6i->fib6_metrics->metrics[] during
> initialization. Right?

Yes.  Think about RCU (Read Copy Update) rules.

We allocate an object, and populate it, then make sure changes are
committed, before publishing the new pointer.

Othewise an other cpu could read a 0 metric, while we wanted something else.