| Message ID | 20220523202543.9019-1-CFSworks@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv6/addrconf: fix timing bug in tempaddr regen | expand |
Hello, On Mon, 2022-05-23 at 14:25 -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 case 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 higher > up so that a temporary address cannot be deprecated until it has had an > opportunity to begin regeneration. 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. I looks like with this change the tmp addresses will never hit the DEPRECATED branch ?!? Thanks! Paolo
Bah, I've had to resend this since it went out as HTML yesterday. Sorry about the double mailing everyone! On Tue, May 24, 2022 at 3:24 AM Paolo Abeni <pabeni@redhat.com> wrote: > I looks like with this change the tmp addresses will never hit the > DEPRECATED branch ?!? The DEPRECATED branch becomes reachable again once this line is hit: ifp->regen_count++; ...because it causes this condition in the elseif to evaluate false: !ifp->regen_count
On Wed, 2022-05-25 at 14:07 -0600, Sam Edwards wrote: > Bah, I've had to resend this since it went out as HTML yesterday. > Sorry about the double mailing everyone! > > On Tue, May 24, 2022 at 3:24 AM Paolo Abeni <pabeni@redhat.com> wrote: > > I looks like with this change the tmp addresses will never hit the > > DEPRECATED branch ?!? > > The DEPRECATED branch becomes reachable again once this line is hit: > ifp->regen_count++; > ...because it causes this condition in the elseif to evaluate false: > !ifp->regen_count That condition looks problematic: 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) { 'age', 'ifp->prefered_lft' and 'regen_advance' are unsigned, and it looks like 'regen_advance' is not constrained to be less then 'ifp- >prefered_lft'. If that happens the condition will (allways) evaluate to false, there will be no temporary address regenaration, 'regen_count' will be untouched and the temporary address will never expire... ... unless I missed something relevant, which is totally possible ;) Otherwise I think we need to explicitly handle the 'regen_advance > ifp->prefered_lft' condition possibly with something alike: unsigned long regen_advance = ifp->idev->cnf.regen_max_retry * //... regen_adavance = min(regen_advance, ifp->prefered_lft); if (age >= ifp->prefered_lft - regen_advance) { //... Thanks, Paolo
On Thu, May 26, 2022 at 1:40 AM Paolo Abeni <pabeni@redhat.com> wrote: > 'age', 'ifp->prefered_lft' and 'regen_advance' are unsigned, and it > looks like 'regen_advance' is not constrained to be less then 'ifp- > >prefered_lft'. If that happens the condition will (allways) evaluate > to false, there will be no temporary address regenaration, > 'regen_count' will be untouched and the temporary address will never > expire... Hmm... I think the answer to whether `regen_advance` is constrained is "yes but actually no." ipv6_create_tempaddr() does have a check that will return early without even creating an address if the calculated preferred lifetime isn't greater than regen_advance. Now, if regen_advance were a constant, I'd say that's the end of it, but it's actually computed from some configuration variables which the system administrator might change (increase) between then and here. So what you said could end up happening, albeit in rare circumstances. > Otherwise I think we need to explicitly handle the 'regen_advance > > ifp->prefered_lft' condition possibly with something alike: > > unsigned long regen_advance = ifp->idev->cnf.regen_max_retry * //... > > regen_adavance = min(regen_advance, ifp->prefered_lft); > if (age >= ifp->prefered_lft - regen_advance) { //... For readability's sake, it might be better to mirror what ipv6_create_tempaddr does: if (age + regen_advance >= ifp->prefered_lft) Does that seem good? I doubt we'll have overflows as they would require unreasonably large regen_advance values. === Now, in thinking about this some more, I'm starting to believe that this if/elseif/elseif/... block is not the appropriate place for tempaddr regeneration at all. The branches of this block implement the *destructive* parts of an address's lifecycle. Most importantly, the branches can be in latest-to-earliest order because the effects are *cumulative*: if an address has a valid_lft only a second longer than the prefered_lft, and addrconf_verify_rtnl runs a second late, then the DEPRECATED branch will be skipped, but that's okay because the address actually gets deleted instead. Regenerating a temporary address is *not* a destructive stage in the address's lifecycle -- the effects are not cumulative -- so if that stage is skipped, regeneration never happens. My patch is trying to fix that by essentially *holding up the lifecycle* until regeneration happens. But that's leading to the more general concern I am seeing from you: a logic error in regeneration (even one that has been there all along, such as the potential underflow you just pointed out) could then affect the whole deprecation flow, not just regeneration. So, I think my v2 patch should probably put the tempaddr case right: age = (now - ifp->tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ; // here if (ifp->valid_lft != INFINITY_LIFE_TIME && age >= ifp->valid_lft) { Thoughts? :) Thank you for your input, Sam
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index b22504176588..5d02e4f0298b 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -4518,6 +4518,35 @@ static void addrconf_verify_rtnl(struct net *net) } else if (ifp->prefered_lft == INFINITY_LIFE_TIME) { spin_unlock(&ifp->lock); continue; + } else if ((ifp->flags&IFA_F_TEMPORARY) && + !(ifp->flags&IFA_F_TENTATIVE) && + !ifp->regen_count && ifp->ifpub) { + 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; + + 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 if (age >= ifp->prefered_lft) { /* jiffies - ifp->tstamp > age >= ifp->prefered_lft */ int deprecate = 0; @@ -4540,35 +4569,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 case 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 higher up so that a temporary address cannot be deprecated until it has had an opportunity to begin regeneration. 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 the latter 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 | 58 ++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 29 deletions(-)