netdev - RE: [net-next v5 3/3] net/tls: Remove redundant array allocation.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Jul 2018 08:22:02 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Dave Watson <davejwatson@...com>,
        David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "borisp@...lanox.com" <borisp@...lanox.com>,
        "aviadye@...lanox.com" <aviadye@...lanox.com>,
        Doron Roberts-Kedes <doronrk@...com>
Subject: RE: [net-next v5 3/3] net/tls: Remove redundant array allocation.



> -----Original Message-----
> From: Dave Watson [mailto:davejwatson@...com]
> Sent: Monday, July 23, 2018 10:05 PM
> To: David Miller <davem@...emloft.net>
> Cc: Vakul Garg <vakul.garg@....com>; netdev@...r.kernel.org;
> borisp@...lanox.com; aviadye@...lanox.com; Doron Roberts-Kedes
> <doronrk@...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.garg@....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.garg@....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.
> 
> 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.

Thanks for explaining. 
I am missing the point why MAX_SKB_FRAGS sized local array 
sgin has been used in tls_sw_recvmsg(). What is special about MAX_SKB_FRAGS so
that we used it as a size factor for 'sgin'?

Will it be a bad idea to get rid of array 'sgin' on stack and simply kmalloc 'sgin' for 
whatever the number the number of pages returned by iov_iter_npages()?
We can allocate for sgout too with the same kmalloc().

(Using a local array based 'sgin' is coming in the way to achieve sending multiple async
record decryption requests to the accelerator without waiting for previous one to complete.)