RE: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow_data.

2018-08-06 Thread Vakul Garg



> -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.

2018-08-06 Thread Doron Roberts-Kedes
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.

2018-08-03 Thread Doron Roberts-Kedes
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.

2018-08-02 Thread Vakul Garg



> -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))
>