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 for Android: free password hash cracker in your pocket

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

Message-ID: <20180725210103.GA74476@davejwatson-mba.dhcp.thefacebook.com>
Date:   Wed, 25 Jul 2018 14:01:03 -0700
From:   Dave Watson <davejwatson@...com>
To:     Vakul Garg <vakul.garg@....com>
CC:     David Miller <davem@...emloft.net>,
        "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.

On 07/24/18 08:22 AM, Vakul Garg wrote:
> > 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'?

There is nothing special about it, in the zerocopy-fastpath if we
happen to fit in MAX_SKB_FRAGS we avoid any kmalloc whatsoever though.
It could be renamed MAX_SC_FOR_FASTPATH or something.

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

Yes we could do this, and yes we would need to heap allocate if you
want to support multiple outstanding decryption requests.  I think
async crypto prevents any sort of zerocopy-fastpath, however.