| Message ID | 20220528004820.5916-1-CFSworks@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] ipv6/addrconf: fix timing bug in tempaddr regen | expand |
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
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 <pabeni@redhat.com> wrote: > > 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 >
On Thu, 2022-06-02 at 23:02 -0600, Sam Edwards wrote: > 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. It does not look easy, but you could use kunit to create e.g. a temporary address on the loopback device with the critical expiration time setting and check that it's deleted in due time - possibly after tuning the defaults to a reasonably short period. The above will additionally open-up opportunities for more autoconf tests. Up to your good will ;) Thanks Paolo
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 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(-)