Re: [PATCH net v2] ipv6: bound temporary address regeneration advance

[Ido Schimmel <idosch@nvidia.com>]

On Thu, Oct 29, 2026 at 11:26:27PM +0800, Yingjie Wang wrote:
> ipv6_get_regen_advance() combines regen_min_advance, regen_max_retry,
> dad_transmits and the neighbor retransmission time. The first three are
> signed sysctls, and the product is evaluated as int. Negative values or
> signed overflow can make the unsigned long result much larger than
> intended.
> 
> The retransmission time can also be supplied by an on-link Router
> Advertisement. On an x86-64 HZ=1000 host with regen_min_advance=2,
> regen_max_retry=3 and dad_transmits=1, an RA Retrans Timer of
> 1431654765 ms makes the old int product wrap to -3001. Division by HZ
> and addition of regen_min_advance then produce -1, which is returned as
> ULONG_MAX. This gives an on-link trigger on hosts that accept RAs and
> use temporary addresses without requiring a sysctl write.
> 
> When ipv6_create_tempaddr() clamps the preferred lifetime, it adds age
> and one to this advance and stores it in a u32. The sum can wrap or be
> truncated to a short preferred lifetime. addrconf_verify_rtnl() then
> finds the new address due for regeneration and restarts its scan after
> creating another one. The temporary-address path has had no
> max_addresses check since commit 4b08a8f1bd8c ("ipv6: remove
> max_addresses check from ipv6_create_tempaddr"). In a baseline namespace
> test, setting dad_transmits=-1 and adding a temporary-address template
> with mngtmpaddr produced 2859 loopback addresses in two seconds.
> 
> Reject negative values for all three sysctls with -EINVAL. Writes that
> used to succeed now fail validation; negative values have no useful
> meaning (negative regen_max_retry already disables temporary addresses
> on the first attempt). Keep zero valid. Use checked u64 arithmetic and
> div_u64() to calculate the advance, including values supplied by an RA,
> and cap it at U32_MAX. Compare the required preferred lifetime in u64
> before narrowing it to u32. Calculate the desync margin without an
> unsigned subtraction wrap on 32-bit builds. Clamp the effective maximum
> to that non-negative margin so a negative max_desync_factor cannot make
> the u32 comparison architecture-dependent.
> 
> Tested on x86-64 KASAN:
> - negative writes to the three sysctls were rejected without changing
>   their values, and zero remained valid;
> - normal temporary-address setup and a case with writable
>   max_desync_factor=-1 each produced three IPv6 addresses;
> - with HZ=1000 and a one-second retransmission time,
>   regen_min_advance=0, regen_max_retry=2147483647 and dad_transmits=1
>   caused the baseline kernel to reach 2847 loopback addresses after
>   two seconds; the fixed kernel left two.
> 
> Kernel selftests were not run.
> 
> An i386 defconfig with CONFIG_IPV6=y and HZ=1000 built addrconf.o with
> W=1; nm -u showed no __udivdi3 reference.
> 
> Fixes: f4bcbf360ac8 ("net: ipv6/addrconf: clamp preferred_lft to the minimum required")
> Signed-off-by: Yingjie Wang <1075151112@qq.com>
> ---
> Changes in v2:
> - Reorder the declarations for the netdev reverse-Christmas-tree convention.
> - Use an intermediate variable to make the max_desync_factor calculation
>   easier to read, as requested by Paolo Abeni.
> - Clamp the effective max_desync_factor to a non-negative range so the u32
>   comparison has the same result on 32-bit and 64-bit builds.
> - Document the RA-supplied Retrans Timer trigger raised in review.
> 
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/tencent_F6E8C7CAC6D390C8E912EEE4FD9FE4333505@qq.com/
> 
>  Documentation/networking/ip-sysctl.rst |  3 ++
>  net/ipv6/addrconf.c                    | 53 ++++++++++++++++++--------
>  2 files changed, 41 insertions(+), 15 deletions(-)

This patch is very hard to understand.

My understanding is that ipv6_create_tempaddr() makes sure that it's not
creating a temporary address that requires immediate regeneration. It's
doing that by setting the preferred lifetime of such an address to
'regen_advance + age + 1', so that at least one second will pass before
a new temporary address needs to be generated.

The problem is that this expression can overflow to 0 or be truncated when
assigned to 'preferred_lft' if it's larger than U32_MAX, resulting in a
preferred lifetime that requires immediate regeneration.

Can't we solve it with the following diff?

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index c8c0dd04982d..d9d13e78bf13 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1487,7 +1487,8 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, bool block)
 	age = (now - tmp_tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
 	if (cfg.preferred_lft <= regen_advance + age) {
 		cfg.preferred_lft = regen_advance + age + 1;
-		if (cfg.preferred_lft > cfg.valid_lft ||
+		if (cfg.preferred_lft <= regen_advance + age ||
+		    cfg.preferred_lft > cfg.valid_lft ||
 		    cfg.preferred_lft > if_public_preferred_lft) {
 			in6_ifa_put(ifp);
 			in6_dev_put(idev);