netdev - Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow

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:   Mon, 20 Aug 2018 17:27:23 -0700
From:   Doron Roberts-Kedes <doronrk@...com>
To:     David Miller <davem@...emloft.net>
CC:     <davejwatson@...com>, <vakul.garg@....com>, <borisp@...lanox.com>,
        <aviadye@...lanox.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path
 without skb_cow_data.

On Sat, Aug 11, 2018 at 11:54:53AM -0700, David Miller wrote:
> From: Doron Roberts-Kedes <doronrk@...com>
> Date: Thu, 9 Aug 2018 15:43:44 -0700
> 
> The reason is that we usually never need to "map" an SKB on receive,
> and on transmit the SKB geometry is normalized by the destination
> device features which means no frag_list.
> 
> And the other existing receive path doing SW crypto, IPSEC, always
> COWs the data so get the number of SG array entries needed from
> skb_cow_data().

Makes sense, thanks! 

> Frag lists on RX should be pretty rare.  They occur when GRO
> segmentation encouters poorly arranged incoming SKBs.  For example
> this happens if the incoming frames use lots of tiny SKB page frags,
> and therefore prevent accumulation into the page frag array of the
> head GRO skb.
> 
> So yes it can happen, and we have to account for it.
> 
> So I guess you really do need to accomodate this situation.  Why
> don't you try to rearrange your new function with some likely()
> and unlikely() tests so that the straight code path optimizes for
> the non-frag-list case?

So I did some poking around and found that basically 100% of skb's
recieved by ktls have a frag_list because of how strparser is
implemented. skb's from TCP that do not a have a frag_list are
accumulated by strparser using frag_list of a new skb. skb's from TCP
that do have a frag_list can become part of skb's with nested frag_lists
(skb's with non-NULL frag_list that are themselves part of a frag_list).
Unfortunatley, frag_list seems to be the common case, so is probably not a
good candidate for an unlikely() test. 

Regarding nested frag_list's more generally, is a good rule of thumb
that multiple layers of frag_list will not occur except for
post-processing such as in strparser? When should skb-handling code be
prepared for nested frag_lists? 

Given that frag_lists are not unlikely in this case, I believe the only
remaining feedback on the original patch was the recursive
implementation. If you'd like, I can re-submit with an iterative
implementation, but I noticed that goes against the existing recursive
pattern in functions like skb_release_data -> kfree_skb_list -> kfree_skb 
-> __kfree_skb -> skb_release_all -> skb_release_data, as well as
skb_to_sgvec. Let me know whether an iterative implementation is
preferred here, or whether I can simply rebase and resubmit a patch
similar to the original (modulo some variable renaming improvements).