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

[Jiayuan Chen <jiayuan.chen@linux.dev>]

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?