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

[Ido Schimmel]

On Mon, Oct 05, 2026 at 06:39:59PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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.

Yes, we need to pass the public address to ipv6_add_addr() so that we
only publish a fully initialized temporary address in the per-idev list
and the per-netns hashtable. Something like the diff below. It should be
another patch in this series.

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index e6764245995f..8e608bb9234a 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -87,6 +87,7 @@ struct ifa6_config {
 	u32			preferred_lft;
 	u32			valid_lft;
 	u16			scope;
+	struct inet6_ifaddr	*ifpub;
 };
 
 enum addr_type_t {
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index c90ee6dd7446..d24773f76805 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1159,6 +1159,7 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
 	ifa->tokenized = false;
 
 	ifa->rt = f6i;
+	ifa->ifpub = cfg->ifpub;
 
 	ifa->idev = idev;
 	in6_dev_hold(idev);
@@ -1487,6 +1488,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 
 	cfg.pfx = &addr;
 	cfg.scope = ipv6_addr_scope(cfg.pfx);
+	cfg.ifpub = ifp;
 
 	ift = ipv6_add_addr(idev, &cfg, block, NULL);
 	if (IS_ERR(ift)) {
@@ -1498,7 +1500,6 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 	}
 
 	spin_lock_bh(&ift->lock);
-	ift->ifpub = ifp;
 	ift->cstamp = now;
 	ift->tstamp = tmp_tstamp;
 	spin_unlock_bh(&ift->lock);