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: <20190922144715.37f71fbf@cakuba.netronome.com>
Date:   Sun, 22 Sep 2019 14:47:15 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Paul Blakey <paulb@...lanox.com>
Cc:     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, 22 Sep 2019 14:51:44 +0300, Paul Blakey wrote:
> The skb extension is currently used for miss path of software offloading OvS rules with recirculation to tc.
> However, we are also preparing patches to support the hardware side of things.
> 
> After the userspace OvS patches to support connection tracking, we'll have two users for
> tc multi chain rules, one from those actually using tc and those translated from OvS rules.
> 
> With both of these use cases, there is similar issue of 'miss', from hardware to tc and tc to OvS and the skb
> extension will serve to recover from both.
> 
> Consider, for example, the following multi chain tc rules:
> 
> tc filter add dev1 ... chain 0 flower dst_mac aa:bb:cc:dd:ee:ff action pedit munge ex set mac dst 02:a0:98:4e:8f:d1 pipe action goto chain 1
> tc filter add dev1 ... chain 1 flower ip_dst 1.1.1.1 action mirred egress redirect dev2
> tc filter add dev1 ... chain 1 flower ip_dst 2.2.2.2 action mirred egress redirect dev2
> 
> It's possible we offload the first two rules, but fail to offload the third rule, because of some hardware failure (e.g unsupported match or action).
> If a packet with (dst_mac=aa:bb:cc:dd:ee:ff) and (dst ip=2.2.2.2) arrives,
> we execute the goto chain 1 in hardware (and the pedit), and continue in chain 1, where we miss.
> 
> Currently we re-start the processing in tc from chain 0, even though we already did part of the processing in hardware.
> The match on the dst_mac will fail as we already modified it, and we won't execute the third rule action.
> In addition, if we did manage to execute any software tc rules, the packet will be counted twice.
> 
> We'll re-use this extension to solve this issue that currently exists by letting drivers tell tc on which chain to start the classification.
> 
> Regarding the config, we suggest changing the default to N and letting users decide to enable it, see attached patch.

Partial offloads are very hard. Could we possibly take a page out of
routing offload's book and do a all or nothing offload in presence of
multiple chains?

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