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

[Hangbin Liu]

On Thu, Mar 26, 2026 at 02:23:15PM +0800, Jiayuan Chen wrote:
> 
> On 3/26/26 12:22 PM, Hangbin Liu 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@xxxxxxxxxx>
> > Signed-off-by: Hangbin Liu <liuhangbin@xxxxxxxxx>
> > ---
> >   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);
> >   	}
> 
> 
> [...]
> 
> >   	f6i->fib6_metrics->metrics[metric - 1] = val;
> 
> Suggest using marked accessors to suppress KCSAN warnings:
> 
> struct dst_metrics *m = READ_ONCE(f6i->fib6_metrics);
> WRITE_ONCE(m->metrics[metric - 1], val);

Thanks, I will update this in next version.

Hangbin