Re: [PATCH net v3 2/3] ipv6: remove ifaddr from hash during ifdown list cleanup

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates the bug being fixed.

--

ipv6: remove ifaddr from hash during ifdown list cleanup

This commit unhashes each non-kept address in addrconf_ifdown() just
before it is removed from the per-device list. That closes a race where
ipv6_add_addr() publishes an address after the Step 2 hash scan and
before the list snapshot.

Link: https://lore.kernel.org/r/20261004135117.GA206930@shredder

> The ifdown path then removes the address from the device list and drops
> its last reference while it is still linked in the hash. This triggers
> the WARN_ON() in inet6_ifa_finish_destroy().

[Severity: Low]
The code change looks correct. Does the commit message understate what
happens before this patch, though?

The WARN_ON() in inet6_ifa_finish_destroy() does not stop execution. In
this race, addrconf_ifdown() has already set ifa->state to
INET6_IFADDR_STATE_DEAD, so the "Freeing alive" early return is skipped
and the object is freed:

net/ipv6/addrconf.c:inet6_ifa_finish_destroy() {
	WARN_ON(!hlist_unhashed(&ifp->addr_lst));
    ...
	if (ifp->state != INET6_IFADDR_STATE_DEAD) {
		pr_warn("Freeing alive inet6 address %p\n", ifp);
		return;
	}

	kfree_rcu(ifp, rcu);
}

That leaves a freed ifa linked in net->ipv6.inet6_addr_lst. Later walks
of that hash chain would read freed memory. Examples include
ipv6_get_ifaddr() and ipv6_chk_addr() under RCU, ipv6_chk_same_addr()
from ipv6_add_addr_hash(), and the Step 2 scan in the next
addrconf_ifdown().

One way to reach it is ipv6_add_addr() called from RA processing through
addrconf_prefix_rcv_add_addr(), racing with a non-unregister
addrconf_ifdown().

> Unhashing before the list put prevents a stale hash entry.

Could the message say that the stale hash entry points to freed memory,
which makes this a use-after-free and not only a WARN?

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