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: <347b446d-ec80-9b8a-6678-86a6c3eddf6a@ucloud.cn>
Date:   Thu, 27 Aug 2020 11:30:54 +0800
From:   wenxu <wenxu@...oud.cn>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "David S. Miller" <davem@...emloft.net>
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 8/25/2020 11:33 PM, Marcelo Ricardo Leitner wrote:
> 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.

Yes, there is no direct relation with conntrack.
The re-fragmenting based on the qdisc_skb_cb(skb)->mru which only setting
in the act_ct. So I named this to ct_xx.

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

I think it is better put this feature to act_mirred. And it also not enable by default.

Only packet with qdisc_skb_cb(skb)->mru which means this packet just do reassemble

before conntrack.

So only othersolution for solving this problem?


BR

wenxu

>
>   Marcelo
>