Re: [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen

[Paolo Abeni <pabeni@redhat.com>]

Hello,

On Fri, 2022-05-27 at 18:48 -0600, Sam Edwards wrote:
> The addrconf_verify_rtnl() function uses a big if/elseif/elseif/... block
> to categorize each address by what type of attention it needs.  An
> about-to-expire (RFC 4941) temporary address is one such category, but the
> previous elseif branch catches addresses that have already run out their
> prefered_lft.  This means that if addrconf_verify_rtnl() fails to run in
> the necessary time window (i.e. REGEN_ADVANCE time units before the end of
> the prefered_lft), the temporary address will never be regenerated, and no
> temporary addresses will be available until each one's valid_lft runs out
> and manage_tempaddrs() begins anew.
> 
> Fix this by moving the entire temporary address regeneration case out of
> that block.  That block is supposed to implement the "destructive" part of
> an address's lifecycle, and regenerating a fresh temporary address is not,
> semantically speaking, actually tied to any particular lifecycle stage.
> The age test is also changed from `age >= prefered_lft - regen_advance`
> to `age + regen_advance >= prefered_lft` instead, to ensure no underflow
> occurs if the system administrator increases the regen_advance to a value
> greater than the already-set prefered_lft.
> 
> Note that this does not fix the problem of addrconf_verify_rtnl() sometimes
> not running in time, resulting in the race condition described in RFC 4941
> section 3.4 - it only ensures that the address is regenerated.  Fixing THAT
> problem may require either using jiffies instead of seconds for all time
> arithmetic here, or always rounding up when regen_advance is converted to
> seconds.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  net/ipv6/addrconf.c | 62 ++++++++++++++++++++++++---------------------
>  1 file changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index b22504176588..57aa46cb85b7 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4507,6 +4507,39 @@ static void addrconf_verify_rtnl(struct net *net)
>  			/* We try to batch several events at once. */
>  			age = (now - ifp->tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
>  
> +			if ((ifp->flags&IFA_F_TEMPORARY) &&
> +			    !(ifp->flags&IFA_F_TENTATIVE) &&
> +			    ifp->prefered_lft != INFINITY_LIFE_TIME &&
> +			    !ifp->regen_count && ifp->ifpub) {
> +				/* This is a non-regenerated temporary addr. */
> +
> +				unsigned long regen_advance = ifp->idev->cnf.regen_max_retry *
> +					ifp->idev->cnf.dad_transmits *
> +					max(NEIGH_VAR(ifp->idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;
> +
> +				if (age + regen_advance >= ifp->prefered_lft) {
> +					struct inet6_ifaddr *ifpub = ifp->ifpub;
> +					if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
> +						next = ifp->tstamp + ifp->prefered_lft * HZ;
> +
> +					ifp->regen_count++;
> +					in6_ifa_hold(ifp);
> +					in6_ifa_hold(ifpub);
> +					spin_unlock(&ifp->lock);
> +
> +					spin_lock(&ifpub->lock);
> +					ifpub->regen_count = 0;
> +					spin_unlock(&ifpub->lock);
> +					rcu_read_unlock_bh();
> +					ipv6_create_tempaddr(ifpub, true);
> +					in6_ifa_put(ifpub);
> +					in6_ifa_put(ifp);
> +					rcu_read_lock_bh();
> +					goto restart;
> +				} else if (time_before(ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ, next))
> +					next = ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ;
> +			}
> +
>  			if (ifp->valid_lft != INFINITY_LIFE_TIME &&
>  			    age >= ifp->valid_lft) {
>  				spin_unlock(&ifp->lock);
> @@ -4540,35 +4573,6 @@ static void addrconf_verify_rtnl(struct net *net)
>  					in6_ifa_put(ifp);
>  					goto restart;
>  				}
> -			} else if ((ifp->flags&IFA_F_TEMPORARY) &&
> -				   !(ifp->flags&IFA_F_TENTATIVE)) {
> -				unsigned long regen_advance = ifp->idev->cnf.regen_max_retry *
> -					ifp->idev->cnf.dad_transmits *
> -					max(NEIGH_VAR(ifp->idev->nd_parms, RETRANS_TIME), HZ/100) / HZ;
> -
> -				if (age >= ifp->prefered_lft - regen_advance) {
> -					struct inet6_ifaddr *ifpub = ifp->ifpub;
> -					if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
> -						next = ifp->tstamp + ifp->prefered_lft * HZ;
> -					if (!ifp->regen_count && ifpub) {
> -						ifp->regen_count++;
> -						in6_ifa_hold(ifp);
> -						in6_ifa_hold(ifpub);
> -						spin_unlock(&ifp->lock);
> -
> -						spin_lock(&ifpub->lock);
> -						ifpub->regen_count = 0;
> -						spin_unlock(&ifpub->lock);
> -						rcu_read_unlock_bh();
> -						ipv6_create_tempaddr(ifpub, true);
> -						in6_ifa_put(ifpub);
> -						in6_ifa_put(ifp);
> -						rcu_read_lock_bh();
> -						goto restart;
> -					}
> -				} else if (time_before(ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ, next))
> -					next = ifp->tstamp + ifp->prefered_lft * HZ - regen_advance * HZ;
> -				spin_unlock(&ifp->lock);
>  			} else {
>  				/* ifp->prefered_lft <= ifp->valid_lft */
>  				if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))

The change looks correct to me, but it feels potentially
dangerous/impacting currently correct behaviours - especially
considering the lack of selftests for this code-path.

This looks like net-next material, and net-next is currently close. I
suggest to add a self-test verifying the tmp address regeneration and
expiration - I'm not sure how complext that will be, sorry - and re-
post when net-next re-opens.
While at that, please fix your SoB tag (there is a case mismatch with
the sender address) and it would be probably nice to shorten the line
exceeding the 100 chars limit.

Thanks,

Paolo