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

[Eric Dumazet <edumazet@google.com>]

On Wed, Mar 25, 2026 at 9:22 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> fib6_metric_set() may be called concurrently from softirq context without
> holding the FIB table lock. A typical path is:
>
>   ndisc_router_discovery()
>     spin_unlock_bh(&table->tb6_lock)        <- lock released
>     fib6_metric_set(rt, RTAX_HOPLIMIT, ...) <- lockless call
>
> When two CPUs process Router Advertisement packets for the same router
> simultaneously, they can both arrive at fib6_metric_set() with the same
> fib6_info pointer whose fib6_metrics still points to dst_default_metrics.
>
>   if (f6i->fib6_metrics == &dst_default_metrics) {   /* both CPUs: true */
>       struct dst_metrics *p = kzalloc_obj(*p, GFP_ATOMIC);
>       refcount_set(&p->refcnt, 1);
>       f6i->fib6_metrics = p;   /* CPU1 overwrites CPU0's p -> p0 leaked */
>   }
>
> The dst_metrics allocated by the losing CPU has refcnt=1 but no pointer
> to it anywhere in memory, producing a kmemleak report:
>
>   unreferenced object 0xff1100025aca1400 (size 96):
>     comm "softirq", pid 0, jiffies 4299271239
>     backtrace:
>       kmalloc_trace+0x28a/0x380
>       fib6_metric_set+0xcd/0x180
>       ndisc_router_discovery+0x12dc/0x24b0
>       icmpv6_rcv+0xc16/0x1360
>
> Fix this by replacing the plain pointer store with cmpxchg() and free
> the allocation safely when competition failed.
>
> Fixes: d4ead6b34b67 ("net/ipv6: move metrics from dst to rt6_info")
> Reported-by: Fei Liu <feliu@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/ipv6/ip6_fib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> 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.


>         f6i->fib6_metrics->metrics[metric - 1] = val;
>
> ---
> base-commit: c4ea7d8907cf72b259bf70bd8c2e791e1c4ff70f
> change-id: 20260326-b4-fib6_metric_set-kmemleak-7aa51978284a
>
> Best regards,
> --
> Hangbin Liu <liuhangbin@gmail.com>
>