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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.

>  (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 ++++++--------

>  (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.

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.

> 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.

Thank you.