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: <CAM_iQpUmQt-264XNjaXaU4EbDmM3ub=8r-LKwMbF7dFgi99DRw@mail.gmail.com>
Date:   Wed, 25 Nov 2020 21:14:30 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     wenxu <wenxu@...oud.cn>, Jakub Kicinski <kuba@...nel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Vlad Buslov <vladbu@...dia.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:04 PM Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
>
> 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.

Yeah, I think compiler is able to compile out unused code with LTO, so
probably not a big deal here.

Thanks.