[PATCH net] ipv6: serialize address publication with addrconf_ifdown

[PATCH net] ipv6: serialize address publication with addrconf_ifdown

[Daehyeon Ko]

ipv6_add_addr() inserts an inet6_ifaddr into the per-net hash before
linking it into idev->addr_list. addrconf_ifdown() clears the hash first
and snapshots the device list later. A nonblocking add can therefore
enter between the two teardown observations.

When a non-loopback device's MTU falls below IPV6_MIN_MTU, teardown
marks and detaches the idev. A late add can then link the ifaddr into the
dead idev. On v7.2, a deterministic interleaving left the object
hash-visible with idev->dead=1. ipv6_get_ifaddr() returned it, and later
device deletion waited indefinitely with usage count 3. During network
namespace exit that wait can stall the single-thread netns cleanup
workqueue.

MTU changes require CAP_NET_ADMIN in the affected network namespace.
Where unprivileged user namespaces are permitted, a local user can
obtain that capability in a new user and network namespace.

Publish the hash and device-list memberships while holding
addrconf_hash_lock followed by idev->lock. Recheck idev->dead and
disable_ipv6 before either publication. If teardown wins, the add fails
before publishing the object or taking a list reference.

The same forced interleaving now returns -ENODEV and device deletion
completes without a KASAN or LOCKDEP diagnostic. A real Router
Advertisement separately reached ipv6_add_addr() with can_block=false;
bounded natural stress did not reproduce the full race. A reproducer is
available on request.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Daehyeon Ko <4ncienth@gmail.com>
---
 net/ipv6/addrconf.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index c90ee6dd7446c..899e9a47f7585 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1047,8 +1047,9 @@ static bool ipv6_chk_same_addr(struct net *net, const struct in6_addr *addr,
 	return false;
 }
 
-static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa)
+static int ipv6_add_addr_hash(struct inet6_dev *idev, struct inet6_ifaddr *ifa)
 {
+	struct net_device *dev = idev->dev;
 	struct net *net = dev_net(dev);
 	unsigned int hash = inet6_addr_hash(net, &ifa->addr);
 	int err = 0;
@@ -1060,7 +1061,23 @@ static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa)
 		netdev_dbg(dev, "ipv6_add_addr: already assigned\n");
 		err = -EEXIST;
 	} else {
-		hlist_add_head_rcu(&ifa->addr_lst, &net->ipv6.inet6_addr_lst[hash]);
+		write_lock(&idev->lock);
+		if (idev->dead || idev->cnf.disable_ipv6) {
+			err = idev->dead ? -ENODEV : -EACCES;
+		} else {
+			hlist_add_head_rcu(&ifa->addr_lst,
+					   &net->ipv6.inet6_addr_lst[hash]);
+			ipv6_link_dev_addr(idev, ifa);
+
+			if (ifa->flags & IFA_F_TEMPORARY) {
+				/* manage_tempaddrs() relies on addresses being added to the head */
+				list_add(&ifa->tmp_list, &idev->tempaddr_list);
+				in6_ifa_hold(ifa);
+			}
+
+			in6_ifa_hold(ifa);
+		}
+		write_unlock(&idev->lock);
 	}
 
 	spin_unlock_bh(&net->ipv6.addrconf_hash_lock);
@@ -1168,26 +1185,12 @@ ipv6_add_addr(struct inet6_dev *idev, struct ifa6_config *cfg,
 
 	rcu_read_lock();
 
-	err = ipv6_add_addr_hash(idev->dev, ifa);
+	err = ipv6_add_addr_hash(idev, ifa);
 	if (err < 0) {
 		rcu_read_unlock();
 		goto out;
 	}
 
-	write_lock_bh(&idev->lock);
-
-	/* Add to inet6_dev unicast addr list. */
-	ipv6_link_dev_addr(idev, ifa);
-
-	if (ifa->flags&IFA_F_TEMPORARY) {
-		/* manage_tempaddrs() relies on addresses being added to the head */
-		list_add(&ifa->tmp_list, &idev->tempaddr_list);
-		in6_ifa_hold(ifa);
-	}
-
-	in6_ifa_hold(ifa);
-	write_unlock_bh(&idev->lock);
-
 	rcu_read_unlock();
 
 	inet6addr_notifier_call_chain(NETDEV_UP, ifa);

base-commit: 7375d38364a9aa66fb31716bcefef38aecad75d8

Re: [PATCH net] ipv6: serialize address publication with addrconf_ifdown

[Ido Schimmel]

On Thu, Oct 01, 2026 at 02:21:50PM +0900, Daehyeon Ko wrote:
> ipv6_add_addr() inserts an inet6_ifaddr into the per-net hash before
> linking it into idev->addr_list. addrconf_ifdown() clears the hash first
> and snapshots the device list later. A nonblocking add can therefore
> enter between the two teardown observations.
> 
> When a non-loopback device's MTU falls below IPV6_MIN_MTU, teardown
> marks and detaches the idev. A late add can then link the ifaddr into the
> dead idev. On v7.2, a deterministic interleaving left the object
> hash-visible with idev->dead=1. ipv6_get_ifaddr() returned it, and later
> device deletion waited indefinitely with usage count 3. During network
> namespace exit that wait can stall the single-thread netns cleanup
> workqueue.
> 
> MTU changes require CAP_NET_ADMIN in the affected network namespace.
> Where unprivileged user namespaces are permitted, a local user can
> obtain that capability in a new user and network namespace.
> 
> Publish the hash and device-list memberships while holding
> addrconf_hash_lock followed by idev->lock. Recheck idev->dead and
> disable_ipv6 before either publication. If teardown wins, the add fails
> before publishing the object or taking a list reference.

Adding to the per-idev list under the per-netns hash lock looks weird.
Can't we instead do the following?

1. Make sure that the dead indication is written under the idev lock.

2. Extend the critical section of the idev lock so that it also covers
the addition to the per-netns hash table and only if the device is not
dead.

Something like [1] (untested).

Also, note that this doesn't solve the problem of addrconf_ifdown() and
ipv6_add_addr() interleaving when the former doesn't mark the device as
dead (e.g., upon NETDEV_DOWN).

ipv6_add_addr() can add an address to the hash table after
addrconf_ifdown() cleared the hash table, but before it cleared the
per-idev list. It will trigger the WARN_ON() in
inet6_ifa_finish_destroy():

WARN_ON(!hlist_unhashed(&ifp->addr_lst));

Can be fixed in a second patch [2] (untested) in the series.

> 
> The same forced interleaving now returns -ENODEV and device deletion
> completes without a KASAN or LOCKDEP diagnostic. A real Router
> Advertisement separately reached ipv6_add_addr() with can_block=false;
> bounded natural stress did not reproduce the full race. A reproducer is

"bounded natural stress did not reproduce the full race" is LLM speak
for "mdelay()s were placed in ipv6_add_addr()"?

> available on request.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

Blaming 8814c4b53381 ("[IPV6] ADDRCONF: Convert addrconf_lock to RCU.") seems
more appropriate.

> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Daehyeon Ko <4ncienth@gmail.com>

[1]
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3dba94bd2ba0..51add8f91139 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1171,14 +1171,18 @@ 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
+		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);
 
@@ -3900,7 +3904,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);

[2]
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3dba94bd2ba0..eae51b5de690 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4022,6 +4028,9 @@ static int addrconf_ifdown(struct net_device *dev, bool unregister)
 		}
 
 		if (!keep) {
+			spin_lock_bh(&net->ipv6.addrconf_hash_lock);
+			hlist_del_init_rcu(&ifa->addr_lst);
+			spin_unlock_bh(&net->ipv6.addrconf_hash_lock);
 			write_lock_bh(&idev->lock);
 			list_del_rcu(&ifa->if_list);
 			write_unlock_bh(&idev->lock);

Re: [PATCH net] ipv6: serialize address publication with addrconf_ifdown

[netdev-bot+sashiko]

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

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

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 fix is incomplete.
- [Low] The new recheck in ipv6_add_addr_hash() reads idev->dead and
  idev->cnf.disable_ipv6 with plain loads…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it leaves open
  a race with the same symptom on the same nonblocking RA path.

--

ipv6: serialize address publication with addrconf_ifdown

This patch moves the idev->addr_list and tempaddr_list linking into
ipv6_add_addr_hash(). Both the hash and the device list publications now
happen under addrconf_hash_lock and then idev->lock. Before publishing,
the patch rechecks idev->dead and disable_ipv6, so an add that loses the
race with addrconf_ifdown() fails before the address is published.

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index c90ee6dd7446c..899e9a47f7585 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c

[ ... ]

> @@ -1060,7 +1061,23 @@ static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa)
>  		netdev_dbg(dev, "ipv6_add_addr: already assigned\n");
>  		err = -EEXIST;
>  	} else {
> -		hlist_add_head_rcu(&ifa->addr_lst, &net->ipv6.inet6_addr_lst[hash]);
> +		write_lock(&idev->lock);
> +		if (idev->dead || idev->cnf.disable_ipv6) {
> +			err = idev->dead ? -ENODEV : -EACCES;

[Severity: High]
Does this recheck close the window for the addrconf_ifdown(dev, false)
callers?

idev->dead is set only when unregister is true. disable_ipv6 is set only
by the sysctl path and the DAD-abort path. Three callers set neither
flag: NETDEV_DOWN, CHANGEMTU on the loopback device, and
NETDEV_CHANGEUPPER to an L3 master:

addrconf_notify() {
    ...
	case NETDEV_DOWN:
	case NETDEV_UNREGISTER:
		...
		addrconf_ifdown(dev, event != NETDEV_DOWN);
    ...
	case NETDEV_CHANGEUPPER:
		...
		if (info->upper_dev && netif_is_l3_master(info->upper_dev))
			addrconf_ifdown(dev, false);
}

In the VRF case, do_vrf_add_slave() calls netdev_master_upper_dev_link()
while the port is still up and receiving RAs.

addrconf_ifdown() clears the hash one bucket at a time. Only after that
does it snapshot addr_list under idev->lock:

addrconf_ifdown() {
    ...
	/* Step 2: clear hash table */
	for (i = 0; i < IN6_ADDR_HSIZE; i++) {
		...
		spin_lock_bh(&net->ipv6.addrconf_hash_lock);
		...
		spin_unlock_bh(&net->ipv6.addrconf_hash_lock);
	}

	write_lock_bh(&idev->lock);
    ...
	list_for_each_entry(ifa, &idev->addr_list, if_list)
		list_add_tail(&ifa->if_list_aux, &tmp_addr_list);
    ...
}

Suppose an RA is processed in softirq, without RTNL, on another CPU:

ndisc_router_discovery()
  addrconf_prefix_rcv()
    addrconf_prefix_rcv_add_addr()
      ipv6_add_addr(in6_dev, &cfg, false, NULL)

If it publishes into a bucket that Step 2 has already cleared, the ifa
goes into the snapshot but stays hashed. SLAAC addresses lack
IFA_F_PERMANENT, so keep is false and the Step 4 loop only does this:

		if (!keep) {
			write_lock_bh(&idev->lock);
			list_del_rcu(&ifa->if_list);
			write_unlock_bh(&idev->lock);
			in6_ifa_put(ifa);
		}

When the last reference is dropped, inet6_ifa_finish_destroy() fires
WARN_ON(!hlist_unhashed(&ifp->addr_lst)). Because the state is DEAD, it
still calls kfree_rcu(). ipv6_del_addr() cannot clean this up either,
because it returns early for DEAD addresses.

Can this leave inet6_addr_lst pointing at a freed inet6_ifaddr? If so,
later hash walks in ipv6_get_ifaddr(), ipv6_chk_addr(),
ipv6_chk_same_addr(), addrconf_verify_rtnl() and /proc/net/if_inet6
would read freed memory. The hlist_del_init_rcu() in a later unregister
would write to it.

This race existed before the patch, and the patch does not widen it.
Since the subject says publication is serialized with addrconf_ifdown,
could this mode be covered too? Two options would be unhashing in the
Step 4 !keep loop, or adding a going-down state that the add path checks.

[Severity: Low]
Should these reads use READ_ONCE()?

The writers hold RTNL only. They hold neither idev->lock nor
addrconf_hash_lock:

addrconf_ifdown():
	WRITE_ONCE(idev->dead, 1);

addrconf_disable_change():
	WRITE_ONCE(idev->cnf.disable_ipv6, newf);

addrconf_dad_work():
	WRITE_ONCE(idev->cnf.disable_ipv6, 1);

So the idev->lock held here does not protect these fields, although the
changelog suggests that the recheck relies on it. The check is correct
today only because addrconf_ifdown() stores dead and disable_ipv6 before
it takes addrconf_hash_lock in Step 2, and this check runs while holding
addrconf_hash_lock.

Could these become READ_ONCE() with a comment that names that ordering?
As written, KCSAN can report the race. A later reordering in
addrconf_ifdown(), such as setting dead after Step 2, would also silently
reopen the window. The earlier checks in ipv6_add_addr() use the same
plain reads.

> +		} else {
> +			hlist_add_head_rcu(&ifa->addr_lst,
> +					   &net->ipv6.inet6_addr_lst[hash]);
> +			ipv6_link_dev_addr(idev, ifa);
> +
> +			if (ifa->flags & IFA_F_TEMPORARY) {
> +				/* manage_tempaddrs() relies on addresses being added to the head */
> +				list_add(&ifa->tmp_list, &idev->tempaddr_list);
> +				in6_ifa_hold(ifa);
> +			}

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but there
seems to be a related window on the same nonblocking RA path.

ipv6_create_tempaddr() publishes the temporary address first. It sets
ifpub only after ipv6_add_addr() has returned and all locks are dropped:

ipv6_create_tempaddr() {
    ...
	ift = ipv6_add_addr(idev, &cfg, block, NULL);
    ...
	spin_lock_bh(&ift->lock);
	ift->ifpub = ifp;
    ...
	addrconf_dad_start(ift);
	in6_ifa_put(ift);
    ...
}

If Step 3 of addrconf_ifdown() runs between those two points, it removes
ift from tempaddr_list while ifpub is still NULL:

		list_del(&ifa->tmp_list);
		...
		if (ifa->ifpub) {
			in6_ifa_put(ifa->ifpub);
			ifa->ifpub = NULL;
		}
		...
		in6_ifa_put(ifa);

The creator then stores ifp in ift->ifpub, which hands over its reference
to ifp. Step 4 marks ift DEAD, because temporary addresses are never
IFA_F_PERMANENT. The final in6_ifa_put(ift) goes through
inet6_ifa_finish_destroy(), which never drops ifpub.

Nothing else releases that reference. ift is no longer on tempaddr_list,
and ipv6_del_addr() returns early for DEAD addresses.

Does this leak the reference on the public inet6_ifaddr? That would also
pin the inet6_dev and net_device, so unregister_netdevice() would wait
forever. This is the same netns cleanup stall that the commit message
describes.

The nonblocking caller is manage_tempaddrs()->ipv6_create_tempaddr(ifp,
false). It is reached from this softirq path without RTNL:

ndisc_router_discovery()
  addrconf_prefix_rcv()
    addrconf_prefix_rcv_add_addr()
      manage_tempaddrs()

So it can run at the same time as an RTNL-held addrconf_ifdown()
triggered by NETDEV_DOWN, an MTU drop, VRF enslave or unregister.

[ ... ]

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