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] [day] [month] [year] [list]

Message-ID: <20180801205207.GB6180@davejwatson-mba.dhcp.thefacebook.com>
Date:   Wed, 1 Aug 2018 13:52:07 -0700
From:   Dave Watson <davejwatson@...com>
To:     Vakul Garg <vakul.garg@....com>
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>,
        David Miller <davem@...emloft.net>
Subject: Re: [net-next v5 3/3] net/tls: Remove redundant array allocation.

On 08/01/18 01:49 PM, 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.
> 
> 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.

I agree it is best to unify them.  I was originally worried about perf
with the extra allocation (which you proposed fixing by merging with
the crypto allocation, which would be great), and the perf of
skb_cow_data().  Zerocopy doesn't require skb_cow_data(), but we do
have to walk the skbs to calculate nsg correctly.

However skb_cow_data perf might be fine after your fix "strparser: Call
skb_unclone conditionally".