On Fri, Aug 03, 2018 at 11:49:02AM -0700, Doron Roberts-Kedes wrote: > On Fri, Aug 03, 2018 at 01:23:33AM +0000, 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 <da...@davemloft.net> > > > Cc: Dave Watson <davejwat...@fb.com>; Vakul Garg > > > <vakul.g...@nxp.com>; Boris Pismenny <bor...@mellanox.com>; Aviad > > > Yehezkel <avia...@mellanox.com>; netdev@vger.kernel.org; Doron > > > Roberts-Kedes <doro...@fb.com> > > > 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 <doro...@fb.com> > > > --- > > > 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.
