pull request (net): ipsec 2014-02-27

1) Build fix for ip_vti when NET_IP_TUNNEL is not set.
   We need this set to have ip_tunnel_get_stats64()
   available.

2) Fix a NULL pointer dereference on sub policy usage.
   We try to access a xfrm_state from the wrong array.

3) Take xfrm_state_lock in xfrm_migrate_state_find(),
   we need it to traverse through the state lists.

4) Clone states properly on migration, otherwise we crash
   when we migrate a state with aead algorithm attached.

5) Fix unlink race when between thread context and timer
   when policies are deleted.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit 960dfc4eb23a28495276b02604d7458e0e1a1ed8:

  Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux (2014-02-18 16:36:07 -0800)

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 3a9016f97fdc8bfbb26ff36ba8f3dc9162eb691b:

  xfrm: Fix unlink race when policies are deleted. (2014-02-26 09:52:02 +0100)

----------------------------------------------------------------
Steffen Klassert (5):
      ip6_vti: Fix build when NET_IP_TUNNEL is not set.
      xfrm: Fix NULL pointer dereference on sub policy usage
      xfrm: Take xfrm_state_lock in xfrm_migrate_state_find
      xfrm: Clone states properly on migration
      xfrm: Fix unlink race when policies are deleted.

 include/net/xfrm.h     |   11 +++++++++++
 net/ipv6/Kconfig       |    1 +
 net/xfrm/xfrm_policy.c |    2 +-
 net/xfrm/xfrm_state.c  |   23 +++++++++++++++++------
 net/xfrm/xfrm_user.c   |    5 -----
 5 files changed, 30 insertions(+), 12 deletions(-)

xfrm_state_sort() takes the unsorted states from the src array
and stores them into the dst array. We try to get the namespace
from the dst array which is empty at this time, so take the
namespace from the src array instead.

Fixes: 283bc9f35bbbc ("xfrm: Namespacify xfrm state/policy locks")
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_state.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index a26b7aa..97d117b 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -1451,7 +1451,7 @@ xfrm_state_sort(struct xfrm_state **dst, struct xfrm_state **src, int n,
 {
 	int err = 0;
 	struct xfrm_state_afinfo *afinfo = xfrm_state_get_afinfo(family);
-	struct net *net = xs_net(*dst);
+	struct net *net = xs_net(*src);
 
 	if (!afinfo)
 		return -EAFNOSUPPORT;

-- 
1.7.9.5

Since commit 469bdcefdc47a ip6_vti uses ip_tunnel_get_stats64(),
so we need to select NET_IP_TUNNEL to have this function available.

Fixes: 469bdcefdc ("ipv6: fix the use of pcpu_tstats in ip6_vti.c")
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv6/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/Kconfig b/net/ipv6/Kconfig
index d92e558..438a73a 100644
--- a/net/ipv6/Kconfig
+++ b/net/ipv6/Kconfig

@@ -138,6 +138,7 @@ config INET6_XFRM_MODE_ROUTEOPTIMIZATION
 config IPV6_VTI
 tristate "Virtual (secure) IPv6: tunneling"
 	select IPV6_TUNNEL
+	select NET_IP_TUNNEL
 	depends on INET6_XFRM_MODE_TUNNEL
 	---help---
 	Tunneling means encapsulating data of one protocol type within

-- 
1.7.9.5

When a policy is unlinked from the lists in thread context,
the xfrm timer can fire before we can mark this policy as dead.
So reinitialize the bydst hlist, then hlist_unhashed() will
notice that this policy is not linked and will avoid a
doulble unlink of that policy.

Reported-by: Xianpeng Zhao <redacted>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4b98b25..1d5c7bf 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c

@@ -1158,7 +1158,7 @@ static struct xfrm_policy *__xfrm_policy_unlink(struct xfrm_policy *pol,
 	if (hlist_unhashed(&pol->bydst))
 		return NULL;
 
-	hlist_del(&pol->bydst);
+	hlist_del_init(&pol->bydst);
 	hlist_del(&pol->byidx);
 	list_del(&pol->walk.all);
 	net->xfrm.policy_count[dir]--;

-- 
1.7.9.5

A comment on xfrm_migrate_state_find() says that xfrm_state_lock
is held. This is apparently not the case, but we need it to
traverse through the state lists.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_state.c |   13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 97d117b..c101023 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -1215,11 +1215,12 @@ out:
 	return NULL;
 }
 
-/* net->xfrm.xfrm_state_lock is held */
 struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net)
 {
 	unsigned int h;
-	struct xfrm_state *x;
+	struct xfrm_state *x = NULL;
+
+	spin_lock_bh(&net->xfrm.xfrm_state_lock);
 
 	if (m->reqid) {
 		h = xfrm_dst_hash(net, &m->old_daddr, &m->old_saddr,

@@ -1236,7 +1237,7 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 					     m->old_family))
 				continue;
 			xfrm_state_hold(x);
-			return x;
+			break;
 		}
 	} else {
 		h = xfrm_src_hash(net, &m->old_daddr, &m->old_saddr,

@@ -1251,11 +1252,13 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 					     m->old_family))
 				continue;
 			xfrm_state_hold(x);
-			return x;
+			break;
 		}
 	}
 
-	return NULL;
+	spin_unlock_bh(&net->xfrm.xfrm_state_lock);
+
+	return x;
 }
 EXPORT_SYMBOL(xfrm_migrate_state_find);
 

-- 
1.7.9.5

We loose a lot of information of the original state if we
clone it with xfrm_state_clone(). In particular, there is
no crypto algorithm attached if the original state uses
an aead algorithm. This patch add the missing information
to the clone state.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/xfrm.h    |   11 +++++++++++
 net/xfrm/xfrm_state.c |    8 ++++++++
 net/xfrm/xfrm_user.c  |    5 -----
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index afa5730..fb5654a 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h

@@ -1648,6 +1648,11 @@ static inline int xfrm_aevent_is_on(struct net *net)
 }
 #endif
 
+static inline int aead_len(struct xfrm_algo_aead *alg)
+{
+	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
+}
+
 static inline int xfrm_alg_len(const struct xfrm_algo *alg)
 {
 	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);

@@ -1686,6 +1691,12 @@ static inline int xfrm_replay_clone(struct xfrm_state *x,
 	return 0;
 }
 
+static inline struct xfrm_algo_aead *xfrm_algo_aead_clone(struct xfrm_algo_aead *orig)
+{
+	return kmemdup(orig, aead_len(orig), GFP_KERNEL);
+}
+
+
 static inline struct xfrm_algo *xfrm_algo_clone(struct xfrm_algo *orig)
 {
 	return kmemdup(orig, xfrm_alg_len(orig), GFP_KERNEL);

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index c101023..40f1b3e 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -1159,6 +1159,11 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
 	}
 	x->props.aalgo = orig->props.aalgo;
 
+	if (orig->aead) {
+		x->aead = xfrm_algo_aead_clone(orig->aead);
+		if (!x->aead)
+			goto error;
+	}
 	if (orig->ealg) {
 		x->ealg = xfrm_algo_clone(orig->ealg);
 		if (!x->ealg)

@@ -1201,6 +1206,9 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig, int *errp)
 	x->props.flags = orig->props.flags;
 	x->props.extra_flags = orig->props.extra_flags;
 
+	x->tfcpad = orig->tfcpad;
+	x->replay_maxdiff = orig->replay_maxdiff;
+	x->replay_maxage = orig->replay_maxage;
 	x->curlft.add_time = orig->curlft.add_time;
 	x->km.state = orig->km.state;
 	x->km.seq = orig->km.seq;

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 1ae3ec7..c274179 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c

@@ -32,11 +32,6 @@
 #include <linux/in6.h>
 #endif
 
-static inline int aead_len(struct xfrm_algo_aead *alg)
-{
-	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
-}
-
 static int verify_one_alg(struct nlattr **attrs, enum xfrm_attr_type_t type)
 {
 	struct nlattr *rt = attrs[type];

-- 
1.7.9.5

On Thu, Feb 27, 2014 at 07:16:39AM +0100, Steffen Klassert wrote:

When a policy is unlinked from the lists in thread context,
the xfrm timer can fire before we can mark this policy as dead.
So reinitialize the bydst hlist, then hlist_unhashed() will
notice that this policy is not linked and will avoid a
doulble unlink of that policy.

Reported-by: Xianpeng Zhao <redacted>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

If this race condition is real then I don't see how this fix
can be correct since there aren't any barriers between the check
and the unlink.

Cheers,
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Thu, Feb 27, 2014 at 02:27:12PM +0800, Herbert Xu wrote:

On Thu, Feb 27, 2014 at 07:16:39AM +0100, Steffen Klassert wrote:

quoted

When a policy is unlinked from the lists in thread context,
the xfrm timer can fire before we can mark this policy as dead.
So reinitialize the bydst hlist, then hlist_unhashed() will
notice that this policy is not linked and will avoid a
doulble unlink of that policy.

Reported-by: Xianpeng Zhao <redacted>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

If this race condition is real then I don't see how this fix
can be correct since there aren't any barriers between the check
and the unlink.

Looking at the code it seems that we've already taken the policy
lock so how can this race occur?

Cheers,
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Thu, Feb 27, 2014 at 02:30:15PM +0800, Herbert Xu wrote:

On Thu, Feb 27, 2014 at 02:27:12PM +0800, Herbert Xu wrote:

quoted

On Thu, Feb 27, 2014 at 07:16:39AM +0100, Steffen Klassert wrote:

quoted

When a policy is unlinked from the lists in thread context,
the xfrm timer can fire before we can mark this policy as dead.
So reinitialize the bydst hlist, then hlist_unhashed() will
notice that this policy is not linked and will avoid a
doulble unlink of that policy.

Reported-by: Xianpeng Zhao <redacted>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

If this race condition is real then I don't see how this fix
can be correct since there aren't any barriers between the check
and the unlink.

Looking at the code it seems that we've already taken the policy
lock so how can this race occur?

We take the policy lock in xfrm_policy_delete() before we
unlink the policy. But we release it before we mark the
policy as dead with xfrm_policy_kill(). If the timer fires
in the meantime it can be unlinked from the timer first
and then again from xfrm_policy_delete().

This issue was originally reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=70471

On Thu, Feb 27, 2014 at 07:53:53AM +0100, Steffen Klassert wrote:

On Thu, Feb 27, 2014 at 02:30:15PM +0800, Herbert Xu wrote:

quoted

On Thu, Feb 27, 2014 at 02:27:12PM +0800, Herbert Xu wrote:

quoted

On Thu, Feb 27, 2014 at 07:16:39AM +0100, Steffen Klassert wrote:

quoted

When a policy is unlinked from the lists in thread context,
the xfrm timer can fire before we can mark this policy as dead.
So reinitialize the bydst hlist, then hlist_unhashed() will
notice that this policy is not linked and will avoid a
doulble unlink of that policy.

Reported-by: Xianpeng Zhao <redacted>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

If this race condition is real then I don't see how this fix
can be correct since there aren't any barriers between the check
and the unlink.

Looking at the code it seems that we've already taken the policy
lock so how can this race occur?

We take the policy lock in xfrm_policy_delete() before we
unlink the policy. But we release it before we mark the
policy as dead with xfrm_policy_kill(). If the timer fires
in the meantime it can be unlinked from the timer first
and then again from xfrm_policy_delete().

This issue was originally reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=70471

Right, I see what you mean.

Thanks for the explanation!
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Thu, 27 Feb 2014 07:16:34 +0100

1) Build fix for ip_vti when NET_IP_TUNNEL is not set.
   We need this set to have ip_tunnel_get_stats64()
   available.

2) Fix a NULL pointer dereference on sub policy usage.
   We try to access a xfrm_state from the wrong array.

3) Take xfrm_state_lock in xfrm_migrate_state_find(),
   we need it to traverse through the state lists.

4) Clone states properly on migration, otherwise we crash
   when we migrate a state with aead algorithm attached.

5) Fix unlink race when between thread context and timer
   when policies are deleted.

Looks good, pulled, thanks Steffen.