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

[netdev-bot+sashiko@kernel.org]

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