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

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

[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 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(-)

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))
-- 
2.35.1

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

[Paolo Abeni]

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

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

[Sam Edwards]

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

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

[Paolo Abeni]

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

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

[Sam Edwards]

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