[PATCHv3 net-next 0/5] xfrm: Add ESN support for AH

[PATCHv3 net-next 0/5] xfrm: Add ESN support for AH

[Fan Du]

Hi,

This is initial Extended Sequence Number support for AH based on IPv4/6.
The rationale is totally by the RFC 4302, which states:

3.3.3.2.2.  Implicit Packet Padding and ESN

   If the ESN option is elected for an SA, then the high-order 32 bits
   of the ESN must be included in the ICV computation.  For purposes of
   ICV computation, these bits are appended (implicitly) immediately
   after the end of the payload, and before any implicit packet padding.

So we attach the high-order 32bits as a scatterlist right after the packet
payload to compute ICV value. 

Test:
I add a knob in iproute2/ip/xfrm_state.c to enable esn when setting SA,
which make it possible to test with-esn and without-esn scenarios, both
cases works ok with ping using packetsize(-s) from default to 32768. 

v2:
  - Patch3/5 and Patch4/5 add IPv6 part as requested by Steffen.
  - Patch5/5 restrict ESN feature only to ESP and AH.
v3:
  - Fix double parens spotted by Sergei, and thanks for reporting.

Fan Du (5):
  {IPv4,xfrm} Add ESN support for AH egress part
  {IPv4,xfrm} Add ESN support for AH ingress part
  {IPv6,xfrm} Add ESN support for AH egress part
  {IPv6,xfrm} Add ESN support for AH ingress part
  xfrm: Don't prohibit AH from using ESN feature

 net/ipv4/ah4.c       |   50 +++++++++++++++++++++++++++++++++++++++++-------
 net/ipv6/ah6.c       |   52 +++++++++++++++++++++++++++++++++++++++++++-------
 net/xfrm/xfrm_user.c |    3 ++-
 3 files changed, 90 insertions(+), 15 deletions(-)

-- 
1.7.9.5

[PATCHv3 net-next 1/5] {IPv4,xfrm} Add ESN support for AH egress part

[Fan Du]

This patch add esn support for AH output stage by attaching upper 32bits
sequence number right after packet payload as specified by RFC 4302.

Then the ICV value will guard upper 32bits sequence number as well when
packet going out.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/ipv4/ah4.c |   25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 7179026..759e489 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -12,6 +12,7 @@
 #include <linux/scatterlist.h>
 #include <net/icmp.h>
 #include <net/protocol.h>
+#include <crypto/scatterwalk.h>
 
 struct ah_skb_cb {
 	struct xfrm_skb_cb xfrm;
@@ -155,6 +156,10 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	struct iphdr *iph, *top_iph;
 	struct ip_auth_hdr *ah;
 	struct ah_data *ahp;
+	int seqhi_len = 0;
+	__be32 *seqhi;
+	int sglists = 0;
+	struct scatterlist *seqhisg;
 
 	ahp = x->data;
 	ahash = ahp->ahash;
@@ -167,14 +172,19 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	ah = ip_auth_hdr(skb);
 	ihl = ip_hdrlen(skb);
 
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sglists = 1;
+		seqhi_len = sizeof(*seqhi);
+	}
 	err = -ENOMEM;
-	iph = ah_alloc_tmp(ahash, nfrags, ihl);
+	iph = ah_alloc_tmp(ahash, nfrags + sglists, ihl + seqhi_len);
 	if (!iph)
 		goto out;
-
-	icv = ah_tmp_icv(ahash, iph, ihl);
+	seqhi = (__be32 *)((char *)iph + ihl);
+	icv = ah_tmp_icv(ahash, seqhi, seqhi_len);
 	req = ah_tmp_req(ahash, icv);
 	sg = ah_req_sg(ahash, req);
+	seqhisg = sg + nfrags;
 
 	memset(ah->auth_data, 0, ahp->icv_trunc_len);
 
@@ -213,7 +223,14 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 	sg_init_table(sg, nfrags);
 	skb_to_sgvec(skb, sg, 0, skb->len);
 
-	ahash_request_set_crypt(req, sg, icv, skb->len);
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sg_unmark_end(&sg[nfrags - 1]);
+		/* Attach seqhi sg right after packet payload */
+		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
+		sg_init_table(seqhisg, sglists);
+		sg_set_buf(seqhisg, seqhi, seqhi_len);
+	}
+	ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
 	ahash_request_set_callback(req, 0, ah_output_done, skb);
 
 	AH_SKB_CB(skb)->tmp = iph;
-- 
1.7.9.5

[PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Fan Du]

This patch add esn support for AH input stage by attaching upper 32bits
sequence number right after packet payload as specified by RFC 4302.

Then the ICV value will guard upper 32bits sequence number as well when
packet getting in.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/ipv4/ah4.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 759e489..7ca2fe7 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -312,6 +312,10 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
 	struct ip_auth_hdr *ah;
 	struct ah_data *ahp;
 	int err = -ENOMEM;
+	int seqhi_len = 0;
+	__be32 *seqhi;
+	int sglists = 0;
+	struct scatterlist *seqhisg;
 
 	if (!pskb_may_pull(skb, sizeof(*ah)))
 		goto out;
@@ -352,14 +356,22 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
 	iph = ip_hdr(skb);
 	ihl = ip_hdrlen(skb);
 
-	work_iph = ah_alloc_tmp(ahash, nfrags, ihl + ahp->icv_trunc_len);
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sglists = 1;
+		seqhi_len = sizeof(*seqhi);
+	}
+
+	work_iph = ah_alloc_tmp(ahash, nfrags + sglists, ihl +
+				ahp->icv_trunc_len + seqhi_len);
 	if (!work_iph)
 		goto out;
 
-	auth_data = ah_tmp_auth(work_iph, ihl);
+	seqhi = (__be32 *)((char *)work_iph + ihl);
+	auth_data = ah_tmp_auth(seqhi, seqhi_len);
 	icv = ah_tmp_icv(ahash, auth_data, ahp->icv_trunc_len);
 	req = ah_tmp_req(ahash, icv);
 	sg = ah_req_sg(ahash, req);
+	seqhisg = sg + nfrags;
 
 	memcpy(work_iph, iph, ihl);
 	memcpy(auth_data, ah->auth_data, ahp->icv_trunc_len);
@@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
 	sg_init_table(sg, nfrags);
 	skb_to_sgvec(skb, sg, 0, skb->len);
 
-	ahash_request_set_crypt(req, sg, icv, skb->len);
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sg_unmark_end(&sg[nfrags - 1]);
+		/* Attach seqhi sg right after packet payload */
+		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
+		sg_init_table(seqhisg, sglists);
+		sg_set_buf(seqhisg, seqhi, seqhi_len);
+	}
+	ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
 	ahash_request_set_callback(req, 0, ah_input_done, skb);
 
 	AH_SKB_CB(skb)->tmp = work_iph;
-- 
1.7.9.5

[PATCHv3 net-next 3/5] {IPv6,xfrm} Add ESN support for AH egress part

[Fan Du]

This patch add esn support for AH output stage by attaching upper 32bits
sequence number right after packet payload as specified by RFC 4302.

Then the ICV value will guard upper 32bits sequence number as well when
packet going out.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/ipv6/ah6.c |   24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 81e496a..3053a01 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -346,6 +346,10 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	struct ip_auth_hdr *ah;
 	struct ah_data *ahp;
 	struct tmp_ext *iph_ext;
+	int seqhi_len = 0;
+	__be32 *seqhi;
+	int sglists = 0;
+	struct scatterlist *seqhisg;
 
 	ahp = x->data;
 	ahash = ahp->ahash;
@@ -359,15 +363,22 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	if (extlen)
 		extlen += sizeof(*iph_ext);
 
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sglists = 1;
+		seqhi_len = sizeof(*seqhi);
+	}
 	err = -ENOMEM;
-	iph_base = ah_alloc_tmp(ahash, nfrags, IPV6HDR_BASELEN + extlen);
+	iph_base = ah_alloc_tmp(ahash, nfrags + sglists, IPV6HDR_BASELEN +
+				extlen + seqhi_len);
 	if (!iph_base)
 		goto out;
 
 	iph_ext = ah_tmp_ext(iph_base);
-	icv = ah_tmp_icv(ahash, iph_ext, extlen);
+	seqhi = (__be32 *)((char *)iph_ext + extlen);
+	icv = ah_tmp_icv(ahash, seqhi, seqhi_len);
 	req = ah_tmp_req(ahash, icv);
 	sg = ah_req_sg(ahash, req);
+	seqhisg = sg + nfrags;
 
 	ah = ip_auth_hdr(skb);
 	memset(ah->auth_data, 0, ahp->icv_trunc_len);
@@ -414,7 +425,14 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 	sg_init_table(sg, nfrags);
 	skb_to_sgvec(skb, sg, 0, skb->len);
 
-	ahash_request_set_crypt(req, sg, icv, skb->len);
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sg_unmark_end(&sg[nfrags - 1]);
+		/* Attach seqhi sg right after packet payload */
+		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
+		sg_init_table(seqhisg, sglists);
+		sg_set_buf(seqhisg, seqhi, seqhi_len);
+	}
+	ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
 	ahash_request_set_callback(req, 0, ah6_output_done, skb);
 
 	AH_SKB_CB(skb)->tmp = iph_base;
-- 
1.7.9.5

[PATCHv3 net-next 4/5] {IPv6,xfrm} Add ESN support for AH ingress part

[Fan Du]

This patch add esn support for AH input stage by attaching upper 32bits
sequence number right after packet payload as specified by RFC 4302.

Then the ICV value will guard upper 32bits sequence number as well when
packet going in.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/ipv6/ah6.c |   28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 3053a01..8119256 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -532,6 +532,10 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
 	int nexthdr;
 	int nfrags;
 	int err = -ENOMEM;
+	int seqhi_len = 0;
+	__be32 *seqhi;
+	int sglists = 0;
+	struct scatterlist *seqhisg;
 
 	if (!pskb_may_pull(skb, sizeof(struct ip_auth_hdr)))
 		goto out;
@@ -568,14 +572,22 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
 
 	skb_push(skb, hdr_len);
 
-	work_iph = ah_alloc_tmp(ahash, nfrags, hdr_len + ahp->icv_trunc_len);
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sglists = 1;
+		seqhi_len = sizeof(*seqhi);
+	}
+
+	work_iph = ah_alloc_tmp(ahash, nfrags + sglists, hdr_len +
+				ahp->icv_trunc_len + seqhi_len);
 	if (!work_iph)
 		goto out;
 
-	auth_data = ah_tmp_auth(work_iph, hdr_len);
-	icv = ah_tmp_icv(ahash, auth_data, ahp->icv_trunc_len);
+	auth_data = ah_tmp_auth((u8 *)work_iph, hdr_len);
+	seqhi = (__be32 *)(auth_data + ahp->icv_trunc_len);
+	icv = ah_tmp_icv(ahash, seqhi, seqhi_len);
 	req = ah_tmp_req(ahash, icv);
 	sg = ah_req_sg(ahash, req);
+	seqhisg = sg + nfrags;
 
 	memcpy(work_iph, ip6h, hdr_len);
 	memcpy(auth_data, ah->auth_data, ahp->icv_trunc_len);
@@ -593,7 +605,15 @@ static int ah6_input(struct xfrm_state *x, struct sk_buff *skb)
 	sg_init_table(sg, nfrags);
 	skb_to_sgvec(skb, sg, 0, skb->len);
 
-	ahash_request_set_crypt(req, sg, icv, skb->len);
+	if (x->props.flags & XFRM_STATE_ESN) {
+		sg_unmark_end(&sg[nfrags - 1]);
+		/* Attach seqhi sg right after packet payload */
+		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
+		sg_init_table(seqhisg, sglists);
+		sg_set_buf(seqhisg, seqhi, seqhi_len);
+	}
+
+	ahash_request_set_crypt(req, sg, icv, skb->len + seqhi_len);
 	ahash_request_set_callback(req, 0, ah6_input_done, skb);
 
 	AH_SKB_CB(skb)->tmp = work_iph;
-- 
1.7.9.5

[PATCHv3 net-next 5/5] xfrm: Don't prohibit AH from using ESN feature

[Fan Du]

Clear checking when user try to use ESN through netlink keymgr for AH.
As only ESP and AH support ESN feature according to RFC.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/xfrm/xfrm_user.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 97681a3..dbd287d 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -142,7 +142,8 @@ static inline int verify_replay(struct xfrm_usersa_info *p,
 	if (!rt)
 		return 0;
 
-	if (p->id.proto != IPPROTO_ESP)
+	/* As only ESP and AH support ESN feature. */
+	if ((p->id.proto != IPPROTO_ESP) && (p->id.proto != IPPROTO_AH))
 		return -EINVAL;
 
 	if (p->replay_window != 0)
-- 
1.7.9.5

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Steffen Klassert]

On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
> @@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
>  	sg_init_table(sg, nfrags);
>  	skb_to_sgvec(skb, sg, 0, skb->len);
>  
> -	ahash_request_set_crypt(req, sg, icv, skb->len);
> +	if (x->props.flags & XFRM_STATE_ESN) {
> +		sg_unmark_end(&sg[nfrags - 1]);
> +		/* Attach seqhi sg right after packet payload */
> +		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);

This is ah_input(), so you should use the high bits of the input
sequence number here. The ipv6 patch has the same problem.

> +		sg_init_table(seqhisg, sglists);

Why do you add a separate SG table for this?

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Fan Du]

On 2014年01月14日 17:54, Steffen Klassert wrote:
> On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
>> @@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
>>   	sg_init_table(sg, nfrags);
>>   	skb_to_sgvec(skb, sg, 0, skb->len);
>>
>> -	ahash_request_set_crypt(req, sg, icv, skb->len);
>> +	if (x->props.flags&  XFRM_STATE_ESN) {
>> +		sg_unmark_end(&sg[nfrags - 1]);
>> +		/* Attach seqhi sg right after packet payload */
>> +		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
>
> This is ah_input(), so you should use the high bits of the input
> sequence number here. The ipv6 patch has the same problem.

ok, I will fix this.

>
>> +		sg_init_table(seqhisg, sglists);
>
> Why do you add a separate SG table for this?

It just initialize a single seqhisg, which is actually followed behind packet payload sg table.
initialized seqhisg actually mark itself as the end of sg list.

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Steffen Klassert]

On Tue, Jan 14, 2014 at 06:01:32PM +0800, Fan Du wrote:
> 
> 
> On 2014年01月14日 17:54, Steffen Klassert wrote:
> >On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
> >>@@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
> >>  	sg_init_table(sg, nfrags);
> >>  	skb_to_sgvec(skb, sg, 0, skb->len);
> >>
> >>-	ahash_request_set_crypt(req, sg, icv, skb->len);
> >>+	if (x->props.flags&  XFRM_STATE_ESN) {
> >>+		sg_unmark_end(&sg[nfrags - 1]);
> >>+		/* Attach seqhi sg right after packet payload */
> >>+		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
> >
> >This is ah_input(), so you should use the high bits of the input
> >sequence number here. The ipv6 patch has the same problem.
> 
> ok, I will fix this.
> 
> >
> >>+		sg_init_table(seqhisg, sglists);
> >
> >Why do you add a separate SG table for this?
> 
> It just initialize a single seqhisg, which is actually followed behind packet payload sg table.
> initialized seqhisg actually mark itself as the end of sg list.
> 

Why don't you just add this entry to the existing SG table?

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Fan Du]

On 2014年01月14日 18:09, Steffen Klassert wrote:
> On Tue, Jan 14, 2014 at 06:01:32PM +0800, Fan Du wrote:
>>
>>
>> On 2014年01月14日 17:54, Steffen Klassert wrote:
>>> On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
>>>> @@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
>>>>   	sg_init_table(sg, nfrags);
>>>>   	skb_to_sgvec(skb, sg, 0, skb->len);
>>>>
>>>> -	ahash_request_set_crypt(req, sg, icv, skb->len);
>>>> +	if (x->props.flags&   XFRM_STATE_ESN) {
>>>> +		sg_unmark_end(&sg[nfrags - 1]);
>>>> +		/* Attach seqhi sg right after packet payload */
>>>> +		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
>>>
>>> This is ah_input(), so you should use the high bits of the input
>>> sequence number here. The ipv6 patch has the same problem.
>>
>> ok, I will fix this.
>>
>>>
>>>> +		sg_init_table(seqhisg, sglists);
>>>
>>> Why do you add a separate SG table for this?
>>
>> It just initialize a single seqhisg, which is actually followed behind packet payload sg table.
>> initialized seqhisg actually mark itself as the end of sg list.
>>
>
> Why don't you just add this entry to the existing SG table?
>

Do you mean scatterwalk_crypto_chain ?

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Steffen Klassert]

On Tue, Jan 14, 2014 at 06:17:26PM +0800, Fan Du wrote:
> 
> 
> On 2014年01月14日 18:09, Steffen Klassert wrote:
> >On Tue, Jan 14, 2014 at 06:01:32PM +0800, Fan Du wrote:
> >>
> >>
> >>On 2014年01月14日 17:54, Steffen Klassert wrote:
> >>>On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
> >>>>@@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
> >>>>  	sg_init_table(sg, nfrags);
> >>>>  	skb_to_sgvec(skb, sg, 0, skb->len);
> >>>>
> >>>>-	ahash_request_set_crypt(req, sg, icv, skb->len);
> >>>>+	if (x->props.flags&   XFRM_STATE_ESN) {
> >>>>+		sg_unmark_end(&sg[nfrags - 1]);
> >>>>+		/* Attach seqhi sg right after packet payload */
> >>>>+		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
> >>>
> >>>This is ah_input(), so you should use the high bits of the input
> >>>sequence number here. The ipv6 patch has the same problem.
> >>
> >>ok, I will fix this.
> >>
> >>>
> >>>>+		sg_init_table(seqhisg, sglists);
> >>>
> >>>Why do you add a separate SG table for this?
> >>
> >>It just initialize a single seqhisg, which is actually followed behind packet payload sg table.
> >>initialized seqhisg actually mark itself as the end of sg list.
> >>
> >
> >Why don't you just add this entry to the existing SG table?
> >
> 
> Do you mean scatterwalk_crypto_chain ?

No, I mean something like:

sg_init_table(sg, nfrags + sglists)

if (x->props.flags & XFRM_STATE_ESN) {
	*seqhi = XFRM_SKB_CB(skb)->seq.input.hi;
	sg_set_buf(sg + nfrags, seqhi, seqhi_len);
}

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Fan Du]

On 2014年01月14日 18:34, Steffen Klassert wrote:
> On Tue, Jan 14, 2014 at 06:17:26PM +0800, Fan Du wrote:
>>
>>
>> On 2014年01月14日 18:09, Steffen Klassert wrote:
>>> On Tue, Jan 14, 2014 at 06:01:32PM +0800, Fan Du wrote:
>>>>
>>>>
>>>> On 2014年01月14日 17:54, Steffen Klassert wrote:
>>>>> On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
>>>>>> @@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
>>>>>>   	sg_init_table(sg, nfrags);
>>>>>>   	skb_to_sgvec(skb, sg, 0, skb->len);
>>>>>>
>>>>>> -	ahash_request_set_crypt(req, sg, icv, skb->len);
>>>>>> +	if (x->props.flags&    XFRM_STATE_ESN) {
>>>>>> +		sg_unmark_end(&sg[nfrags - 1]);
>>>>>> +		/* Attach seqhi sg right after packet payload */
>>>>>> +		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
>>>>>
>>>>> This is ah_input(), so you should use the high bits of the input
>>>>> sequence number here. The ipv6 patch has the same problem.
>>>>
>>>> ok, I will fix this.
>>>>
>>>>>
>>>>>> +		sg_init_table(seqhisg, sglists);
>>>>>
>>>>> Why do you add a separate SG table for this?
>>>>
>>>> It just initialize a single seqhisg, which is actually followed behind packet payload sg table.
>>>> initialized seqhisg actually mark itself as the end of sg list.
>>>>
>>>
>>> Why don't you just add this entry to the existing SG table?
>>>
>>
>> Do you mean scatterwalk_crypto_chain ?
>
> No, I mean something like:
>
> sg_init_table(sg, nfrags + sglists)
>
> if (x->props.flags&  XFRM_STATE_ESN) {
> 	*seqhi = XFRM_SKB_CB(skb)->seq.input.hi;
> 	sg_set_buf(sg + nfrags, seqhi, seqhi_len);
> }
>

hehe, it's the same as the option this patch used.
Anyway, I will make it as you suggested in the next round review.


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Steffen Klassert]

On Tue, Jan 14, 2014 at 06:41:15PM +0800, Fan Du wrote:
> 
> 
> On 2014年01月14日 18:34, Steffen Klassert wrote:
> >On Tue, Jan 14, 2014 at 06:17:26PM +0800, Fan Du wrote:
> >>On 2014年01月14日 18:09, Steffen Klassert wrote:
> >
> >No, I mean something like:
> >
> >sg_init_table(sg, nfrags + sglists)
> >
> >if (x->props.flags&  XFRM_STATE_ESN) {
> >	*seqhi = XFRM_SKB_CB(skb)->seq.input.hi;
> >	sg_set_buf(sg + nfrags, seqhi, seqhi_len);
> >}
> >
> 
> hehe, it's the same as the option this patch used.

No, you don't need to sg_unmark_end() before you can add
your entry and it is more obvious what happens here.

Also, I'm not absolutely sure whether the sg magic allows
what you did. Did you test with sg debugging enabled?

> Anyway, I will make it as you suggested in the next round review.
> 

Thanks!

Re: [PATCHv3 net-next 2/5] {IPv4,xfrm} Add ESN support for AH ingress part

[Fan Du]

On 2014年01月14日 18:34, Steffen Klassert wrote:
> On Tue, Jan 14, 2014 at 06:17:26PM +0800, Fan Du wrote:
>>
>>
>> On 2014年01月14日 18:09, Steffen Klassert wrote:
>>> On Tue, Jan 14, 2014 at 06:01:32PM +0800, Fan Du wrote:
>>>>
>>>>
>>>> On 2014年01月14日 17:54, Steffen Klassert wrote:
>>>>> On Tue, Jan 14, 2014 at 09:39:09AM +0800, Fan Du wrote:
>>>>>> @@ -381,7 +393,14 @@ static int ah_input(struct xfrm_state *x, struct sk_buff *skb)
>>>>>>   	sg_init_table(sg, nfrags);
>>>>>>   	skb_to_sgvec(skb, sg, 0, skb->len);
>>>>>>
>>>>>> -	ahash_request_set_crypt(req, sg, icv, skb->len);
>>>>>> +	if (x->props.flags&    XFRM_STATE_ESN) {
>>>>>> +		sg_unmark_end(&sg[nfrags - 1]);
>>>>>> +		/* Attach seqhi sg right after packet payload */
>>>>>> +		*seqhi = htonl(XFRM_SKB_CB(skb)->seq.output.hi);
>>>>>
>>>>> This is ah_input(), so you should use the high bits of the input
>>>>> sequence number here. The ipv6 patch has the same problem.
>>>>
>>>> ok, I will fix this.
>>>>
>>>>>
>>>>>> +		sg_init_table(seqhisg, sglists);
>>>>>
>>>>> Why do you add a separate SG table for this?
>>>>
>>>> It just initialize a single seqhisg, which is actually followed behind packet payload sg table.
>>>> initialized seqhisg actually mark itself as the end of sg list.
>>>>
>>>
>>> Why don't you just add this entry to the existing SG table?
>>>
>>
>> Do you mean scatterwalk_crypto_chain ?
>
> No, I mean something like:
>
> sg_init_table(sg, nfrags + sglists)
>
> if (x->props.flags&  XFRM_STATE_ESN) {
> 	*seqhi = XFRM_SKB_CB(skb)->seq.input.hi;
> 	sg_set_buf(sg + nfrags, seqhi, seqhi_len);
> }
>

Calling skb_to_sgvec to map first part of payload into global sg list will mark the sg
which contains last data of payload as new end, that's side effect of skb_to_sgvec.
This doesn't work well unless calling sg_unmark_end again to revert such effect.

Here we need another method to map payload to sg list without changing the end sg.

-- 
浮沉随浪只记今朝笑

--fan