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: <20191017161157.rr4lrolsjbnmk3ke@salvia>
Date:   Thu, 17 Oct 2019 18:11:57 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
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

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.