netdev - Re: CONFIG_NET_TC_SKB

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

Date:   Mon, 23 Sep 2019 15:46:19 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Paul Blakey <paulb@...lanox.com>, Pravin Shelar <pshelar@....org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Vlad Buslov <vladbu@...lanox.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Or Gerlitz <gerlitz.or@...il.com>
Subject: Re: CONFIG_NET_TC_SKB_EXT

On Sun, Sep 22, 2019 at 02:47:15PM -0700, Jakub Kicinski wrote:
> On Sun, 22 Sep 2019 14:51:44 +0300, Paul Blakey wrote:
...
> > ------------------------------------------------------------
> > 
> > Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N
> > 
> > This a new feature, it is prefered that it defaults to N.
> > 
> > Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
> > Signed-off-by: Paul Blakey <paulb@...lanox.com>
> > ---
> >  net/sched/Kconfig | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> > index b3faafe..4bb10b7 100644
> > --- a/net/sched/Kconfig
> > +++ b/net/sched/Kconfig
> > @@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
> >  config NET_TC_SKB_EXT
> >  	bool "TC recirculation support"
> >  	depends on NET_CLS_ACT
> > -	default y if NET_CLS_ACT
> >  	select SKB_EXTENSIONS
> >  
> >  	help
> 
>  - Linus suggested we hide this option from user and autoselect if
>    possible.
>  - As Daniel said all distros will enable this.
>  - If correctness is really our concern, giving users an option to
>    select whether they want something to behave correctly seems like
>    a hack of lowest kind. Perhaps it's time to add a CONFIG for TC
>    offload in general? That's a meaningful set of functionality.

Now that we have the static branch in place on OvS code, maybe we can
remove this option and rely just on NET_CLS_ACT instead (such as in
cls_api.c and skbuff.h).

Probing by OvS userspace is still somewhat possible, as the flag
OVS_DP_F_TC_RECIRC_SHARING wouldn't be known and be rejected
otherwise. 'somewhat because there is a limitation, as prior versions
(prior to this commit) didn't validate if the flags being set were at
least known.  But this is not news.

General OvS performance won't suffer, thanks to the static branch.

Drivers won't allocate the skb extension just because. They will only
do it when tc offloading is being done and, as already explained in
the thread and other places, it's needed for correct operation. So
general performance is also ok here.

For tc sw datapath, maybe we can have another static branch in there,
more specifically at tcf_classify(). Not sure how to control/toggle
it, though.