pull request (net): ipsec 2014-07-23

pull request (net): ipsec 2014-07-23

[Steffen Klassert]

Just two fixes this time, both are stable candidates.

1) Fix the dst_entry refcount on socket policy usage.

2) Fix a wrong SPI check that prevents AH SAs from getting
   installed, dependent on the SPI. From Tobias Brunner. 

Please pull or let me know if there are problems.

Thanks!

The following changes since commit d7933ab727ed035bdf420d7381b831ba959cecc5:

  Merge branch 'for-next' of git://git.samba.org/sfrench/cifs-2.6 (2014-06-25 21:47:28 -0700)

are available in the git repository at:


  git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master

for you to fetch changes up to a0e5ef53aac8e5049f9344857d8ec5237d31e58b:

  xfrm: Fix installation of AH IPsec SAs (2014-06-30 07:42:12 +0200)

----------------------------------------------------------------
Steffen Klassert (1):
      xfrm: Fix refcount imbalance in xfrm_lookup

Tobias Brunner (1):
      xfrm: Fix installation of AH IPsec SAs

 net/xfrm/xfrm_policy.c | 2 ++
 net/xfrm/xfrm_user.c   | 7 +++----
 2 files changed, 5 insertions(+), 4 deletions(-)

[PATCH 1/2] xfrm: Fix refcount imbalance in xfrm_lookup

[Steffen Klassert]

xfrm_lookup must return a dst_entry with a refcount for the caller.
Git commit 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
removed this refcount for the socket policy case accidentally.
This patch restores it and sets DST_NOCACHE flag to make sure
that the dst_entry is freed when the refcount becomes null.

Fixes: 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index a8ef510..0525d78 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2097,6 +2097,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
 				goto no_transform;
 			}
 
+			dst_hold(&xdst->u.dst);
+			xdst->u.dst.flags |= DST_NOCACHE;
 			route = xdst->route;
 		}
 	}
-- 
1.9.1

Re: [PATCH 1/2] xfrm: Fix refcount imbalance in xfrm_lookup

[Eric Dumazet]

On Wed, 2014-07-23 at 10:18 +0200, Steffen Klassert wrote:
> xfrm_lookup must return a dst_entry with a refcount for the caller.
> Git commit 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
> removed this refcount for the socket policy case accidentally.
> This patch restores it and sets DST_NOCACHE flag to make sure
> that the dst_entry is freed when the refcount becomes null.
> 
> Fixes: 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  net/xfrm/xfrm_policy.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index a8ef510..0525d78 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -2097,6 +2097,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
>  				goto no_transform;
>  			}
>  
> +			dst_hold(&xdst->u.dst);
> +			xdst->u.dst.flags |= DST_NOCACHE;
>  			route = xdst->route;
>  		}
>  	}


Hmm... are you sure ?

Before 1a1ccc96abb we did a dst_hold() because we kept the dst in
xfrm_policy_sk_bundles. So keeping a reference count was mandatory.

But after, I don't understand why it is needed at all.

Re: [PATCH 1/2] xfrm: Fix refcount imbalance in xfrm_lookup

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 23 Jul 2014 19:21:13 +0200

> On Wed, 2014-07-23 at 10:18 +0200, Steffen Klassert wrote:
>> xfrm_lookup must return a dst_entry with a refcount for the caller.
>> Git commit 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
>> removed this refcount for the socket policy case accidentally.
>> This patch restores it and sets DST_NOCACHE flag to make sure
>> that the dst_entry is freed when the refcount becomes null.
>> 
>> Fixes: 1a1ccc96abb ("xfrm: Remove caching of xfrm_policy_sk_bundles")
>> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
>> ---
>>  net/xfrm/xfrm_policy.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
>> index a8ef510..0525d78 100644
>> --- a/net/xfrm/xfrm_policy.c
>> +++ b/net/xfrm/xfrm_policy.c
>> @@ -2097,6 +2097,8 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
>>  				goto no_transform;
>>  			}
>>  
>> +			dst_hold(&xdst->u.dst);
>> +			xdst->u.dst.flags |= DST_NOCACHE;
>>  			route = xdst->route;
>>  		}
>>  	}
> 
> 
> Hmm... are you sure ?
> 
> Before 1a1ccc96abb we did a dst_hold() because we kept the dst in
> xfrm_policy_sk_bundles. So keeping a reference count was mandatory.
> 
> But after, I don't understand why it is needed at all.

It should be needed.

We create a new bundle here, from scratch, based upon the socket
policy.

Such new bundles make routes with dst refcount == 0.

All callers expect that the route given back to them is either
the original 'dst' passed into xfrm_lookup() unmolested, or a
new 'dst' having a refcount taken for this caller.

That's why all call sites expect that they can just go
"dst_release(dst)" and these objects it works.

So, if we returned 'dst' with refcount == 0 it would not work.

Re: [PATCH 1/2] xfrm: Fix refcount imbalance in xfrm_lookup

[Eric Dumazet]

On Wed, 2014-07-23 at 15:09 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>

> > Hmm... are you sure ?
> > 
> > Before 1a1ccc96abb we did a dst_hold() because we kept the dst in
> > xfrm_policy_sk_bundles. So keeping a reference count was mandatory.
> > 
> > But after, I don't understand why it is needed at all.
> 
> It should be needed.
> 
> We create a new bundle here, from scratch, based upon the socket
> policy.
> 
> Such new bundles make routes with dst refcount == 0.
> 
> All callers expect that the route given back to them is either
> the original 'dst' passed into xfrm_lookup() unmolested, or a
> new 'dst' having a refcount taken for this caller.
> 
> That's why all call sites expect that they can just go
> "dst_release(dst)" and these objects it works.
> 
> So, if we returned 'dst' with refcount == 0 it would not work.

OK, I understood, thanks !

[PATCH 2/2] xfrm: Fix installation of AH IPsec SAs

[Steffen Klassert]

From: Tobias Brunner <tobias@strongswan.org>

The SPI check introduced in ea9884b3acf3311c8a11db67bfab21773f6f82ba
was intended for IPComp SAs but actually prevented AH SAs from getting
installed (depending on the SPI).

Fixes: ea9884b3acf3 ("xfrm: check user specified spi for IPComp")
Cc: Fan Du <fan.du@windriver.com>
Signed-off-by: Tobias Brunner <tobias@strongswan.org>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 412d9dc..d4db6eb 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -177,9 +177,7 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 		    attrs[XFRMA_ALG_AEAD]	||
 		    attrs[XFRMA_ALG_CRYPT]	||
 		    attrs[XFRMA_ALG_COMP]	||
-		    attrs[XFRMA_TFCPAD]		||
-		    (ntohl(p->id.spi) >= 0x10000))
-
+		    attrs[XFRMA_TFCPAD])
 			goto out;
 		break;
 
@@ -207,7 +205,8 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 		    attrs[XFRMA_ALG_AUTH]	||
 		    attrs[XFRMA_ALG_AUTH_TRUNC]	||
 		    attrs[XFRMA_ALG_CRYPT]	||
-		    attrs[XFRMA_TFCPAD])
+		    attrs[XFRMA_TFCPAD]		||
+		    (ntohl(p->id.spi) >= 0x10000))
 			goto out;
 		break;
 
-- 
1.9.1

Re: pull request (net): ipsec 2014-07-23

[David Miller]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Wed, 23 Jul 2014 10:18:05 +0200

> Just two fixes this time, both are stable candidates.
> 
> 1) Fix the dst_entry refcount on socket policy usage.
> 
> 2) Fix a wrong SPI check that prevents AH SAs from getting
>    installed, dependent on the SPI. From Tobias Brunner. 
> 
> Please pull or let me know if there are problems.

Pulled, and both patches queued up for -stable, thanks.