netdev - Re: [PATCH net-next,v3 0/4] flow_offload: update mangle action representation

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:   Fri, 6 Sep 2019 14:37:16 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
CC:     <netfilter-devel@...r.kernel.org>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <jakub.kicinski@...ronome.com>,
        <jiri@...nulli.us>, <saeedm@...lanox.com>, <vishal@...lsio.com>,
        <vladbu@...lanox.com>
Subject: Re: [PATCH net-next,v3 0/4] flow_offload: update mangle action
 representation

On 06/09/2019 14:14, Pablo Neira Ayuso wrote:
> OK, I can document this semantics, I need just _time_ to write that
> documentation. I was expecting this patch description is enough by now
> until I can get to finish that documentation.
I think for two structs with apparently the same contents but different
 semantics (one has the mask bitwise complemented) it's best to hold up
 the code change until the comment is ready to come with it, because
 until then it's a dangerously confusing situation.

>> And you can't just coalesce all consecutive mangles, because if you
>>  mangle two consecutive fields (e.g. UDP sport and dport) the driver
>>  still needs to disentangle that if it works on a 'fields' (rather
>>  than 'u32s') level.
> This infrastructure is _not_ coalescing two consecutive field, e.g.
> UDP sport and dport is _not_ coalesced. The coalesce routine does
> _not_ handle multiple tc pedit ex actions.
So an IPv6 address mangle only comes as a single action if it's from
 netfilter, not if it's coming from TC pedit.  Therefore drivers still
 need to handle an IPv6 or MAC address mangle coming in multiple
 actions, therefore your driver simplifications are invalid.  No?

> The model you propose would still need this code for tc pedit to
> adjust offset/length and coalesce u32 fields.
Yes, but we don't add code/features to the kernel based on what we
 *could* use it for later; every submission has to be self-contained
 in providing something of demonstrable value.  So either implement
 "the model I propose" (which to be clear I'm *not* proposing, I want
 the u32 pedit left as it is; it's just that it's a better model than
 what you're implementing here), or leave well alone.

-Ed