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

[Jiayuan Chen]

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?