RE: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
> -Original Message- > From: Doron Roberts-Kedes [mailto:doro...@fb.com] > Sent: Tuesday, August 7, 2018 12:02 AM > To: Vakul Garg > Cc: David S . Miller ; Dave Watson > ; Boris Pismenny ; Aviad > Yehezkel ; netdev@vger.kernel.org > Subject: Re: [PATCH net-next] net/tls: Calculate nsg for zerocopy path > without skb_cow_data. > > On Fri, Aug 03, 2018 at 11:49:02AM -0700, Doron Roberts-Kedes wrote: > > On Fri, Aug 03, 2018 at 01:23:33AM +, Vakul Garg wrote: > > > > > > > > > > -Original Message- > > > > From: Doron Roberts-Kedes [mailto:doro...@fb.com] > > > > Sent: Friday, August 3, 2018 6:00 AM > > > > To: David S . Miller > > > > Cc: Dave Watson ; Vakul Garg > > > > ; Boris Pismenny ; > Aviad > > > > Yehezkel ; netdev@vger.kernel.org; Doron > > > > Roberts-Kedes > > > > Subject: [PATCH net-next] net/tls: Calculate nsg for zerocopy path > > > > without skb_cow_data. > > > > > > > > decrypt_skb fails if the number of sg elements required to map is > > > > greater than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must > > > > always be calculated, but skb_cow_data adds unnecessary memcpy's > > > > for the zerocopy case. > > > > > > > > The new function skb_nsg calculates the number of scatterlist > > > > elements required to map the skb without the extra overhead of > > > > skb_cow_data. This function mimics the structure of skb_to_sgvec. > > > > > > > > Fixes: c46234ebb4d1 ("tls: RX path for ktls") > > > > Signed-off-by: Doron Roberts-Kedes > > > > --- > > > > net/tls/tls_sw.c | 89 > > > > ++-- > > > > 1 file changed, 86 insertions(+), 3 deletions(-) > > > > > > > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE); > > > > if (!sgout) { > > > > - nsg = skb_cow_data(skb, 0, &unused) + 1; > > > > + nsg = skb_cow_data(skb, 0, &unused); > > > > + } else { > > > > + nsg = skb_nsg(skb, > > > > + rxm->offset + tls_ctx->rx.prepend_size, > > > > + rxm->full_len - tls_ctx->rx.prepend_size); > > > > + if (nsg <= 0) > > > > + return nsg; > > > Comparison should be (nsg < 1). TLS forbids '0' sized records. > > > > Yes true, v2 incoming > > > > Glancing at this a second time, I actually don't believe this should be > changed. nsg <= 0 is equivalent to nsg < 1. Correct. > Returning 0 if the record is > 0 sized is the proper behavior here, since decrypting a zero-length skb is a > no- > op. It is true that zero sized TLS records are forbidden, but it is confusing > to > enforce that in this part of the code. I would be surpised to learn that > tls_sw_recvmsg could be invoked with a len equal to 0, but if I'm wrong, and > that case does need to be handled, then it should be in a different patch.
Re: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
On Fri, Aug 03, 2018 at 11:49:02AM -0700, Doron Roberts-Kedes wrote: > On Fri, Aug 03, 2018 at 01:23:33AM +, Vakul Garg wrote: > > > > > > > -Original Message- > > > From: Doron Roberts-Kedes [mailto:doro...@fb.com] > > > Sent: Friday, August 3, 2018 6:00 AM > > > To: David S . Miller > > > Cc: Dave Watson ; Vakul Garg > > > ; Boris Pismenny ; Aviad > > > Yehezkel ; netdev@vger.kernel.org; Doron > > > Roberts-Kedes > > > Subject: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without > > > skb_cow_data. > > > > > > decrypt_skb fails if the number of sg elements required to map is greater > > > than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must always be > > > calculated, but skb_cow_data adds unnecessary memcpy's for the zerocopy > > > case. > > > > > > The new function skb_nsg calculates the number of scatterlist elements > > > required to map the skb without the extra overhead of skb_cow_data. This > > > function mimics the structure of skb_to_sgvec. > > > > > > Fixes: c46234ebb4d1 ("tls: RX path for ktls") > > > Signed-off-by: Doron Roberts-Kedes > > > --- > > > net/tls/tls_sw.c | 89 > > > ++-- > > > 1 file changed, 86 insertions(+), 3 deletions(-) > > > > > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE); > > > if (!sgout) { > > > - nsg = skb_cow_data(skb, 0, &unused) + 1; > > > + nsg = skb_cow_data(skb, 0, &unused); > > > + } else { > > > + nsg = skb_nsg(skb, > > > + rxm->offset + tls_ctx->rx.prepend_size, > > > + rxm->full_len - tls_ctx->rx.prepend_size); > > > + if (nsg <= 0) > > > + return nsg; > > Comparison should be (nsg < 1). TLS forbids '0' sized records. > > Yes true, v2 incoming > Glancing at this a second time, I actually don't believe this should be changed. nsg <= 0 is equivalent to nsg < 1. Returning 0 if the record is 0 sized is the proper behavior here, since decrypting a zero-length skb is a no-op. It is true that zero sized TLS records are forbidden, but it is confusing to enforce that in this part of the code. I would be surpised to learn that tls_sw_recvmsg could be invoked with a len equal to 0, but if I'm wrong, and that case does need to be handled, then it should be in a different patch.
Re: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
On Fri, Aug 03, 2018 at 01:23:33AM +, Vakul Garg wrote: > > > > -Original Message- > > From: Doron Roberts-Kedes [mailto:doro...@fb.com] > > Sent: Friday, August 3, 2018 6:00 AM > > To: David S . Miller > > Cc: Dave Watson ; Vakul Garg > > ; Boris Pismenny ; Aviad > > Yehezkel ; netdev@vger.kernel.org; Doron > > Roberts-Kedes > > Subject: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without > > skb_cow_data. > > > > decrypt_skb fails if the number of sg elements required to map is greater > > than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must always be > > calculated, but skb_cow_data adds unnecessary memcpy's for the zerocopy > > case. > > > > The new function skb_nsg calculates the number of scatterlist elements > > required to map the skb without the extra overhead of skb_cow_data. This > > function mimics the structure of skb_to_sgvec. > > > > Fixes: c46234ebb4d1 ("tls: RX path for ktls") > > Signed-off-by: Doron Roberts-Kedes > > --- > > net/tls/tls_sw.c | 89 > > ++-- > > 1 file changed, 86 insertions(+), 3 deletions(-) > > > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index > > ff3a6904a722..c62793601cfc 100644 > > --- a/net/tls/tls_sw.c > > +++ b/net/tls/tls_sw.c > > @@ -43,6 +43,76 @@ > > > > #define MAX_IV_SIZETLS_CIPHER_AES_GCM_128_IV_SIZE > > > > +static int __skb_nsg(struct sk_buff *skb, int offset, int len, > > + unsigned int recursion_level) > > +{ > > +int start = skb_headlen(skb); > > +int i, copy = start - offset; > > +struct sk_buff *frag_iter; > > +int elt = 0; > > + > > +if (unlikely(recursion_level >= 24)) > > +return -EMSGSIZE; > > + > > +if (copy > 0) { > > +if (copy > len) > > +copy = len; > > +elt++; > > +if ((len -= copy) == 0) > > +return elt; > > +offset += copy; > > +} > > + > > +for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > > +int end; > > + > > +WARN_ON(start > offset + len); > > + > > +end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]); > > +if ((copy = end - offset) > 0) { > > +if (copy > len) > > +copy = len; > > +elt++; > > +if (!(len -= copy)) > > +return elt; > > +offset += copy; > > +} > > +start = end; > > +} > > + > > +skb_walk_frags(skb, frag_iter) { > > +int end, ret; > > + > > +WARN_ON(start > offset + len); > > + > > +end = start + frag_iter->len; > > +if ((copy = end - offset) > 0) { > > + > > +if (copy > len) > > +copy = len; > > +ret = __skb_nsg(frag_iter, offset - start, copy, > > + recursion_level + 1); > > +if (unlikely(ret < 0)) > > +return ret; > > +elt += ret; > > +if ((len -= copy) == 0) > > +return elt; > > +offset += copy; > > +} > > +start = end; > > +} > > +BUG_ON(len); > > +return elt; > > +} > > + > > +/* Return the number of scatterlist elements required to completely map > > +the > > + * skb, or -EMSGSIZE if the recursion depth is exceeded. > > + */ > > +static int skb_nsg(struct sk_buff *skb, int offset, int len) { > > + return __skb_nsg(skb, offset, len, 0); } > > + > > These is generic function and useful elsewhere too. > Should the above two functions be exported by skbuff.c? True. Perhaps it can move into skbuff.c if/when there is a second use case for it. > > > static int tls_do_decryption(struct sock *sk, > > struct scatterlist *sgin, > > struct scatterlist *sgout, > > @@ -693,7 +763,7 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb, > > struct scatterlist sgin_arr[MAX_SKB_FRAGS + 2]; > > struct scatterlist *sgin = &sgin_arr[0]; > > struct strp_msg *rxm = strp_msg(skb); > > - int ret, nsg = ARRAY_SIZE(sgin_arr); > > + int ret, nsg; > > struct sk_buff *unused; > > > > ret = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE, @@ - > > 704,10 +774,23 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb, > > > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE); > > if (!sgout) { > > - nsg = skb_cow_data(skb, 0, &unused) + 1; > > + nsg = skb_cow_data(skb, 0, &unused); > > + } else { > > + nsg = skb_nsg(skb, > > + rx
RE: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
> -Original Message- > From: Doron Roberts-Kedes [mailto:doro...@fb.com] > Sent: Friday, August 3, 2018 6:00 AM > To: David S . Miller > Cc: Dave Watson ; Vakul Garg > ; Boris Pismenny ; Aviad > Yehezkel ; netdev@vger.kernel.org; Doron > Roberts-Kedes > Subject: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without > skb_cow_data. > > decrypt_skb fails if the number of sg elements required to map is greater > than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must always be > calculated, but skb_cow_data adds unnecessary memcpy's for the zerocopy > case. > > The new function skb_nsg calculates the number of scatterlist elements > required to map the skb without the extra overhead of skb_cow_data. This > function mimics the structure of skb_to_sgvec. > > Fixes: c46234ebb4d1 ("tls: RX path for ktls") > Signed-off-by: Doron Roberts-Kedes > --- > net/tls/tls_sw.c | 89 > ++-- > 1 file changed, 86 insertions(+), 3 deletions(-) > > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index > ff3a6904a722..c62793601cfc 100644 > --- a/net/tls/tls_sw.c > +++ b/net/tls/tls_sw.c > @@ -43,6 +43,76 @@ > > #define MAX_IV_SIZE TLS_CIPHER_AES_GCM_128_IV_SIZE > > +static int __skb_nsg(struct sk_buff *skb, int offset, int len, > +unsigned int recursion_level) > +{ > +int start = skb_headlen(skb); > +int i, copy = start - offset; > +struct sk_buff *frag_iter; > +int elt = 0; > + > +if (unlikely(recursion_level >= 24)) > +return -EMSGSIZE; > + > +if (copy > 0) { > +if (copy > len) > +copy = len; > +elt++; > +if ((len -= copy) == 0) > +return elt; > +offset += copy; > +} > + > +for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) { > +int end; > + > +WARN_ON(start > offset + len); > + > +end = start + skb_frag_size(&skb_shinfo(skb)->frags[i]); > +if ((copy = end - offset) > 0) { > +if (copy > len) > +copy = len; > +elt++; > +if (!(len -= copy)) > +return elt; > +offset += copy; > +} > +start = end; > +} > + > +skb_walk_frags(skb, frag_iter) { > +int end, ret; > + > +WARN_ON(start > offset + len); > + > +end = start + frag_iter->len; > +if ((copy = end - offset) > 0) { > + > +if (copy > len) > +copy = len; > +ret = __skb_nsg(frag_iter, offset - start, copy, > + recursion_level + 1); > +if (unlikely(ret < 0)) > +return ret; > +elt += ret; > +if ((len -= copy) == 0) > +return elt; > +offset += copy; > +} > +start = end; > +} > +BUG_ON(len); > +return elt; > +} > + > +/* Return the number of scatterlist elements required to completely map > +the > + * skb, or -EMSGSIZE if the recursion depth is exceeded. > + */ > +static int skb_nsg(struct sk_buff *skb, int offset, int len) { > + return __skb_nsg(skb, offset, len, 0); } > + These is generic function and useful elsewhere too. Should the above two functions be exported by skbuff.c? > static int tls_do_decryption(struct sock *sk, >struct scatterlist *sgin, >struct scatterlist *sgout, > @@ -693,7 +763,7 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb, > struct scatterlist sgin_arr[MAX_SKB_FRAGS + 2]; > struct scatterlist *sgin = &sgin_arr[0]; > struct strp_msg *rxm = strp_msg(skb); > - int ret, nsg = ARRAY_SIZE(sgin_arr); > + int ret, nsg; > struct sk_buff *unused; > > ret = skb_copy_bits(skb, rxm->offset + TLS_HEADER_SIZE, @@ - > 704,10 +774,23 @@ int decrypt_skb(struct sock *sk, struct sk_buff *skb, > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE); > if (!sgout) { > - nsg = skb_cow_data(skb, 0, &unused) + 1; > + nsg = skb_cow_data(skb, 0, &unused); > + } else { > + nsg = skb_nsg(skb, > + rxm->offset + tls_ctx->rx.prepend_size, > + rxm->full_len - tls_ctx->rx.prepend_size); > + if (nsg <= 0) > + return nsg; Comparison should be (nsg < 1). TLS forbids '0' sized records. > + } > + > + // We need one extra for ctx->rx_aad_ciphertext > + nsg++; > + > + if (nsg > ARRAY_SIZE(sgin_arr)) >