[XFRM][RFC v1] Fix unexpected SA hard expiration after setting new date

[XFRM][RFC v1] Fix unexpected SA hard expiration after setting new date

[fan.du]

First, I'm not sure whether I Cced to the right person, if not,
apologize for the noise.


*Background*:
Once IPsec SAs are created between two peers, kernel setup a timer to monitor
two events: soft/hard expiration. However the timer handler use xtime to
caculate whether it's soft or hard expiration event.

normal code flow(hard expire time:100s, soft expire time:82s)

a) When new SAs created, xfrm_timer_handler is called one second
after its creation. At this point, calculate soft expire
interval(81s), setup the timer;

b) soft expire occur, rearm the timer with hard expire interval(18s)
then notify racoon2 about soft expire event. racoon2 will create
new SAs.

c) hard expire happen, notify racoon2 about it. racoon2 will delete
the old SAs.

*Scenario*:
Setting a new date before b),and after a) could result c) happens first,
As a result, old SAs is deleted before new ones are created. Normally
new SAs will be created by the next time networking traffic, but there
is a small time being when networking connection is down, this could
result in upper layer connections failed in tel comm area, thus it's
better to keep it strict in sequence.

*Workaround*:
set new time could happen:
1) before a), then SAs is updated with new time.
2) before b),and after a)
2a) When new SAs created, xfrm_timer_handler is called one second
after its creation. At this point, calculate soft expire
interval(81s), setup the timer;(set flag to mark next time should
be soft time expire)

<<---- new date comes

2b) soft expire occur, the calculation results in a hard time expire
event, but flag is set, so catch ya. Sync the addtime, and rearm
the timer with hard expire interval(18s), then notify racoon2
about soft expire event;

2c) hard expire happen, notify racoon2 about it;
so everything is in order.

3) after b), hard expire always happened anyway.


So, could you please give your comments on this?

thanks

[PATCH] [XFRM] Fix unexpected SA hard expiration after changing date

[fan.du]

After SA is setup, one timer is armed to detect soft/hard expiration,
however the timer handler uses xtime to do the math. This makes hard
expiration occurs first before soft expiration after setting new date
with big interval. As a result new child SA is deleted before rekeying
the new one.

Signed-off-by: fan.du <fan.du@windriver.com>
---
 include/net/xfrm.h    |    2 ++
 net/xfrm/xfrm_state.c |   22 ++++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 2933d74..1734acc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -214,6 +214,8 @@ struct xfrm_state
 	/* Private data of this transformer, format is opaque,
 	 * interpreted by xfrm_type methods. */
 	void			*data;
+	u32				flags;
+	long			saved_tmo;
 };
 
 /* xflags - make enum if more show up */
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index fd77cf0..da2cc78 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -442,8 +442,18 @@ static void xfrm_timer_handler(unsigned long data)
 	if (x->lft.hard_add_expires_seconds) {
 		long tmo = x->lft.hard_add_expires_seconds +
 			x->curlft.add_time - now;
-		if (tmo <= 0)
-			goto expired;
+		if (tmo <= 0) {
+			if (x->flags != 1)
+				goto expired;
+			else {
+				/* enter hard expire without soft expire first?!
+				 * setting a new date could trigger this.
+				 * workarbound: fix x->curflt.add_time by below:
+				 */
+				x->curlft.add_time = now - x->saved_tmo - 1;
+				tmo = x->lft.hard_add_expires_seconds - x->saved_tmo;
+			}
+		}
 		if (tmo < next)
 			next = tmo;
 	}
@@ -460,10 +470,14 @@ static void xfrm_timer_handler(unsigned long data)
 	if (x->lft.soft_add_expires_seconds) {
 		long tmo = x->lft.soft_add_expires_seconds +
 			x->curlft.add_time - now;
-		if (tmo <= 0)
+		if (tmo <= 0) {
 			warn = 1;
-		else if (tmo < next)
+			x->flags = 0;
+		} else if (tmo < next) {
 			next = tmo;
+			x->flags = 1;
+			x->saved_tmo = tmo;
+		}
 	}
 	if (x->lft.soft_use_expires_seconds) {
 		long tmo = x->lft.soft_use_expires_seconds +
-- 
1.6.3.1

Re: [PATCH] [XFRM] Fix unexpected SA hard expiration after changing date

[Steffen Klassert]

On Mon, Jun 18, 2012 at 04:24:16PM +0800, fan.du wrote:
> After SA is setup, one timer is armed to detect soft/hard expiration,
> however the timer handler uses xtime to do the math. This makes hard
> expiration occurs first before soft expiration after setting new date
> with big interval. As a result new child SA is deleted before rekeying
> the new one.
> 
> Signed-off-by: fan.du <fan.du@windriver.com>
> ---
>  include/net/xfrm.h    |    2 ++
>  net/xfrm/xfrm_state.c |   22 ++++++++++++++++++----
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 2933d74..1734acc 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -214,6 +214,8 @@ struct xfrm_state
>  	/* Private data of this transformer, format is opaque,
>  	 * interpreted by xfrm_type methods. */
>  	void			*data;
> +	u32				flags;

We already have the xflags field, it holds exactly one flag
at the moment. So I think we don't need yet another u32 that
holds one flag too.

Re: [PATCH] [XFRM] Fix unexpected SA hard expiration after changing date

[Fan Du]

On 2012年06月18日 19:05, Steffen Klassert wrote:
> On Mon, Jun 18, 2012 at 04:24:16PM +0800, fan.du wrote:
>> After SA is setup, one timer is armed to detect soft/hard expiration,
>> however the timer handler uses xtime to do the math. This makes hard
>> expiration occurs first before soft expiration after setting new date
>> with big interval. As a result new child SA is deleted before rekeying
>> the new one.
>>
>> Signed-off-by: fan.du<fan.du@windriver.com>
>> ---
>>   include/net/xfrm.h    |    2 ++
>>   net/xfrm/xfrm_state.c |   22 ++++++++++++++++++----
>>   2 files changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index 2933d74..1734acc 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -214,6 +214,8 @@ struct xfrm_state
>>   	/* Private data of this transformer, format is opaque,
>>   	 * interpreted by xfrm_type methods. */
>>   	void			*data;
>> +	u32				flags;
>
> We already have the xflags field, it holds exactly one flag
> at the moment. So I think we don't need yet another u32 that
> holds one flag too.
>

good point!
I will make it in the next version.


-- 

Love each day!
--fan

Re: [XFRM][RFC v1] Fix unexpected SA hard expiration after setting new date

[Herbert Xu]

On Mon, Jun 18, 2012 at 04:24:15PM +0800, fan.du wrote:
> 
> So, could you please give your comments on this?

Well, this used to work back when we were using relative time
instead of absolute time.  Then someone came along and changed
it for suspend/resume.

I didn't like this new behaviour but Dave convinced me that
it is a good thing :)

I guess I can live with your workaround if Dave is happy with
it.  But IMHO we should just go back to relative time and fix
the suspend/resume user-space scripts instead.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [XFRM][RFC v1] Fix unexpected SA hard expiration after setting new date

[Fan Du]

On 2012年06月18日 16:40, Herbert Xu wrote:
> On Mon, Jun 18, 2012 at 04:24:15PM +0800, fan.du wrote:
>>
>> So, could you please give your comments on this?
>
> Well, this used to work back when we were using relative time
> instead of absolute time.  Then someone came along and changed
> it for suspend/resume.
>
> I didn't like this new behaviour but Dave convinced me that
> it is a good thing :)
>

One of our custom complained the networking down when changing date,
even it's only less than 10 seconds, but they complain anyway:)
So it probably hurts much in practice for tel comm company.


> I guess I can live with your workaround if Dave is happy with
> it.  But IMHO we should just go back to relative time and fix
> the suspend/resume user-space scripts instead.

Ok, let's see what Dave will say about this.
And thanks for your comments.

>
> Cheers,

-- 

Love each day!
--fan

Re: [XFRM][RFC v1] Fix unexpected SA hard expiration after setting new date

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 18 Jun 2012 16:40:21 +0800

> I guess I can live with your workaround if Dave is happy with
> it.  But IMHO we should just go back to relative time and fix
> the suspend/resume user-space scripts instead.

In the short term I'm happy with it too.

But I seem to remember that last time this issue came up the
suggestion was to use highres timers.  Someone tried but they did an
amazingly poor job so the effort just died off.  It takes someone with
some skill because highres timers operate with different context
requirements than normal timers.

Re: [XFRM][RFC v1] Fix unexpected SA hard expiration after setting new date

[Herbert Xu]

On Tue, Jun 19, 2012 at 12:34:22AM -0700, David Miller wrote:
> 
> But I seem to remember that last time this issue came up the
> suggestion was to use highres timers.  Someone tried but they did an
> amazingly poor job so the effort just died off.  It takes someone with
> some skill because highres timers operate with different context
> requirements than normal timers.

Sorry I think I didn't see that discussion or if I did, then
it has completely left my mind :)

So excuse my ignorance, are highres timers relative like jiffies
or absolute? If it's absolute, I don't see how it would impact this
case.

If it's relative, what exactly does it buy for us over jiffies?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt