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

[Hangbin Liu <liuhangbin@gmail.com>]

On Thu, Mar 26, 2026 at 07:01:36AM -0700, Eric Dumazet wrote:
> On Thu, Mar 26, 2026 at 6:44 AM Jiayuan Chen <jiayuan.chen@linux.dev> 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.

Ah, got it. Thanks for the explanation.

Regards
Hangbin