netdev - Re: [PATCH net-next] net/sched: add act_ct

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: <20200825153318.GA2444@localhost.localdomain>
Date:   Tue, 25 Aug 2020 12:33:18 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     wenxu@...oud.cn
Cc:     netdev@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Paul Blakey <paulb@...lanox.com>, Oz Shlomo <ozsh@...lanox.com>
Subject: Re: [PATCH net-next] net/sched: add act_ct_output support

On Tue, Aug 25, 2020 at 02:07:43PM +0800, wenxu@...oud.cn wrote:
...
> +static LIST_HEAD(ct_output_list);
> +static DEFINE_SPINLOCK(ct_output_list_lock);
> +
> +#define CT_OUTPUT_RECURSION_LIMIT    4
> +static DEFINE_PER_CPU(unsigned int, ct_output_rec_level);

Wenxu, first of all, thanks for doing this.

Hopefully this helps to show how much duplicated code this means.
Later on, any bug that we find on mirrer, we also need to fix in
act_ct_output, which is not good.

Currently act_ct is the only one doing defrag and leading to this
need, but that may change in the future. The action here, AFAICT, has
nothing in specific to conntrack.  It is "just" re-fragmenting
packets. The only specific reference to nf/ct I could notice is for
the v6ops, to have access to ip6_fragment(), which can also be done
via struct ipv6_stub (once added there). That said, it shouldn't be
named after conntrack, to avoid future confusions.

I still don't understand Cong's argument for not having this on
act_mirred because TC is L2. That's actually not right. TC hooks at L2
but deals with L3 and L4 (after all, it does static NAT, mungles L4
headers and classifies based on virtually anything) since beginning,
and this is just another case.

What I can understand, is that this feature shouldn't be enabled by
default on mirred. So that we are sure that users opting-in know what
they are doing. It can have a "l3" flag, to enable L3 semantics, and
that's it. Code re-used, no performance drawback for pure L2 users (it
can even be protected by a static_key. Once a l3-enabled mirred is
loaded, enable it), user knows what to expect and no confusion on
which action to use.

  Marcelo