[IPSEC] add missing flow_cache_genid update to xfrm_policy_delete()

[IPSEC] add missing flow_cache_genid update to xfrm_policy_delete()

[Eugene Surovegin]

Hi!

I think there is a missing atomic_inc(&flow_cache_genid) in 
xfrm_policy.c::xfrm_policy_delete().

We were experiencing system lockups I think I tracked down to the stale 
xfrm policy in the flow cache. 

Here is our scenario:

policy timer expired -> xfrm_policy_timer() -> xfrm_policy_delete() -> 
xfrm_policy_kill() (here policy->dead is set to 1).

Note that none of these calls increment flow_cache_genid.

Some time later xfrm_lookup() is called (in my case it happened in softirq 
context). flow_cache_lookup() returns policy from the flow cache (i.e. 
resolver xfrm_policy_lookup() is NOT called) and this policy happens to be 
the one previously killed (i.e. dead == 1). This will cause infinite loop 
in xfrm_lookup().

Attached patch is against recent 2.6 BK, although I debugged this problem 
on 2.4 + IPSec backport. From quick look 2.6 still needs this fix (but I 
couldn't test 2.6 on our hw).

Also, I think xfrm_sk_policy_insert() doesn't require similar change, but 
I'm not 100% sure. Could IPSec gurus confirm this? 

Signed-off-by: Eugene Surovegin <ebs@ebshome.net>

===== net/xfrm/xfrm_policy.c 1.52 vs edited =====
--- 1.52/net/xfrm/xfrm_policy.c	2004-07-23 13:23:33 -07:00
+++ edited/net/xfrm/xfrm_policy.c	2004-08-04 18:18:45 -07:00
@@ -536,8 +536,10 @@
 	write_lock_bh(&xfrm_policy_lock);
 	pol = __xfrm_policy_unlink(pol, dir);
 	write_unlock_bh(&xfrm_policy_lock);
-	if (pol)
+	if (pol){
+		atomic_inc(&flow_cache_genid);
 		xfrm_policy_kill(pol);
+	}	
 }
 
 int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol)

Re: [IPSEC] add missing flow_cache_genid update to xfrm_policy_delete()

[Herbert Xu]

Eugene Surovegin <ebs@ebshome.net> wrote:
> 
> Attached patch is against recent 2.6 BK, although I debugged this problem 
> on 2.4 + IPSec backport. From quick look 2.6 still needs this fix (but I 
> couldn't test 2.6 on our hw).

Thanks for the diagnosis and patch.  Looks like I had created yet another
policy timer bug :)

> Also, I think xfrm_sk_policy_insert() doesn't require similar change, but 
> I'm not 100% sure. Could IPSec gurus confirm this? 

Correct.  sk policies are not stored in the flow cache so they don't and
shouldn't cause genid to be incremented.

> ===== net/xfrm/xfrm_policy.c 1.52 vs edited =====
> --- 1.52/net/xfrm/xfrm_policy.c 2004-07-23 13:23:33 -07:00
> +++ edited/net/xfrm/xfrm_policy.c       2004-08-04 18:18:45 -07:00
> @@ -536,8 +536,10 @@
>        write_lock_bh(&xfrm_policy_lock);
>        pol = __xfrm_policy_unlink(pol, dir);
>        write_unlock_bh(&xfrm_policy_lock);
> -       if (pol)
> +       if (pol){
> +               atomic_inc(&flow_cache_genid);

Please add a dir < XFRM_POLICY_MAX check before the atomic_inc so that
dying sockets with policies don't blow away the flow cache.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [IPSEC] add missing flow_cache_genid update to xfrm_policy_delete()

[Eugene Surovegin]

On Thu, Aug 05, 2004 at 10:01:40PM +1000, Herbert Xu wrote:
> Please add a dir < XFRM_POLICY_MAX check before the atomic_inc so that
> dying sockets with policies don't blow away the flow cache.

Ohh, you're right :). Updated patch follows.

Signed-off-by: Eugene Surovegin<ebs@ebshome.net>

===== net/xfrm/xfrm_policy.c 1.52 vs edited =====
--- 1.52/net/xfrm/xfrm_policy.c	2004-07-23 13:23:33 -07:00
+++ edited/net/xfrm/xfrm_policy.c	2004-08-05 08:58:22 -07:00
@@ -536,8 +536,11 @@
 	write_lock_bh(&xfrm_policy_lock);
 	pol = __xfrm_policy_unlink(pol, dir);
 	write_unlock_bh(&xfrm_policy_lock);
-	if (pol)
+	if (pol){
+		if (dir < XFRM_POLICY_MAX)
+			atomic_inc(&flow_cache_genid);
 		xfrm_policy_kill(pol);
+	}	
 }
 
 int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol)

Re: [IPSEC] add missing flow_cache_genid update to xfrm_policy_delete()

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Thu, Aug 05, 2004 at 09:01:44AM -0700, Eugene Surovegin wrote:
> On Thu, Aug 05, 2004 at 10:01:40PM +1000, Herbert Xu wrote:
> > Please add a dir < XFRM_POLICY_MAX check before the atomic_inc so that
> > dying sockets with policies don't blow away the flow cache.
> 
> Ohh, you're right :). Updated patch follows.

Thanks.

Dave, please apply Eugene's patch to fix hard policy expiration.

Signed-off-by: Eugene Surovegin <ebs@ebshome.net>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 507 bytes --]

===== net/xfrm/xfrm_policy.c 1.52 vs edited =====
--- 1.52/net/xfrm/xfrm_policy.c	2004-07-23 13:23:33 -07:00
+++ edited/net/xfrm/xfrm_policy.c	2004-08-05 08:58:22 -07:00
@@ -536,8 +536,11 @@
 	write_lock_bh(&xfrm_policy_lock);
 	pol = __xfrm_policy_unlink(pol, dir);
 	write_unlock_bh(&xfrm_policy_lock);
-	if (pol)
+	if (pol) {
+		if (dir < XFRM_POLICY_MAX)
+			atomic_inc(&flow_cache_genid);
 		xfrm_policy_kill(pol);
+	}
 }
 
 int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol)

Re: [IPSEC] add missing flow_cache_genid update to xfrm_policy_delete()

[David S. Miller]

On Fri, 6 Aug 2004 07:24:21 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Thu, Aug 05, 2004 at 09:01:44AM -0700, Eugene Surovegin wrote:
> > On Thu, Aug 05, 2004 at 10:01:40PM +1000, Herbert Xu wrote:
> > > Please add a dir < XFRM_POLICY_MAX check before the atomic_inc so that
> > > dying sockets with policies don't blow away the flow cache.
> > 
> > Ohh, you're right :). Updated patch follows.
> 
> Thanks.
> 
> Dave, please apply Eugene's patch to fix hard policy expiration.

Done.

Thanks.