netdev - Re: CONFIG_NET_TC_SKB

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]

Date:   Thu, 26 Sep 2019 18:02:25 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Paul Blakey <paulb@...lanox.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     Pravin Shelar <pshelar@....org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Vlad Buslov <vladbu@...lanox.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Or Gerlitz <gerlitz.or@...il.com>
Subject: Re: CONFIG_NET_TC_SKB_EXT

On 26/09/2019 16:14, Paul Blakey wrote:
> On 9/26/2019 5:26 PM, Edward Cree wrote:
>> On 26/09/2019 14:56, Paul Blakey wrote:
>>>>> In nat scenarios the packet will be modified, and then there can be a miss:
>>>>>
>>>>>               -trk .... CT(zone X, Restore NAT),goto chain 1
>>>>>
>>>>>               +trk+est, match on ipv4, CT(zone Y), goto chain 2
>>>>>
>>>>>               +trk+est, output..
>>>> I'm confused, I thought the usual nat scenario looked more like
>>>>       0: -trk ... action ct(zone x), goto chain 1
>>>>       1: +trk+new ... action ct(commit, nat=foo) # sw only
>>>>       1: +trk+est ... action ct(nat), mirred eth1
>>>> i.e. the NAT only happens after conntrack has matched (and thus provided
>>>>    the saved NAT metadata), at the end of the pipe.  I don't see how you
>>>>    can NAT a -trk packet.
>>> Both are valid, Nat in the first hop, executes the nat stored on the
>>> connection if available (configured by commit).
>> This still isn't making sense to me.
>> Until you've done a conntrack lookup and found the connection, you can't
>>   use NAT information that's stored in the connection.
>> So the NAT can only happen after a conntrack match is found.
> That's how it works, CT action restores the metadata (nf_conn on the 
> SKB), you can then, if needed,  execute the nat,
Yes, and 'setting metadata' is idempotent (or reversible, if you prefer)
 so it doesn't matter if HW does it and then says "oops, can't do this"
 and software path starts from chain 0.
Problems only appear if you have _more matching_ after executing the NAT,
 in which case you're not a "simple NAT use-case" any more but rather the
 more complicated thing.

> The change didn't cause any problem
I mean, this whole thread is here because it *did* cause a problem:
 skb-extensions being turned on when not really needed.

> As we are discussing the config default, I've sent a patch to set the
> config to N.
As others have pointed out upthread, distros will just enable it anyway,
 regardless of the default.  As Daniel said,
> Adding new skb extensions should really have a strong justification behind
> it (close but slightly less strict to how we treat adding new members to
> skb itself). 
I'd also like to see a clear explanation of why you need extra out-of-skb
 storage space for this and can't use skb->tc_index as suggested by Pravin.