netdev - Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201114224617.GK3913@localhost.localdomain>
Date:   Sat, 14 Nov 2020 19:46:17 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     wenxu <wenxu@...oud.cn>, Jakub Kicinski <kuba@...nel.org>,
        Vlad Buslov <vladbu@...dia.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v10 net-next 3/3] net/sched: act_frag: add implict packet
 fragment support.

On Sat, Nov 14, 2020 at 10:05:39AM -0800, Cong Wang wrote:
> On Wed, Nov 11, 2020 at 9:44 PM <wenxu@...oud.cn> wrote:
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index 9c79fb9..dff3c40 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -1541,8 +1541,14 @@ static int __init ct_init_module(void)
> >         if (err)
> >                 goto err_register;
> >
> > +       err = tcf_set_xmit_hook(tcf_frag_xmit_hook);
> 
> Yeah, this approach is certainly much better than extending act_mirred.
> Just one comment below.

Nice. :-)

> 
> 
> > diff --git a/net/sched/act_frag.c b/net/sched/act_frag.c
> > new file mode 100644
> > index 0000000..3a7ab92
> > --- /dev/null
> > +++ b/net/sched/act_frag.c
> 
> It is kinda confusing to see this is a module. It provides some
> wrappers and hooks the dev_xmit_queue(), it belongs more to
> the core tc code than any modularized code. How about putting
> this into net/sched/sch_generic.c?

Davide had shared similar concerns with regards of the new module too.
The main idea behind the new module was to keep it as
isolated/contained as possible, and only so. So thumbs up from my
side. 

To be clear, you're only talking about the module itself, right? It
would still need to have the Kconfig to enable this feature, or not?

Thanks,
Marcelo