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 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: <0e9a1701-356f-5f94-b88e-a39175dee77a@iogearbox.net>
Date:   Sat, 21 Sep 2019 00:15:16 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Vlad Buslov <vladbu@...lanox.com>
Cc:     David Miller <davem@...emloft.net>,
        Paul Blakey <paulb@...lanox.com>,
        "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>,
        Pravin Shelar <pshelar@....org>,
        Simon Horman <simon.horman@...ronome.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Or Gerlitz <gerlitz.or@...il.com>
Subject: Re: CONFIG_NET_TC_SKB_EXT

On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@...emloft.net> wrote:
>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>> is really not acceptable.
>>>
>>> It should not be enabled by default at all.
>>>
>>> Instead the actual users should turn it on or depend upon it, which in
>>> this case seems to be OVS.
>>>
>>> Please fix this, thank you.
>>
>> Hi David,
>>
>> We are working on it, but Paul is OoO today. Is it okay if we send the
>> fix early next week?
> 
> Doesn't really seem like we have too many ways forward here, right?
> 
> How about this?
> 
> ------>8-----------------------------------
> 
> net: hide NET_TC_SKB_EXT as a config option
> 
> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> Indeed, it should really be selected to ensure correct OvS operation
> if TC offload is used. Hopefully those who care about TC-only and
> OvS-only performance disable the other one at compilation time.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> ---
>   net/openvswitch/Kconfig |  1 +
>   net/sched/Kconfig       | 13 +++----------
>   2 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 22d7d5604b4c..bd407ea7c263 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -15,6 +15,7 @@ config OPENVSWITCH
>   	select NET_MPLS_GSO
>   	select DST_CACHE
>   	select NET_NSH
> +	select NET_TC_SKB_EXT if NET_CLS_ACT
>   	---help---
>   	  Open vSwitch is a multilayer Ethernet switch targeted at virtualized
>   	  environments.  In addition to supporting a variety of features
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index b3faafeafab9..f1062ef55098 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
>   config NET_CLS_ACT
>   	bool "Actions"
>   	select NET_CLS
> +	select NET_TC_SKB_EXT if OPENVSWITCH

But how would that make much of a difference :( Distros are still going to
enable all of this blindlessly. Given discussion in [0], could we just get
rid of this tasteless hack altogether which is for such a narrow use case
anyway?

I thought idea of stuffing things into skb extensions are only justified if
it's not enabled by default for everyone. :(

   [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u

>   	---help---
>   	  Say Y here if you want to use traffic control actions. Actions
>   	  get attached to classifiers and are invoked after a successful
> @@ -964,18 +965,10 @@ config NET_IFE_SKBTCINDEX
>           depends on NET_ACT_IFE
>   
>   config NET_TC_SKB_EXT
> -	bool "TC recirculation support"
> -	depends on NET_CLS_ACT
> -	default y if NET_CLS_ACT
> +	bool
> +	depends on NET_CLS_ACT && OPENVSWITCH
>   	select SKB_EXTENSIONS
>   
> -	help
> -	  Say Y here to allow tc chain misses to continue in OvS datapath in
> -	  the correct recirc_id, and hardware chain misses to continue in
> -	  the correct chain in tc software datapath.
> -
> -	  Say N here if you won't be using tc<->ovs offload or tc chains offload.
> -
>   endif # NET_SCHED
>   
>   config NET_SCH_FIFO
>