netdev - Re: [PATCH net-next,v5 3/4] net: flow_offload: mangle action at byte level

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: <20191017103059.3b7ff828@cakuba.netronome.com>
Date:   Thu, 17 Oct 2019 10:30:59 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, jiri@...nulli.us, saeedm@...lanox.com,
        vishal@...lsio.com, vladbu@...lanox.com, ecree@...arflare.com
Subject: Re: [PATCH net-next,v5 3/4] net: flow_offload: mangle action at
 byte level

On Thu, 17 Oct 2019 18:11:57 +0200, Pablo Neira Ayuso wrote:
> Hello Jakub,
> 
> On Wed, Oct 16, 2019 at 04:36:51PM -0700, Jakub Kicinski wrote:
> > Let's see if I can recount the facts:
> >  (1) this is a "improvement" to simplify driver work but driver
> >      developers (Ed and I) don't like it;  
> 
> Ed requested to support for partial mangling of header fields. This
> patchset already supports for this, eg. mangle one single byte of a
> TCP port.

Ed said:

As Jakub said, 'We suffered through enough haphazard "updates"'.  Please
 can you fix the problems your previous API changes caused (I still haven't
 had an answer about the flow block changes since sending you my driver code
 two weeks ago) before trying to ram new ones through.

> >  (2) it's supposed to simplify things yet it makes the code longer;  
> 
> The driver codebase is simplified at the cost of adding more frontend
> code which is common to everyone.
> 
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c |  162 +++---------
>  drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.h |   40 ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c      |   80 ++----
>  drivers/net/ethernet/netronome/nfp/flower/action.c   |  191 ++++++--------

Well selected. Also part of the savings here are patch 2 which no one
objects to :/

> >  (3) it causes loss of functionality (looks like a single u32 changing
> >      both sport and dport is rejected by the IR since it wouldn't
> >      match fields);  
> 
> Not correct.

As you discovered yourself in the follow up, it is correct.

> tc filter add dev eth0 protocol ip \
>         parent ffff: \
>         pref 11 \
>         flower ip_proto tcp \
>         dst_port 80 \
>         src_ip 1.1.2.3/24 \
>         action pedit ex munge tcp src 2004 \
>         action pedit ex munge tcp dst 80
> 
> This results in two independent tc pedit actions:
> 
> * One tc pedit action with one single key, with value 0xd4070000 /
>   0x0000ffff.
> * Another tc pedit action with one single key, with value 0x00005000
>   / 0xffff0000.
> 
> This works perfectly with this patchset.
>
> >  (4) at v5 it still is buggy (see below).  
> 
> That, I can fix, thank you for reporting.

You keep breaking flow offloads are we are tired of it.

> > The motivation for this patch remains unclear.  
> 
> The motivation is to provide a representation for drivers that is
> easier to interpret. Have a look at the nfp driver and tell me if it
> not easier to follow. This is already saving complexity from the
> drivers.

Obviously IMO it's just churn, otherwise why would I object?

You keep repeating that it's making drivers better yet the only driver
authors who respond to you are against this change.

How are you not seeing the irony of that?


Ed requested this was a opt-in/helper the driver can call if they
choose to. Please do that. Please provide selftests.

Thank you.