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]

Message-ID: <20190906131457.7olkal45kkdtbevo@salvia>
Date:   Fri, 6 Sep 2019 15:14:57 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Edward Cree <ecree@...arflare.com>
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 Fri, Sep 06, 2019 at 01:55:29PM +0100, Edward Cree wrote:
> On 06/09/2019 11:56, Pablo Neira Ayuso wrote:
> > On Fri, Sep 06, 2019 at 11:02:18AM +0100, Edward Cree wrote:
> >> Still NAK for the same reasons as three versions ago (when it was called
> >>  "netfilter: payload mangling offload support"), you've never managed to
> >>  explain why this extra API complexity is useful.  (Reducing LOC does not
> >>  mean you've reduced complexity.)
> > Oh well...
> >
> > Patch 1) Mask is inverted for no reason, just because tc pedit needs
> > this in this way. All drivers reverse this mask.
> >
> > Patch 2) All drivers mask out meaningless fields in the value field.
>
> To be clear: I have no issue with these two patches; they look fine to me.
> (Though I'd like to see some comments on struct flow_action_entry specifying
>  the semantics of these fields, especially if they're going to differ from
>  the corresponding fields in struct tc_pedit_key.)

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.

> > Patch 3) Without this patchset, offsets are on the 32-bit boundary.
> > Drivers need to play with the 32-bit mask to infer what field they are
> > supposed to mangle... eg. with 32-bit offset alignment, checking if
> > the use want to alter the ttl/hop_limit need for helper structures to
> > check the 32-bit mask. Mangling a IPv6 address comes with one single
> > action...
>
> Drivers are still going to need to handle multiple pedit actions, in
>  case the original rule wanted to mangle two non-consecutive fields.
> 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.

I'll clarify this in the cover letter in the next patchset round.

>  So either have the core convert things into named protocol fields
>  (i.e. "set src IPv6 to 1234::5 and add 1 to UDP sport"), or leave
>  the current sequence-of-u32-mangles as it is. This in-between "we'll
>  coalesce things together despite not knowing what fields they are" is
>  neither fish nor fowl.

The model you propose would still need this code for tc pedit to
adjust offset/length and coalesce u32 fields.