* [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen
@ 2022-05-28 0:48 Sam Edwards
2022-05-31 7:50 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: Sam Edwards @ 2022-05-28 0:48 UTC (permalink / raw)
To: David S . Miller, Hideaki YOSHIFUJI, David Ahern
Cc: Linux Network Development Mailing List, Paolo Abeni, Sam Edwards
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))
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen
2022-05-28 0:48 [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen Sam Edwards
@ 2022-05-31 7:50 ` Paolo Abeni
2022-06-03 5:02 ` Sam Edwards
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Abeni @ 2022-05-31 7:50 UTC (permalink / raw)
To: Sam Edwards, David S . Miller, Hideaki YOSHIFUJI, David Ahern
Cc: Linux Network Development Mailing List
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen
2022-05-31 7:50 ` Paolo Abeni
@ 2022-06-03 5:02 ` Sam Edwards
2022-06-03 17:13 ` Paolo Abeni
0 siblings, 1 reply; 4+ messages in thread
From: Sam Edwards @ 2022-06-03 5:02 UTC (permalink / raw)
To: Paolo Abeni
Cc: David S . Miller, Hideaki YOSHIFUJI, David Ahern,
Linux Network Development Mailing List
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen
2022-06-03 5:02 ` Sam Edwards
@ 2022-06-03 17:13 ` Paolo Abeni
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2022-06-03 17:13 UTC (permalink / raw)
To: Sam Edwards
Cc: David S . Miller, Hideaki YOSHIFUJI, David Ahern,
Linux Network Development Mailing List
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-03 17:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-28 0:48 [PATCH v2] ipv6/addrconf: fix timing bug in tempaddr regen Sam Edwards
2022-05-31 7:50 ` Paolo Abeni
2022-06-03 5:02 ` Sam Edwards
2022-06-03 17:13 ` Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.