Hello,

Since this is a periodic routine, it receives coverage as the system
runs normally. If we're concerned about protecting this change from
regressions and/or through the merge process, some kind of automated
test might be in order, but right now the way to test it is to leave
the system up, with tempaddrs enabled, on a IPv6 SLAAC network, for
several multiples of the temp_prefered_lft. Though the way I see it,
that suggests that a selftest to do just that may be inappropriate
here: one of the rules for selftests is "Don’t take too long."

I can shorten that long line, though do note that my Signed-off-by tag
is correct. That is the canonical capitalization of my email address!
:)

Thanks and see you when net-next is open,
Sam

On Tue, May 31, 2022 at 1:50 AM Paolo Abeni [off-list ref] wrote:

Hello,

On Fri, 2022-05-27 at 18:48 -0600, Sam Edwards wrote:

quoted

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 <redacted>
---
 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