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

Powered by blists - more mailing lists