netdev - Re: [PATCH v4 net-next 3/3] net/sched: sch_frag: add generic packet fragment support.

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: <20201125200407.GB449907@localhost.localdomain>
Date:   Wed, 25 Nov 2020 17:04:07 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     wenxu@...oud.cn
Cc:     kuba@...nel.org, jhs@...atatu.com, netdev@...r.kernel.org,
        vladbu@...dia.com, xiyou.wangcong@...il.com
Subject: Re: [PATCH v4 net-next 3/3] net/sched: sch_frag: add generic packet
 fragment support.

On Wed, Nov 25, 2020 at 12:01:23PM +0800, wenxu@...oud.cn wrote:
> From: wenxu <wenxu@...oud.cn>
> 
> Currently kernel tc subsystem can do conntrack in cat_ct. But when several
                                               typo ^^^

> fragment packets go through the act_ct, function tcf_ct_handle_fragments
> will defrag the packets to a big one. But the last action will redirect
> mirred to a device which maybe lead the reassembly big packet over the mtu
> of target device.
> 
> This patch add support for a xmit hook to mirred, that gets executed before
> xmiting the packet. Then, when act_ct gets loaded, it configs that hook.
> The frag xmit hook maybe reused by other modules.

(I'm back from PTO)

This paragraph was kept from previous version and now although it can
match the current implementation, it's a somewhat forced
understanding. So what about:
"""
This patch adds support for a xmit hook to mirred, that gets executed
before xmiting the packet. Then, when act_ct gets loaded, it enables
such hook.
The hook may also be enabled by other modules.
"""

Rationale is to not give room for the understanding that the hook is
configurable (i.e., replaceable with something else), to cope with v4
changes.

> 
> Signed-off-by: wenxu <wenxu@...oud.cn>
> ---
> v2: make act_frag just buildin for tc core but not a module
>     return an error code from tcf_fragment
>     depends on INET for ip_do_fragment
> v3: put the whole sch_frag.c under CONFIG_INET

I was reading on past discussions that led to this and I miss one
point of discussion. Cong had mentioned that as this is a must have
for act_ct, that we should get rid of user visible Kconfigs for it
(which makes sense).  v3 then removed the Kconfig entirely.
My question then is: shouldn't it have an *invisible* Kconfig instead?
As is, sch_frag will be always enabled, regardless of having act_ct
enabled or not.

I don't think it's worth tiying this to act_ct itself, as I think
other actions can do defrag later on or so. So I'm thinking act_ct
could select this other Kconfig, that depends on INET, and use it to
enable/disable building this code.

> v4: remove the abstraction for xmit_hook 

Other than this, LGTM.