> -----Original Message-----
> From: Dave Watson [mailto:davejwat...@fb.com]
> Sent: Monday, July 23, 2018 10:05 PM
> To: David Miller <da...@davemloft.net>
> Cc: Vakul Garg <vakul.g...@nxp.com>; netdev@vger.kernel.org;
> bor...@mellanox.com; avia...@mellanox.com; Doron Roberts-Kedes
> <doro...@fb.com>
> Subject: Re: [net-next v5 3/3] net/tls: Remove redundant array allocation.
>
> On 07/21/18 07:25 PM, David Miller wrote:
> > From: Vakul Garg <vakul.g...@nxp.com>
> > Date: Thu, 19 Jul 2018 21:56:13 +0530
> >
> > > In function decrypt_skb(), array allocation in case when sgout is
> > > NULL is unnecessary. Instead, local variable sgin_arr[] can be used.
> > >
> > > Signed-off-by: Vakul Garg <vakul.g...@nxp.com>
> >
> > Hmmm...
> >
> > Dave, can you take a look at this? Do you think there might have been
> > a reason you felt that you needed to dynamically allocate the
> > scatterlists when you COW and skb and do in-place decryption?
> >
> > I guess this change is ok, nsg can only get smaller when the SKB is
> > COW'd.
> >
>
> > > memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
> > > if (!sgout) {
> > > nsg = skb_cow_data(skb, 0, &unused) + 1;
> > > - sgin = kmalloc_array(nsg, sizeof(*sgin),
> > > sk->sk_allocation);
> > > sgout = sgin;
> > > }
>
> I don't think this patch is safe as-is. sgin_arr is a stack array of size
> MAX_SKB_FRAGS (+ overhead), while my read of skb_cow_data is that it
> walks the whole chain of skbs from skb->next, and can return any number of
> segments. Therefore we need to heap allocate. I think I copied the IPSEC
> code here.
>
> For perf though, we could use the stack array if skb_cow_data returns <=
> MAX_SKB_FRAGS.
skb_cow_data() is being called only when caller passes sgout=NULL (i.e.
non-zero copy case). In case of zero-copy, we do not call skb_cow_data()
and just assume that MAX_SKB_FRAGS+2 sized scatterlist array sgin_arr[]
is sufficient. This assumption could be wrong. So skb_cow_data() should be
called unconditionally to determine number of scatterlist entries required
for skb.
>
> This code is slightly confusing though, since we don't heap allocate in the
> zerocopy case - what happens is that skb_to_sgvec returns -EMSGSIZE, and
> we fall back to the non-zerocopy case, and return again to this function,
> where we then hit the skb_cow_data path and heap allocate.
If skb_to_sgvec return -EMSGSIZE, decrypt_skb() would return failure,
resulting in abort of TLS session.
