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

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

On 3/26/26 3:13 PM, Hangbin Liu wrote:
> 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@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);
>>>>    	}
>>>
>>> [...]
>>>
>>>>    	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
If concurrent writes to the same memory by two CPUs are considered a bug,
the solution is prevention, not detection by KCSAN. Furthermore, WRITE_ONCE
also helps suppress warnings from read-write races. For example, commit
6e84fc395e90 demonstrates this.