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

[Hangbin Liu]

On Thu, Mar 26, 2026 at 06:44:43AM +0000, Hangbin Liu wrote:
> 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.

BTW, do we really need to WRITE_ONCE here? What if the `val` are different
on 2 CPUs? This would hide the problem, right?

Thanks
Hangbin