Re: [PATCH net v3 3/3] ipv6: initialize temporary ifaddr before publication

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch makes the temporary ifaddr visible to
  manage_tempaddrs() before ipv6_create_tempaddr() has finished setting…

--

ipv6: initialize temporary ifaddr before publication

This patch passes the public ifaddr to ipv6_add_addr() through
ifa6_config, so ift->ifpub is set before the temporary address goes on
the hash and per-device lists. That closes a window where
addrconf_ifdown() could remove the temporary address while ifpub was
still NULL and leak the public ifaddr reference.

Link: https://lore.kernel.org/r/20261007164548.GA1153540@shredder

> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 5a7e7129d4346..699d058f5c786 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);

[ ... ]

> @@ -1504,7 +1506,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);

[Severity: Medium]

Now that ifpub is set before publication, can manage_tempaddrs() find the
new ift before these cstamp and tstamp stores run?

Before this patch, ifpub stayed NULL until this ift->lock section.
manage_tempaddrs() skips entries that don't match ifpub, so it ignored the
new address until all three fields were final:

net/ipv6/addrconf.c:manage_tempaddrs() {
    ...
	list_for_each_entry(ift, &idev->tempaddr_list, tmp_list) {
		...
		if (ifp != ift->ifpub)
			continue;
    ...
		ift->valid_lft = valid_lft;
		ift->prefered_lft = prefered_lft;
		ift->tstamp = now;
    ...
}

There is now a window between write_unlock_bh(&idev->lock) in
ipv6_add_addr() and the spin_lock_bh(&ift->lock) above. The NETDEV_UP
inet6addr notifier chain also runs inside this window.

The RA softirq path does not hold RTNL. It doesn't seem to be serialized
against ipv6_create_tempaddr() callers such as addrconf_verify_rtnl() or
addrconf_dad_stop():

CPU1
ipv6_create_tempaddr(ifp)
  tmp_tstamp = ifp->tstamp;
  ipv6_add_addr()
    ift published with ift->ifpub == ifp

CPU2 (RA softirq)
addrconf_prefix_rcv_add_addr()
  WRITE_ONCE(ifp->tstamp, now);
  manage_tempaddrs(in6_dev, ifp, valid_lft, prefered_lft, create, now)
    ift->valid_lft = valid_lft;
    ift->prefered_lft = prefered_lft;
    ift->tstamp = now;

CPU1
  ift->cstamp = now;
  ift->tstamp = tmp_tstamp;

Would ift then have lifetimes computed relative to CPU2's now, but a
tstamp taken from the older tmp_tstamp?

If so, the temporary address loses (now - tmp_tstamp) seconds of preferred
and valid lifetime. addrconf_verify() would then deprecate, regenerate or
expire it early.

Could cstamp and tstamp also be set before publication? One way is to
pass the timestamp through ifa6_config, or to set it in ipv6_add_addr()
next to ifpub. Then nothing in ift is written after it becomes reachable.

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