Re: [PATCH net v2 1/2] ipv6: serialize address publication with device teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch leaves a teardown race open that leaks a reference and
  has the same symptom it sets out to fix.

--

ipv6: serialize address publication with device teardown

This patch takes idev->lock around the write of idev->dead in
addrconf_ifdown(). In ipv6_add_addr() it holds idev->lock across both the
hash and the device-list publication, and rechecks dead and disable_ipv6
before publishing. The goal is to stop an address being published on a
dead idev, which leaks references and makes device deletion hang.

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index c90ee6dd7446c..426739abb0744 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1168,14 +1168,20 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
>  
>  	rcu_read_lock();
>  
> -	err = ipv6_add_addr_hash(idev->dev, ifa);
> +	write_lock_bh(&idev->lock);
> +
> +	if (idev->dead)
> +		err = -ENODEV;
> +	else if (READ_ONCE(idev->cnf.disable_ipv6))
> +		err = -EACCES;
> +	else
> +		err = ipv6_add_addr_hash(idev->dev, ifa);
>  	if (err < 0) {
> +		write_unlock_bh(&idev->lock);
>  		rcu_read_unlock();
>  		goto out;
>  	}
>  
> -	write_lock_bh(&idev->lock);
> -
>  	/* Add to inet6_dev unicast addr list. */
>  	ipv6_link_dev_addr(idev, ifa);
>  

[Severity: High]
Can the same kind of leak still happen for temporary addresses after this
change?

The new critical section ends in ipv6_add_addr() once the address is on the
hash, addr_list and tempaddr_list:

net/ipv6/addrconf.c:ipv6_add_addr() {
    ...
	in6_ifa_hold(ifa);
	write_unlock_bh(&idev->lock);

	rcu_read_unlock();

	inet6addr_notifier_call_chain(NETDEV_UP, ifa);
    ...
}

The ifp reference held by ipv6_create_tempaddr() is handed to the new
address only after ipv6_add_addr() returns:

net/ipv6/addrconf.c:ipv6_create_tempaddr() {
    ...
	ift = ipv6_add_addr(idev, &cfg, block, NULL);
    ...
	spin_lock_bh(&ift->lock);
	ift->ifpub = ifp;
	ift->cstamp = now;
    ...
	addrconf_dad_start(ift);
	in6_ifa_put(ift);
    ...
}

On the RA path, ipv6_create_tempaddr() runs in softirq without RTNL:

ndisc_router_discovery()
  addrconf_prefix_rcv()
    addrconf_prefix_rcv_add_addr()
      manage_tempaddrs()
        ipv6_create_tempaddr(ifp, false)

Meanwhile, addrconf_notify() handles NETDEV_CHANGEMTU with
dev->mtu < IPV6_MIN_MTU by calling addrconf_ifdown(dev, true). The device
is still up and keeps receiving RAs. This is the same trigger the commit
message cites.

Suppose addrconf_ifdown() takes idev->lock right after ipv6_add_addr()
drops it. It sets dead, and the new recheck can't help because the address
is already published. Step 3 then sees ift on tempaddr_list with ifpub
still NULL, so it has nothing to drop:

net/ipv6/addrconf.c:addrconf_ifdown() {
    ...
		if (ifa->ifpub) {
			in6_ifa_put(ifa->ifpub);
			ifa->ifpub = NULL;
		}
    ...
}

The rest of the loop marks ift DEAD, unlinks it and puts it. Then
ipv6_create_tempaddr() stores ifp in ift->ifpub. addrconf_dad_start() does
nothing because ift is DEAD, and in6_ifa_put(ift) drops the last reference.

inet6_ifa_finish_destroy() never releases ifpub. Only ipv6_del_addr() and
Step 3 of addrconf_ifdown() do, and only while the address is still on
tempaddr_list.

If so, the public ifp stays pinned. That pins its idev, and the idev pins
the net_device through dev_tracker. Later device deletion would then wait
forever in netdev_wait_allrefs_any(), which is the symptom described in the
commit message.

Would it make sense to set ifpub inside the idev->lock publication section,
for example by passing it into ipv6_add_addr()? Another option would be a
teardown or destroy path that releases an ifpub attached this late.

> @@ -3897,7 +3903,9 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
>  	 *	   Do not dev_put!
>  	 */
>  	if (unregister) {
> +		write_lock_bh(&idev->lock);
>  		WRITE_ONCE(idev->dead, 1);
> +		write_unlock_bh(&idev->lock);
>  
>  		/* protected by rtnl_lock */
>  		RCU_INIT_POINTER(dev->ip6_ptr, NULL);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004183639.3773498-1-4ncienth%40gmail.com