[0/7] IPsec: More work on async crypto on output

[0/7] IPsec: More work on async crypto on output

[Herbert Xu]

Hi Dave:

Here's another round of patches which ends with the moving down
of the state lock into x->type->output.  I need to stop getting
distracted by fixing bugs and concentrate on creating them :)

Cheers,
-- 
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

[PATCH 1/7] [IPSEC]: Remove bogus ref count in xfrm_secpath_reject

[Herbert Xu]

[IPSEC]: Remove bogus ref count in xfrm_secpath_reject

Constructs of the form

	xfrm_state_hold(x);
	foo(x);
	xfrm_state_put(x);

tend to be broken because foo is either synchronous where this is totally
unnecessary or if foo is asynchronous then the reference count is in the
wrong spot.

In the case of xfrm_secpath_reject, the function is synchronous and therefore
we should just kill the reference count.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/xfrm/xfrm_policy.c |    6 +-----
 1 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 76f172f..af27c19 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1682,17 +1682,13 @@ static inline int
 xfrm_secpath_reject(int idx, struct sk_buff *skb, struct flowi *fl)
 {
 	struct xfrm_state *x;
-	int err;
 
 	if (!skb->sp || idx < 0 || idx >= skb->sp->len)
 		return 0;
 	x = skb->sp->xvec[idx];
 	if (!x->type->reject)
 		return 0;
-	xfrm_state_hold(x);
-	err = x->type->reject(x, skb, fl);
-	xfrm_state_put(x);
-	return err;
+	return x->type->reject(x, skb, fl);
 }
 
 /* When skb is transformed back to its "native" form, we have to

Re: [PATCH 1/7] [IPSEC]: Remove bogus ref count in xfrm_secpath_reject

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:31 +0800

> [IPSEC]: Remove bogus ref count in xfrm_secpath_reject
> 
> Constructs of the form
> 
> 	xfrm_state_hold(x);
> 	foo(x);
> 	xfrm_state_put(x);
> 
> tend to be broken because foo is either synchronous where this is totally
> unnecessary or if foo is asynchronous then the reference count is in the
> wrong spot.
> 
> In the case of xfrm_secpath_reject, the function is synchronous and therefore
> we should just kill the reference count.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks!

[PATCH 2/7] [IPSEC]: Store IPv6 nh pointer in mac_header on output

[Herbert Xu]

[IPSEC]: Store IPv6 nh pointer in mac_header on output

Current the x->mode->output functions store the IPv6 nh pointer in the
skb network header.  This is inconvenient because the network header then
has to be fixed up before the packet can leave the IPsec stack.  The mac
header field is unused on output so we can use that to store this instead.

This patch does that and removes the network header fix-up in xfrm_output.

It also uses ipv6_hdr where appropriate in the x->type->output functions.

There is also a minor clean-up in esp4 to make it use the same code as
esp6 to help any subsequent effort to merge the two.

Lastly it kills two redundant skb_set_* statements in BEET that were
simply copied over from transport mode.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/ipv4/esp4.c                 |    2 +-
 net/ipv6/ah6.c                  |    6 +++---
 net/ipv6/esp6.c                 |    7 ++++---
 net/ipv6/ipcomp6.c              |    6 +++---
 net/ipv6/mip6.c                 |   12 ++++++------
 net/ipv6/xfrm6_mode_beet.c      |   16 +++++++---------
 net/ipv6/xfrm6_mode_ro.c        |   13 +++++++------
 net/ipv6/xfrm6_mode_transport.c |   13 +++++++------
 net/ipv6/xfrm6_mode_tunnel.c    |   13 +++++++------
 net/ipv6/xfrm6_tunnel.c         |    2 +-
 net/xfrm/xfrm_output.c          |    2 --
 11 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 0f62af9..ffd5653 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -59,7 +59,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	tail[clen - skb->len - 2] = (clen - skb->len) - 2;
 	pskb_put(skb, trailer, clen - skb->len);
 
-	__skb_push(skb, skb->data - skb_network_header(skb));
+	__skb_push(skb, -skb_network_offset(skb));
 	top_iph = ip_hdr(skb);
 	esph = (struct ip_esp_hdr *)(skb_network_header(skb) +
 				     top_iph->ihl * 4);
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index ae68a90..ff904a7 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -235,11 +235,11 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 		char hdrs[0];
 	} *tmp_ext;
 
-	top_iph = (struct ipv6hdr *)skb->data;
+	top_iph = ipv6_hdr(skb);
 	top_iph->payload_len = htons(skb->len - sizeof(*top_iph));
 
-	nexthdr = *skb_network_header(skb);
-	*skb_network_header(skb) = IPPROTO_AH;
+	nexthdr = *skb_mac_header(skb);
+	*skb_mac_header(skb) = IPPROTO_AH;
 
 	/* When there are no extension headers, we only need to save the first
 	 * 8 bytes of the base IP header.
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 0c5fb81..9fc1940 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -88,11 +88,12 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	tail[clen-skb->len - 2] = (clen - skb->len) - 2;
 	pskb_put(skb, trailer, clen - skb->len);
 
-	top_iph = (struct ipv6hdr *)__skb_push(skb, hdr_len);
+	__skb_push(skb, -skb_network_offset(skb));
+	top_iph = ipv6_hdr(skb);
 	esph = (struct ipv6_esp_hdr *)skb_transport_header(skb);
 	top_iph->payload_len = htons(skb->len + alen - sizeof(*top_iph));
-	*(skb_tail_pointer(trailer) - 1) = *skb_network_header(skb);
-	*skb_network_header(skb) = IPPROTO_ESP;
+	*(skb_tail_pointer(trailer) - 1) = *skb_mac_header(skb);
+	*skb_mac_header(skb) = IPPROTO_ESP;
 
 	esph->spi = x->id.spi;
 	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 91b2a75..71a14c0 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -157,15 +157,15 @@ static int ipcomp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	pskb_trim(skb, hdr_len + dlen + sizeof(struct ip_comp_hdr));
 
 	/* insert ipcomp header and replace datagram */
-	top_iph = (struct ipv6hdr *)skb->data;
+	top_iph = ipv6_hdr(skb);
 
 	top_iph->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
 
 	ipch = (struct ipv6_comp_hdr *)start;
-	ipch->nexthdr = *skb_network_header(skb);
+	ipch->nexthdr = *skb_mac_header(skb);
 	ipch->flags = 0;
 	ipch->cpi = htons((u16 )ntohl(x->id.spi));
-	*skb_network_header(skb) = IPPROTO_COMP;
+	*skb_mac_header(skb) = IPPROTO_COMP;
 
 out_ok:
 	return 0;
diff --git a/net/ipv6/mip6.c b/net/ipv6/mip6.c
index 8a1399c..7261c29 100644
--- a/net/ipv6/mip6.c
+++ b/net/ipv6/mip6.c
@@ -153,11 +153,11 @@ static int mip6_destopt_output(struct xfrm_state *x, struct sk_buff *skb)
 	u8 nexthdr;
 	int len;
 
-	iph = (struct ipv6hdr *)skb->data;
+	iph = ipv6_hdr(skb);
 	iph->payload_len = htons(skb->len - sizeof(*iph));
 
-	nexthdr = *skb_network_header(skb);
-	*skb_network_header(skb) = IPPROTO_DSTOPTS;
+	nexthdr = *skb_mac_header(skb);
+	*skb_mac_header(skb) = IPPROTO_DSTOPTS;
 
 	dstopt = (struct ipv6_destopt_hdr *)skb_transport_header(skb);
 	dstopt->nexthdr = nexthdr;
@@ -365,11 +365,11 @@ static int mip6_rthdr_output(struct xfrm_state *x, struct sk_buff *skb)
 	struct rt2_hdr *rt2;
 	u8 nexthdr;
 
-	iph = (struct ipv6hdr *)skb->data;
+	iph = ipv6_hdr(skb);
 	iph->payload_len = htons(skb->len - sizeof(*iph));
 
-	nexthdr = *skb_network_header(skb);
-	*skb_network_header(skb) = IPPROTO_ROUTING;
+	nexthdr = *skb_mac_header(skb);
+	*skb_mac_header(skb) = IPPROTO_ROUTING;
 
 	rt2 = (struct rt2_hdr *)skb_transport_header(skb);
 	rt2->rt_hdr.nexthdr = nexthdr;
diff --git a/net/ipv6/xfrm6_mode_beet.c b/net/ipv6/xfrm6_mode_beet.c
index 2e61d6d..65e6b2a 100644
--- a/net/ipv6/xfrm6_mode_beet.c
+++ b/net/ipv6/xfrm6_mode_beet.c
@@ -26,10 +26,11 @@
  *	payload_len
  *
  * On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header.  The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
  */
 static int xfrm6_beet_output(struct xfrm_state *x, struct sk_buff *skb)
 {
@@ -41,15 +42,12 @@ static int xfrm6_beet_output(struct xfrm_state *x, struct sk_buff *skb)
 	iph = ipv6_hdr(skb);
 
 	hdr_len = ip6_find_1stfragopt(skb, &prevhdr);
-	skb_set_network_header(skb,
-			       (prevhdr - x->props.header_len) - skb->data);
-	skb_set_transport_header(skb, hdr_len);
 	memmove(skb->data, iph, hdr_len);
 
+	skb_set_mac_header(skb, offsetof(struct ipv6hdr, nexthdr));
 	skb_reset_network_header(skb);
+	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
 	top_iph = ipv6_hdr(skb);
-	skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
-	skb->network_header += offsetof(struct ipv6hdr, nexthdr);
 
 	ipv6_addr_copy(&top_iph->saddr, (struct in6_addr *)&x->props.saddr);
 	ipv6_addr_copy(&top_iph->daddr, (struct in6_addr *)&x->id.daddr);
diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c
index a156373..2575804 100644
--- a/net/ipv6/xfrm6_mode_ro.c
+++ b/net/ipv6/xfrm6_mode_ro.c
@@ -39,10 +39,11 @@
  * space for the route optimization header.
  *
  * On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header.  The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
  */
 static int xfrm6_ro_output(struct xfrm_state *x, struct sk_buff *skb)
 {
@@ -54,8 +55,8 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct sk_buff *skb)
 	iph = ipv6_hdr(skb);
 
 	hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
-	skb_set_network_header(skb,
-			       (prevhdr - x->props.header_len) - skb->data);
+	skb_set_mac_header(skb, (prevhdr - x->props.header_len) - skb->data);
+	skb_reset_network_header(skb);
 	skb_set_transport_header(skb, hdr_len);
 	memmove(skb->data, iph, hdr_len);
 
diff --git a/net/ipv6/xfrm6_mode_transport.c b/net/ipv6/xfrm6_mode_transport.c
index c026bfe..65c166b 100644
--- a/net/ipv6/xfrm6_mode_transport.c
+++ b/net/ipv6/xfrm6_mode_transport.c
@@ -20,10 +20,11 @@
  * space for the encapsulation header.
  *
  * On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header.  The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
  */
 static int xfrm6_transport_output(struct xfrm_state *x, struct sk_buff *skb)
 {
@@ -35,8 +36,8 @@ static int xfrm6_transport_output(struct xfrm_state *x, struct sk_buff *skb)
 	iph = ipv6_hdr(skb);
 
 	hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
-	skb_set_network_header(skb,
-			       (prevhdr - x->props.header_len) - skb->data);
+	skb_set_mac_header(skb, (prevhdr - x->props.header_len) - skb->data);
+	skb_reset_network_header(skb);
 	skb_set_transport_header(skb, hdr_len);
 	memmove(skb->data, iph, hdr_len);
 	return 0;
diff --git a/net/ipv6/xfrm6_mode_tunnel.c b/net/ipv6/xfrm6_mode_tunnel.c
index 9fc95bc..3dd40af 100644
--- a/net/ipv6/xfrm6_mode_tunnel.c
+++ b/net/ipv6/xfrm6_mode_tunnel.c
@@ -38,10 +38,11 @@ static inline void ip6ip_ecn_decapsulate(struct sk_buff *skb)
  *	payload_len
  *
  * On exit, skb->h will be set to the start of the encapsulation header to be
- * filled in by x->type->output and skb->nh will be set to the nextheader field
- * of the extension header directly preceding the encapsulation header, or in
- * its absence, that of the top IP header.  The value of skb->data will always
- * point to the top IP header.
+ * filled in by x->type->output and the mac header will be set to the
+ * nextheader field of the extension header directly preceding the
+ * encapsulation header, or in its absence, that of the top IP header.
+ * The value of skb->data and the network header will always point to the
+ * top IP header.
  */
 static int xfrm6_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
 {
@@ -53,10 +54,10 @@ static int xfrm6_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
 	skb_push(skb, x->props.header_len);
 	iph = ipv6_hdr(skb);
 
+	skb_set_mac_header(skb, offsetof(struct ipv6hdr, nexthdr));
 	skb_reset_network_header(skb);
+	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
 	top_iph = ipv6_hdr(skb);
-	skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
-	skb->network_header   += offsetof(struct ipv6hdr, nexthdr);
 
 	top_iph->version = 6;
 	if (xdst->route->ops->family == AF_INET6) {
diff --git a/net/ipv6/xfrm6_tunnel.c b/net/ipv6/xfrm6_tunnel.c
index 30f3236..aeb0607 100644
--- a/net/ipv6/xfrm6_tunnel.c
+++ b/net/ipv6/xfrm6_tunnel.c
@@ -244,7 +244,7 @@ static int xfrm6_tunnel_output(struct xfrm_state *x, struct sk_buff *skb)
 {
 	struct ipv6hdr *top_iph;
 
-	top_iph = (struct ipv6hdr *)skb->data;
+	top_iph = ipv6_hdr(skb);
 	top_iph->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
 
 	return 0;
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 8c85211..9847bae 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -76,8 +76,6 @@ int xfrm_output(struct sk_buff *skb)
 
 		spin_unlock_bh(&x->lock);
 
-		skb_reset_network_header(skb);
-
 		if (!(skb->dst = dst_pop(dst))) {
 			err = -EHOSTUNREACH;
 			goto error_nolock;

Re: [PATCH 2/7] [IPSEC]: Store IPv6 nh pointer in mac_header on output

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:32 +0800

> [IPSEC]: Store IPv6 nh pointer in mac_header on output
> 
> Current the x->mode->output functions store the IPv6 nh pointer in the
> skb network header.  This is inconvenient because the network header then
> has to be fixed up before the packet can leave the IPsec stack.  The mac
> header field is unused on output so we can use that to store this instead.
> 
> This patch does that and removes the network header fix-up in xfrm_output.
> 
> It also uses ipv6_hdr where appropriate in the x->type->output functions.
> 
> There is also a minor clean-up in esp4 to make it use the same code as
> esp6 to help any subsequent effort to merge the two.
> 
> Lastly it kills two redundant skb_set_* statements in BEET that were
> simply copied over from transport mode.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks Herbert.

[PATCH 3/7] [IPSEC]: Remove gratuitous km wake-up events on ACQUIRE

[Herbert Xu]

[IPSEC]: Remove gratuitous km wake-up events on ACQUIRE

There is no point in waking people up when creating/updating larval states
because they'll just go back to sleep again as larval states by definition
cannot be found by xfrm_state_find.

We should only wake them up when the larvals mature or die.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/xfrm/xfrm_state.c |    2 --
 1 files changed, 2 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index a00745a..0d07f6b 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -848,7 +848,6 @@ static struct xfrm_state *__find_acq_core(unsigned short family, u8 mode, u32 re
 		hlist_add_head(&x->bydst, xfrm_state_bydst+h);
 		h = xfrm_src_hash(daddr, saddr, family);
 		hlist_add_head(&x->bysrc, xfrm_state_bysrc+h);
-		wake_up(&km_waitq);
 
 		xfrm_state_num++;
 
@@ -1311,7 +1310,6 @@ xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi)
 		h = xfrm_spi_hash(&x->id.daddr, x->id.spi, x->id.proto, x->props.family);
 		hlist_add_head(&x->byspi, xfrm_state_byspi+h);
 		spin_unlock_bh(&xfrm_state_lock);
-		wake_up(&km_waitq);
 	}
 }
 EXPORT_SYMBOL(xfrm_alloc_spi);

Re: [PATCH 3/7] [IPSEC]: Remove gratuitous km wake-up events on ACQUIRE

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:33 +0800

> [IPSEC]: Remove gratuitous km wake-up events on ACQUIRE
> 
> There is no point in waking people up when creating/updating larval states
> because they'll just go back to sleep again as larval states by definition
> cannot be found by xfrm_state_find.
> 
> We should only wake them up when the larvals mature or die.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks!

[PATCH 4/7] [IPSEC]: Move common code into xfrm_alloc_spi

[Herbert Xu]

[IPSEC]: Move common code into xfrm_alloc_spi

This patch moves some common code that conceptually belongs to the xfrm core
from af_key/xfrm_user into xfrm_alloc_spi.

In particular, the spin lock on the state is now taken inside xfrm_alloc_spi.
Previously it also protected the construction of the response PF_KEY/XFRM
messages to user-space.  This is inconsistent as other identical constructions
are not protected by the state lock.  This is bad because they in fact should
be protected but only in certain spots (so as not to hold the lock for too
long which may cause packet drops).

The SPI byte order conversion has also been moved.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 include/net/xfrm.h    |    2 +-
 net/key/af_key.c      |   29 ++++++++++++-----------------
 net/xfrm/xfrm_state.c |   26 ++++++++++++++++++++------
 net/xfrm/xfrm_user.c  |   13 ++++---------
 4 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 064a4ca..1c116dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1084,7 +1084,7 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(u8 type, int dir,
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
 int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
-void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
+extern int xfrm_alloc_spi(struct xfrm_state *x, u32 minspi, u32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index ff5c3d0..143d46f 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1253,8 +1253,11 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, struct sadb_msg *h
 	struct sadb_x_sa2 *sa2;
 	struct sadb_address *saddr, *daddr;
 	struct sadb_msg *out_hdr;
+	struct sadb_spirange *range;
 	struct xfrm_state *x = NULL;
 	int mode;
+	int err;
+	u32 min_spi, max_spi;
 	u32 reqid;
 	u8 proto;
 	unsigned short family;
@@ -1309,25 +1312,17 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, struct sadb_msg *h
 	if (x == NULL)
 		return -ENOENT;
 
-	resp_skb = ERR_PTR(-ENOENT);
-
-	spin_lock_bh(&x->lock);
-	if (x->km.state != XFRM_STATE_DEAD) {
-		struct sadb_spirange *range = ext_hdrs[SADB_EXT_SPIRANGE-1];
-		u32 min_spi, max_spi;
+	min_spi = 0x100;
+	max_spi = 0x0fffffff;
 
-		if (range != NULL) {
-			min_spi = range->sadb_spirange_min;
-			max_spi = range->sadb_spirange_max;
-		} else {
-			min_spi = 0x100;
-			max_spi = 0x0fffffff;
-		}
-		xfrm_alloc_spi(x, htonl(min_spi), htonl(max_spi));
-		if (x->id.spi)
-			resp_skb = pfkey_xfrm_state2msg(x, 0, 3);
+	range = ext_hdrs[SADB_EXT_SPIRANGE-1];
+	if (range) {
+		min_spi = range->sadb_spirange_min;
+		max_spi = range->sadb_spirange_max;
 	}
-	spin_unlock_bh(&x->lock);
+
+	err = xfrm_alloc_spi(x, min_spi, max_spi);
+	resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x, 0, 3);
 
 	if (IS_ERR(resp_skb)) {
 		xfrm_state_put(x);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 0d07f6b..344f0a6 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1275,26 +1275,33 @@ u32 xfrm_get_acqseq(void)
 }
 EXPORT_SYMBOL(xfrm_get_acqseq);
 
-void
-xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi)
+int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
 {
 	unsigned int h;
 	struct xfrm_state *x0;
+	int err = -ENOENT;
+	__be32 minspi = htonl(low);
+	__be32 maxspi = htonl(high);
+
+	spin_lock_bh(&x->lock);
+	if (x->km.state == XFRM_STATE_DEAD)
+		goto unlock;
 
+	err = 0;
 	if (x->id.spi)
-		return;
+		goto unlock;
+
+	err = -ENOENT;
 
 	if (minspi == maxspi) {
 		x0 = xfrm_state_lookup(&x->id.daddr, minspi, x->id.proto, x->props.family);
 		if (x0) {
 			xfrm_state_put(x0);
-			return;
+			goto unlock;
 		}
 		x->id.spi = minspi;
 	} else {
 		u32 spi = 0;
-		u32 low = ntohl(minspi);
-		u32 high = ntohl(maxspi);
 		for (h=0; h<high-low+1; h++) {
 			spi = low + net_random()%(high-low+1);
 			x0 = xfrm_state_lookup(&x->id.daddr, htonl(spi), x->id.proto, x->props.family);
@@ -1310,7 +1317,14 @@ xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi)
 		h = xfrm_spi_hash(&x->id.daddr, x->id.spi, x->id.proto, x->props.family);
 		hlist_add_head(&x->byspi, xfrm_state_byspi+h);
 		spin_unlock_bh(&xfrm_state_lock);
+
+		err = 0;
 	}
+
+unlock:
+	spin_unlock_bh(&x->lock);
+
+	return err;
 }
 EXPORT_SYMBOL(xfrm_alloc_spi);
 
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8e10e90..52c7fce 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -784,16 +784,11 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (x == NULL)
 		goto out_noput;
 
-	resp_skb = ERR_PTR(-ENOENT);
-
-	spin_lock_bh(&x->lock);
-	if (x->km.state != XFRM_STATE_DEAD) {
-		xfrm_alloc_spi(x, htonl(p->min), htonl(p->max));
-		if (x->id.spi)
-			resp_skb = xfrm_state_netlink(skb, x, nlh->nlmsg_seq);
-	}
-	spin_unlock_bh(&x->lock);
+	err = xfrm_alloc_spi(x, p->min, p->max);
+	if (err)
+		goto out;
 
+	resp_skb = xfrm_state_netlink(skb, x, nlh->nlmsg_seq);
 	if (IS_ERR(resp_skb)) {
 		err = PTR_ERR(resp_skb);
 		goto out;

Re: [PATCH 4/7] [IPSEC]: Move common code into xfrm_alloc_spi

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:34 +0800

> [IPSEC]: Move common code into xfrm_alloc_spi
> 
> This patch moves some common code that conceptually belongs to the xfrm core
> from af_key/xfrm_user into xfrm_alloc_spi.
> 
> In particular, the spin lock on the state is now taken inside xfrm_alloc_spi.
> Previously it also protected the construction of the response PF_KEY/XFRM
> messages to user-space.  This is inconsistent as other identical constructions
> are not protected by the state lock.  This is bad because they in fact should
> be protected but only in certain spots (so as not to hold the lock for too
> long which may cause packet drops).
> 
> The SPI byte order conversion has also been moved.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks!

[PATCH 5/7] [XFRM] user: Move attribute copying code into copy_to_user_state_extra

[Herbert Xu]

[XFRM] user: Move attribute copying code into copy_to_user_state_extra

Here's a good example of code duplication leading to code rot.  The
notification patch did its own netlink message creation for xfrm states.
It duplicated code that was already in dump_one_state.  Guess what, the
next time (and the time after) when someone updated dump_one_state the
notification path got zilch.

This patch moves that code from dump_one_state to copy_to_user_state_extra
and uses it in xfrm_notify_sa too.  Unfortunately whoever updates this
still needs to update xfrm_sa_len since the notification path wants to
know the exact size for allocation.

At least I've added a comment saying so and if someone still forgest, we'll
have a WARN_ON telling us so.

I also changed the security size calculation to use xfrm_user_sec_ctx since
that's what we actually put into the skb.  However it makes no practical
difference since it has the same size as xfrm_sec_ctx.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/xfrm/xfrm_user.c |   76 +++++++++++++++++++++++++++++++--------------------
 1 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 52c7fce..2cbbe5e 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -483,9 +483,9 @@ struct xfrm_dump_info {
 
 static int copy_sec_ctx(struct xfrm_sec_ctx *s, struct sk_buff *skb)
 {
-	int ctx_size = sizeof(struct xfrm_sec_ctx) + s->ctx_len;
 	struct xfrm_user_sec_ctx *uctx;
 	struct nlattr *attr;
+	int ctx_size = sizeof(*uctx) + s->ctx_len;
 
 	attr = nla_reserve(skb, XFRMA_SEC_CTX, ctx_size);
 	if (attr == NULL)
@@ -502,23 +502,11 @@ static int copy_sec_ctx(struct xfrm_sec_ctx *s, struct sk_buff *skb)
 	return 0;
 }
 
-static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
+/* Don't change this without updating xfrm_sa_len! */
+static int copy_to_user_state_extra(struct xfrm_state *x,
+				    struct xfrm_usersa_info *p,
+				    struct sk_buff *skb)
 {
-	struct xfrm_dump_info *sp = ptr;
-	struct sk_buff *in_skb = sp->in_skb;
-	struct sk_buff *skb = sp->out_skb;
-	struct xfrm_usersa_info *p;
-	struct nlmsghdr *nlh;
-
-	if (sp->this_idx < sp->start_idx)
-		goto out;
-
-	nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
-			XFRM_MSG_NEWSA, sizeof(*p), sp->nlmsg_flags);
-	if (nlh == NULL)
-		return -EMSGSIZE;
-
-	p = nlmsg_data(nlh);
 	copy_to_user_state(x, p);
 
 	if (x->aalg)
@@ -540,6 +528,35 @@ static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
 	if (x->lastused)
 		NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
 
+	return 0;
+
+nla_put_failure:
+	return -EMSGSIZE;
+}
+
+static int dump_one_state(struct xfrm_state *x, int count, void *ptr)
+{
+	struct xfrm_dump_info *sp = ptr;
+	struct sk_buff *in_skb = sp->in_skb;
+	struct sk_buff *skb = sp->out_skb;
+	struct xfrm_usersa_info *p;
+	struct nlmsghdr *nlh;
+	int err;
+
+	if (sp->this_idx < sp->start_idx)
+		goto out;
+
+	nlh = nlmsg_put(skb, NETLINK_CB(in_skb).pid, sp->nlmsg_seq,
+			XFRM_MSG_NEWSA, sizeof(*p), sp->nlmsg_flags);
+	if (nlh == NULL)
+		return -EMSGSIZE;
+
+	p = nlmsg_data(nlh);
+
+	err = copy_to_user_state_extra(x, p, skb);
+	if (err)
+		goto nla_put_failure;
+
 	nlmsg_end(skb, nlh);
 out:
 	sp->this_idx++;
@@ -547,7 +564,7 @@ out:
 
 nla_put_failure:
 	nlmsg_cancel(skb, nlh);
-	return -EMSGSIZE;
+	return err;
 }
 
 static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)
@@ -1973,6 +1990,14 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
 		l += nla_total_size(sizeof(*x->calg));
 	if (x->encap)
 		l += nla_total_size(sizeof(*x->encap));
+	if (x->security)
+		l += nla_total_size(sizeof(struct xfrm_user_sec_ctx) +
+				    x->security->ctx_len);
+	if (x->coaddr)
+		l += nla_total_size(sizeof(*x->coaddr));
+
+	/* Must count this as this may become non-zero behind our back. */
+	l += nla_total_size(sizeof(x->lastused));
 
 	return l;
 }
@@ -2018,23 +2043,16 @@ static int xfrm_notify_sa(struct xfrm_state *x, struct km_event *c)
 		p = nla_data(attr);
 	}
 
-	copy_to_user_state(x, p);
-
-	if (x->aalg)
-		NLA_PUT(skb, XFRMA_ALG_AUTH, alg_len(x->aalg), x->aalg);
-	if (x->ealg)
-		NLA_PUT(skb, XFRMA_ALG_CRYPT, alg_len(x->ealg), x->ealg);
-	if (x->calg)
-		NLA_PUT(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
-
-	if (x->encap)
-		NLA_PUT(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
+	if (copy_to_user_state_extra(x, p, skb))
+		goto nla_put_failure;
 
 	nlmsg_end(skb, nlh);
 
 	return nlmsg_multicast(xfrm_nl, skb, 0, XFRMNLGRP_SA, GFP_ATOMIC);
 
 nla_put_failure:
+	/* Somebody screwed up with xfrm_sa_len! */
+	WARN_ON(1);
 	kfree_skb(skb);
 	return -1;
 }

Re: [PATCH 5/7] [XFRM] user: Move attribute copying code into copy_to_user_state_extra

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:35 +0800

> [XFRM] user: Move attribute copying code into copy_to_user_state_extra
> 
> Here's a good example of code duplication leading to code rot.  The
> notification patch did its own netlink message creation for xfrm states.
> It duplicated code that was already in dump_one_state.  Guess what, the
> next time (and the time after) when someone updated dump_one_state the
> notification path got zilch.
> 
> This patch moves that code from dump_one_state to copy_to_user_state_extra
> and uses it in xfrm_notify_sa too.  Unfortunately whoever updates this
> still needs to update xfrm_sa_len since the notification path wants to
> know the exact size for allocation.
> 
> At least I've added a comment saying so and if someone still forgest, we'll
> have a WARN_ON telling us so.
> 
> I also changed the security size calculation to use xfrm_user_sec_ctx since
> that's what we actually put into the skb.  However it makes no practical
> difference since it has the same size as xfrm_sec_ctx.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks!

[PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[Herbert Xu]

[IPSEC]: Lock state when copying non-atomic fields to user-space

This patch adds locking so that when we're copying non-atomic fields such as
life-time or coaddr to user-space we don't get a partial result.

For af_key I've changed every instance of pfkey_xfrm_state2msg apart from
expiration notification to include the keys and life-times.  This is in-line
with XFRM behaviour.

The actual cases affected are:

* pfkey_getspi: No change as we don't have any keys to copy.
* key_notify_sa:
	+ ADD/UPD: This wouldn't work otherwise.
	+ DEL: It can't hurt.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/key/af_key.c     |   35 +++++++++++++++++++++++++----------
 net/xfrm/xfrm_user.c |   14 ++++++++------
 2 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 143d46f..7969f8a 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -655,7 +655,8 @@ static inline int pfkey_mode_to_xfrm(int mode)
 	}
 }
 
-static struct sk_buff * pfkey_xfrm_state2msg(struct xfrm_state *x, int add_keys, int hsc)
+static struct sk_buff *__pfkey_xfrm_state2msg(struct xfrm_state *x,
+					      int add_keys, int hsc)
 {
 	struct sk_buff *skb;
 	struct sadb_msg *hdr;
@@ -1009,6 +1010,24 @@ static struct sk_buff * pfkey_xfrm_state2msg(struct xfrm_state *x, int add_keys,
 	return skb;
 }
 
+
+static inline struct sk_buff *pfkey_xfrm_state2msg(struct xfrm_state *x)
+{
+	struct sk_buff *skb;
+
+	spin_lock_bh(&x->lock);
+	skb = __pfkey_xfrm_state2msg(x, 1, 3);
+	spin_unlock_bh(&x->lock);
+
+	return skb;
+}
+
+static inline struct sk_buff *pfkey_xfrm_state2msg_expire(struct xfrm_state *x,
+							  int hsc)
+{
+	return __pfkey_xfrm_state2msg(x, 0, hsc);
+}
+
 static struct xfrm_state * pfkey_msg2xfrm_state(struct sadb_msg *hdr,
 						void **ext_hdrs)
 {
@@ -1322,7 +1341,7 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, struct sadb_msg *h
 	}
 
 	err = xfrm_alloc_spi(x, min_spi, max_spi);
-	resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x, 0, 3);
+	resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x);
 
 	if (IS_ERR(resp_skb)) {
 		xfrm_state_put(x);
@@ -1412,12 +1431,8 @@ static int key_notify_sa(struct xfrm_state *x, struct km_event *c)
 {
 	struct sk_buff *skb;
 	struct sadb_msg *hdr;
-	int hsc = 3;
-
-	if (c->event == XFRM_MSG_DELSA)
-		hsc = 0;
 
-	skb = pfkey_xfrm_state2msg(x, 0, hsc);
+	skb = pfkey_xfrm_state2msg(x);
 
 	if (IS_ERR(skb))
 		return PTR_ERR(skb);
@@ -1529,7 +1544,7 @@ static int pfkey_get(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hdr,
 	if (x == NULL)
 		return -ESRCH;
 
-	out_skb = pfkey_xfrm_state2msg(x, 1, 3);
+	out_skb = pfkey_xfrm_state2msg(x);
 	proto = x->id.proto;
 	xfrm_state_put(x);
 	if (IS_ERR(out_skb))
@@ -1709,7 +1724,7 @@ static int dump_sa(struct xfrm_state *x, int count, void *ptr)
 	struct sk_buff *out_skb;
 	struct sadb_msg *out_hdr;
 
-	out_skb = pfkey_xfrm_state2msg(x, 1, 3);
+	out_skb = pfkey_xfrm_state2msg(x);
 	if (IS_ERR(out_skb))
 		return PTR_ERR(out_skb);
 
@@ -2910,7 +2925,7 @@ static int key_notify_sa_expire(struct xfrm_state *x, struct km_event *c)
 	else
 		hsc = 1;
 
-	out_skb = pfkey_xfrm_state2msg(x, 0, hsc);
+	out_skb = pfkey_xfrm_state2msg_expire(x, hsc);
 	if (IS_ERR(out_skb))
 		return PTR_ERR(out_skb);
 
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 2cbbe5e..5238f6a 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -507,8 +507,16 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 				    struct xfrm_usersa_info *p,
 				    struct sk_buff *skb)
 {
+	spin_lock_bh(&x->lock);
 	copy_to_user_state(x, p);
 
+	if (x->coaddr)
+		NLA_PUT(skb, XFRMA_COADDR, sizeof(*x->coaddr), x->coaddr);
+
+	if (x->lastused)
+		NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
+	spin_unlock_bh(&x->lock);
+
 	if (x->aalg)
 		NLA_PUT(skb, XFRMA_ALG_AUTH, alg_len(x->aalg), x->aalg);
 	if (x->ealg)
@@ -522,12 +530,6 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 	if (x->security && copy_sec_ctx(x->security, skb) < 0)
 		goto nla_put_failure;
 
-	if (x->coaddr)
-		NLA_PUT(skb, XFRMA_COADDR, sizeof(*x->coaddr), x->coaddr);
-
-	if (x->lastused)
-		NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
-
 	return 0;
 
 nla_put_failure:

Re: [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:36 +0800

> [IPSEC]: Lock state when copying non-atomic fields to user-space
> 
> This patch adds locking so that when we're copying non-atomic fields such as
> life-time or coaddr to user-space we don't get a partial result.
> 
> For af_key I've changed every instance of pfkey_xfrm_state2msg apart from
> expiration notification to include the keys and life-times.  This is in-line
> with XFRM behaviour.
> 
> The actual cases affected are:
> 
> * pfkey_getspi: No change as we don't have any keys to copy.
> * key_notify_sa:
> 	+ ADD/UPD: This wouldn't work otherwise.
> 	+ DEL: It can't hurt.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied.

I would be more careful with the changelog description for
something like this in the future.  It sounds like this
patch will cause us to touch userspace with locks held,
which obviously only works in very limited scenerios
and is usually a BUG.

But you're actually just constructing SKB response netlink
bits, which later will be copied into userspace but long
after we've released these locks.

Re: [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[Herbert Xu]

On Tue, Oct 09, 2007 at 01:33:07PM -0700, David Miller wrote:
>
> I would be more careful with the changelog description for
> something like this in the future.  It sounds like this
> patch will cause us to touch userspace with locks held,
> which obviously only works in very limited scenerios
> and is usually a BUG.
> 
> But you're actually just constructing SKB response netlink
> bits, which later will be copied into userspace but long
> after we've released these locks.

Good point.  I should've said skb destined for user-spac

Cheers,
-- 
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: [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[Masahide NAKAMURA]

Hello Herbert,

Wednesday 10 October 2007 09:48, Herbert Xu wrote:
> On Tue, Oct 09, 2007 at 01:33:07PM -0700, David Miller wrote:
> >
> > I would be more careful with the changelog description for
> > something like this in the future.  It sounds like this
> > patch will cause us to touch userspace with locks held,
> > which obviously only works in very limited scenerios
> > and is usually a BUG.
> > 
> > But you're actually just constructing SKB response netlink
> > bits, which later will be copied into userspace but long
> > after we've released these locks.
> 
> Good point.  I should've said skb destined for user-spac

With SMP enabled kernel, I found a lock problem at xfrm_state_walk()
path with the patch on current net-2.6.25. Its log is "circular locking
dependency detected".

As you might notice it, to fix it possibly you should have either:

(1) Change xfrm_state_walk() as xfrm_state_flush() does; Unlock
just before accessing the state instance and increment its reference
count, then call the hook.

or
(2) Change (or create new) hook functions for xfrm_state_walk() without
any lock since it is called within the list global lock (aka xfrm_state_lock ) is held
i.e. af_key.c(dump_sa),  xfrm_user.c(copy_to_user_state_extra).


Regards,

-- 
Masahide NAKAMURA

Re: [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[Herbert Xu]

On Mon, Nov 26, 2007 at 12:03:46PM +0900, Masahide NAKAMURA wrote:
>
> With SMP enabled kernel, I found a lock problem at xfrm_state_walk()
> path with the patch on current net-2.6.25. Its log is "circular locking
> dependency detected".

Thanks.  Ingo Molnar reported that too.

I'm just going to revert this patch for 2.6.24 since we've lived
with this race for so long anyway.

Later I'm going to change the locking order so that we always do
the xfrm_state_lock first before taking the x->lock. The rationale
is that only x->lock is taken on the fast path so we want minimise
its outage while xfrm_state_lock isn't.

In fact another change I want to do even later is to move the
expire timers into a work queue which would let us get rid of the
xfrm_*_lock's altogether.

Hmm, I've been talking about that second item for years :) Anybody
who wants to contribute to either of this is most welcome since
my plate's pretty full already.

Cheers,
-- 
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: [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[Herbert Xu]

On Mon, Nov 26, 2007 at 11:18:45AM +0800, Herbert Xu wrote:
>
> I'm just going to revert this patch for 2.6.24 since we've lived
> with this race for so long anyway.

Actually, instead of reverting it completely I'm just going to
remove the newly added locks which should be just as effective.

This would reduce the churn in the code as we'd be putting most
of it back soon anyway.

Cheers,
-- 
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
--
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 3b2d864..878039b 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1015,9 +1015,7 @@ static inline struct sk_buff *pfkey_xfrm_state2msg(struct xfrm_state *x)
 {
 	struct sk_buff *skb;
 
-	spin_lock_bh(&x->lock);
 	skb = __pfkey_xfrm_state2msg(x, 1, 3);
-	spin_unlock_bh(&x->lock);
 
 	return skb;
 }
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index d41588d..e75dbdc 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -507,7 +507,6 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 				    struct xfrm_usersa_info *p,
 				    struct sk_buff *skb)
 {
-	spin_lock_bh(&x->lock);
 	copy_to_user_state(x, p);
 
 	if (x->coaddr)
@@ -515,7 +514,6 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 
 	if (x->lastused)
 		NLA_PUT_U64(skb, XFRMA_LASTUSED, x->lastused);
-	spin_unlock_bh(&x->lock);
 
 	if (x->aalg)
 		NLA_PUT(skb, XFRMA_ALG_AUTH, alg_len(x->aalg), x->aalg);

Re: [PATCH 6/7] [IPSEC]: Lock state when copying non-atomic fields to user-space

[Masahide NAKAMURA]

Herbert,

Monday 26 November 2007 20:07, Herbert Xu wrote:
> On Mon, Nov 26, 2007 at 11:18:45AM +0800, Herbert Xu wrote:
> >
> > I'm just going to revert this patch for 2.6.24 since we've lived
> > with this race for so long anyway.
> 
> Actually, instead of reverting it completely I'm just going to
> remove the newly added locks which should be just as effective.
> 
> This would reduce the churn in the code as we'd be putting most
> of it back soon anyway.

With the patch you sent, the xfrm_state_walk() issue I reported
is solved at current net-2.6.25.

-- 
Masahide NAKAMURA

[PATCH 7/7] [IPSEC]: Move state lock into x->type->output

[Herbert Xu]

[IPSEC]: Move state lock into x->type->output

This patch releases the lock on the state before calling x->type->output.
It also adds the lock to the spots where they're currently needed.

Most of those places (all except mip6) are expected to disappear with
async crypto.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---

 net/ipv4/ah4.c         |    7 ++++++-
 net/ipv4/esp4.c        |   10 ++++++++--
 net/ipv6/ah6.c         |    9 ++++++---
 net/ipv6/esp6.c        |   10 ++++++++--
 net/ipv6/mip6.c        |    4 ++++
 net/xfrm/xfrm_output.c |    8 ++++----
 6 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 58af298..3513149 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -5,6 +5,7 @@
 #include <net/ah.h>
 #include <linux/crypto.h>
 #include <linux/pfkeyv2.h>
+#include <linux/spinlock.h>
 #include <net/icmp.h>
 #include <net/protocol.h>
 #include <asm/scatterlist.h>
@@ -97,10 +98,14 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->reserved = 0;
 	ah->spi = x->id.spi;
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
+
+	spin_lock_bh(&x->lock);
 	err = ah_mac_digest(ahp, skb, ah->auth_data);
+	memcpy(ah->auth_data, ahp->work_icv, ahp->icv_trunc_len);
+	spin_unlock_bh(&x->lock);
+
 	if (err)
 		goto error;
-	memcpy(ah->auth_data, ahp->work_icv, ahp->icv_trunc_len);
 
 	top_iph->tos = iph->tos;
 	top_iph->ttl = iph->ttl;
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index ffd5653..452910d 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/pfkeyv2.h>
 #include <linux/random.h>
+#include <linux/spinlock.h>
 #include <net/icmp.h>
 #include <net/protocol.h>
 #include <net/udp.h>
@@ -66,6 +67,8 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	top_iph->tot_len = htons(skb->len + alen);
 	*(skb_tail_pointer(trailer) - 1) = top_iph->protocol;
 
+	spin_lock_bh(&x->lock);
+
 	/* this is non-NULL only with UDP Encapsulation */
 	if (x->encap) {
 		struct xfrm_encap_tmpl *encap = x->encap;
@@ -111,7 +114,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 		if (unlikely(nfrags > ESP_NUM_FAST_SG)) {
 			sg = kmalloc(sizeof(struct scatterlist)*nfrags, GFP_ATOMIC);
 			if (!sg)
-				goto error;
+				goto unlock;
 		}
 		skb_to_sgvec(skb, sg, esph->enc_data+esp->conf.ivlen-skb->data, clen);
 		err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
@@ -120,7 +123,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	} while (0);
 
 	if (unlikely(err))
-		goto error;
+		goto unlock;
 
 	if (esp->conf.ivlen) {
 		memcpy(esph->enc_data, esp->conf.ivec, esp->conf.ivlen);
@@ -133,6 +136,9 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 		memcpy(pskb_put(skb, trailer, alen), esp->auth.work_icv, alen);
 	}
 
+unlock:
+	spin_unlock_bh(&x->lock);
+
 	ip_send_check(top_iph);
 
 error:
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index ff904a7..c51d775 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -29,6 +29,7 @@
 #include <net/ah.h>
 #include <linux/crypto.h>
 #include <linux/pfkeyv2.h>
+#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <net/icmp.h>
 #include <net/ipv6.h>
@@ -284,12 +285,14 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah->reserved = 0;
 	ah->spi = x->id.spi;
 	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
+
+	spin_lock_bh(&x->lock);
 	err = ah_mac_digest(ahp, skb, ah->auth_data);
-	if (err)
-		goto error_free_iph;
 	memcpy(ah->auth_data, ahp->work_icv, ahp->icv_trunc_len);
+	spin_unlock_bh(&x->lock);
 
-	err = 0;
+	if (err)
+		goto error_free_iph;
 
 	memcpy(top_iph, tmp_base, sizeof(tmp_base));
 	if (tmp_ext) {
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 9fc1940..7355bb0 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -34,6 +34,7 @@
 #include <linux/kernel.h>
 #include <linux/pfkeyv2.h>
 #include <linux/random.h>
+#include <linux/spinlock.h>
 #include <net/icmp.h>
 #include <net/ipv6.h>
 #include <net/protocol.h>
@@ -98,6 +99,8 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	esph->spi = x->id.spi;
 	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
 
+	spin_lock_bh(&x->lock);
+
 	if (esp->conf.ivlen) {
 		if (unlikely(!esp->conf.ivinitted)) {
 			get_random_bytes(esp->conf.ivec, esp->conf.ivlen);
@@ -112,7 +115,7 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 		if (unlikely(nfrags > ESP_NUM_FAST_SG)) {
 			sg = kmalloc(sizeof(struct scatterlist)*nfrags, GFP_ATOMIC);
 			if (!sg)
-				goto error;
+				goto unlock;
 		}
 		skb_to_sgvec(skb, sg, esph->enc_data+esp->conf.ivlen-skb->data, clen);
 		err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
@@ -121,7 +124,7 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	} while (0);
 
 	if (unlikely(err))
-		goto error;
+		goto unlock;
 
 	if (esp->conf.ivlen) {
 		memcpy(esph->enc_data, esp->conf.ivec, esp->conf.ivlen);
@@ -134,6 +137,9 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 		memcpy(pskb_put(skb, trailer, alen), esp->auth.work_icv, alen);
 	}
 
+unlock:
+	spin_unlock_bh(&x->lock);
+
 error:
 	return err;
 }
diff --git a/net/ipv6/mip6.c b/net/ipv6/mip6.c
index 7261c29..6475bac 100644
--- a/net/ipv6/mip6.c
+++ b/net/ipv6/mip6.c
@@ -172,7 +172,9 @@ static int mip6_destopt_output(struct xfrm_state *x, struct sk_buff *skb)
 	len = ((char *)hao - (char *)dstopt) + sizeof(*hao);
 
 	memcpy(&hao->addr, &iph->saddr, sizeof(hao->addr));
+	spin_lock_bh(&x->lock);
 	memcpy(&iph->saddr, x->coaddr, sizeof(iph->saddr));
+	spin_unlock_bh(&x->lock);
 
 	BUG_TRAP(len == x->props.header_len);
 	dstopt->hdrlen = (x->props.header_len >> 3) - 1;
@@ -381,7 +383,9 @@ static int mip6_rthdr_output(struct xfrm_state *x, struct sk_buff *skb)
 	BUG_TRAP(rt2->rt_hdr.hdrlen == 2);
 
 	memcpy(&rt2->addr, &iph->daddr, sizeof(rt2->addr));
+	spin_lock_bh(&x->lock);
 	memcpy(&iph->daddr, x->coaddr, sizeof(iph->daddr));
+	spin_unlock_bh(&x->lock);
 
 	return 0;
 }
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 9847bae..0eb3377 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -67,15 +67,15 @@ int xfrm_output(struct sk_buff *skb)
 		if (err)
 			goto error;
 
-		err = x->type->output(x, skb);
-		if (err)
-			goto error;
-
 		x->curlft.bytes += skb->len;
 		x->curlft.packets++;
 
 		spin_unlock_bh(&x->lock);
 
+		err = x->type->output(x, skb);
+		if (err)
+			goto error_nolock;
+
 		if (!(skb->dst = dst_pop(dst))) {
 			err = -EHOSTUNREACH;
 			goto error_nolock;

Re: [PATCH 7/7] [IPSEC]: Move state lock into x->type->output

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 09 Oct 2007 22:36:37 +0800

> [IPSEC]: Move state lock into x->type->output
> 
> This patch releases the lock on the state before calling x->type->output.
> It also adds the lock to the spots where they're currently needed.
> 
> Most of those places (all except mip6) are expected to disappear with
> async crypto.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks!