| Message ID | 1389663552-29638-3-git-send-email-fan.du@windriver.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 ?
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); } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;
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(-)