Re: [PATCH net v3 1/3] ipv6: serialize address publication with device teardown

[Ido Schimmel]

On Sat, Oct 10, 2026 at 05:43:14AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. The
> recheck above does not cover the window after ipv6_add_addr() returns to
> addrconf_dad_failure().
> 
> addrconf_dad_failure() runs from ndisc_recv_ns()/ndisc_recv_na() in
> softirq without RTNL. For IFA_F_STABLE_PRIVACY addresses it does:
> 
> net/ipv6/addrconf.c:addrconf_dad_failure() {
>     ...
> 		ifp2 = ipv6_add_addr(idev, &cfg, false, NULL);
> 		if (IS_ERR(ifp2))
> 			goto lock_errdad;
> 
> 		spin_lock_bh(&ifp2->lock);
> 		ifp2->stable_privacy_retry = retries;
> 		ifp2->state = INET6_IFADDR_STATE_PREDAD;
> 		spin_unlock_bh(&ifp2->lock);
> 
> 		addrconf_mod_dad_work(ifp2, net->ipv6.sysctl.idgen_delay);
>     ...
> }
> 
> addrconf_ifdown() can run on another CPU because of NETDEV_DOWN,
> disable_ipv6, or NETDEV_CHANGEMTU below IPV6_MIN_MTU. If it snapshots
> addr_list after ifp2 is linked, it can tear ifp2 down completely before
> the spin_lock_bh(&ifp2->lock) above:
> 
> CPU1 (addrconf_ifdown)
>     ifa->state = INET6_IFADDR_STATE_DEAD;
>     __ipv6_ifa_notify(RTM_DELADDR, ifa)
>         ip6_del_rt(net, ifp->rt, false);
>         ifp->rt = NULL;
>     list_del_rcu(&ifa->if_list);
>     in6_ifa_put(ifa);
> 
> CPU2 (addrconf_dad_failure)
>     ifp2->state = INET6_IFADDR_STATE_PREDAD;
>     addrconf_mod_dad_work(ifp2, ...);
> 
> Can CPU2 overwrite the DEAD state here and queue DAD work on an address
> that has already been torn down?
> 
> Suppose IF_READY is still set and ifp2 has inherited IFA_F_OPTIMISTIC.
> Then addrconf_dad_work()->addrconf_dad_begin() reaches:
> 
> 	if (ifp->flags & IFA_F_OPTIMISTIC) {
> 		ip6_ins_rt(net, ifp->rt);
> 
> At this point ifp->rt is NULL, and __ip6_ins_rt() dereferences
> rt->fib6_table. IF_READY stays set in two cases:
> 
> - the NETDEV_CHANGEMTU path calls addrconf_ifdown(dev, true) without a
>   prior NETDEV_DOWN
> - the link comes back up within idgen_delay
> 
> Otherwise the address is left in state DAD or POSTDAD. On the final put,
> inet6_ifa_finish_destroy() then hits:
> 
> 	if (ifp->state != INET6_IFADDR_STATE_DEAD) {
> 		pr_warn("Freeing alive inet6 address %p\n", ifp);
> 		return;
> 	}
> 
> so the ifaddr is never freed.
> 
> addrconf_dad_start() avoids this by checking for DEAD first:
> 
> 	if (ifp->state != INET6_IFADDR_STATE_DEAD) {
> 		ifp->state = INET6_IFADDR_STATE_PREDAD;
> 		begin_dad = true;
> 	}
> 
> Would a similar check under ifp2->lock in addrconf_dad_failure() work?
> The PREDAD write itself looks redundant, because ipv6_add_addr() already
> returns the object in state 0 (PREDAD). No later patch in this series
> changes addrconf_dad_failure().

Looks like a valid issue. Please fix in patch #4 in v4 with something
like this:

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 7bd0e8820abb..7cb156ddc191 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2230,7 +2230,6 @@ void addrconf_dad_failure(struct sk_buff *skb, struct inet6_ifaddr *ifp)
 
 		spin_lock_bh(&ifp2->lock);
 		ifp2->stable_privacy_retry = retries;
-		ifp2->state = INET6_IFADDR_STATE_PREDAD;
 		spin_unlock_bh(&ifp2->lock);
 
 		addrconf_mod_dad_work(ifp2, net->ipv6.sysctl.idgen_delay);