netdev - Re: [PATCH v2 net-next 1/6] net: sched: propagate "skip_sw" flag to offload for flower and matchall

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: <ZxUo0Dc0M5Y6l9qF@shredder.mtl.com>
Date: Sun, 20 Oct 2024 18:59:12 +0300
From: Ido Schimmel <idosch@...dia.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Petr Machata <petrm@...dia.com>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	UNGLinuxDriver@...rochip.com, Jamal Hadi Salim <jhs@...atatu.com>,
	Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	Vlad Buslov <vladbu@...dia.com>, Simon Horman <horms@...nel.org>,
	Christian Marangi <ansuelsmth@...il.com>,
	Arun Ramadoss <arun.ramadoss@...rochip.com>,
	Arınç ÜNAL <arinc.unal@...nc9.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 net-next 1/6] net: sched: propagate "skip_sw" flag to
 offload for flower and matchall

On Thu, Oct 17, 2024 at 07:52:10PM +0300, Vladimir Oltean wrote:
> Background: switchdev ports offload the Linux bridge, and most of the
> packets they handle will never see the CPU. The ports between which
> there exists no hardware data path are considered 'foreign' to switchdev.
> These can either be normal physical NICs without switchdev offload, or
> incompatible switchdev ports, or virtual interfaces like veth/dummy/etc.
> 
> In some cases, an offloaded filter can only do half the work, and the
> rest must be handled by software. Redirecting/mirroring from the ingress
> of a switchdev port towards a foreign interface is one example of
> combined hardware/software data path. The most that the switchdev port
> can do is to extract the matching packets from its offloaded data path
> and send them to the CPU. From there on, the software filter runs
> (a second time, after the first run in hardware) on the packet and
> performs the mirred action.
> 
> It makes sense for switchdev drivers which allow this kind of "half
> offloading" to sense the "skip_sw" flag of the filter/action pair, and
> deny attempts from the user to install a filter that does not run in
> software, because that simply won't work.
> 
> In fact, a mirred action on a switchdev port towards a dummy interface
> appears to be a valid way of (selectively) monitoring offloaded traffic
> that flows through it. IFF_PROMISC was also discussed years ago, but
> (despite initial disagreement) there seems to be consensus that this
> flag should not affect the destination taken by packets, but merely
> whether or not the NIC discards packets with unknown MAC DA for local
> processing.
> 
> Only the flower and matchall classifiers are of interest to me for
> purely pragmatic reasons: these are offloaded by DSA currently.

Possibly a stupid question given I don't remember all the details of the
TC offload, but is there a reason not to put the 'skip_sw' indication in
'struct flow_cls_common_offload' and initialize the new field as part of
tc_cls_common_offload_init()?

Seems like it won't require patching every classifier and will also work
for the re-offload case (e.g., fl_reoffload())?

Something like:

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 292cd8f4b762..596ab9791e4d 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -685,6 +685,7 @@ struct flow_cls_common_offload {
        u32 chain_index;
        __be16 protocol;
        u32 prio;
+       bool skip_sw;
        struct netlink_ext_ack *extack;
 };
 
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 4880b3a7aced..cf199af85c52 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -755,6 +755,7 @@ tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common,
        cls_common->chain_index = tp->chain->index;
        cls_common->protocol = tp->protocol;
        cls_common->prio = tp->prio >> 16;
+       cls_common->skip_sw = tc_skip_sw(flags);
        if (tc_skip_sw(flags) || flags & TCA_CLS_FLAGS_VERBOSE)
                cls_common->extack = extack;
 }

> 
> [1] https://lore.kernel.org/netdev/20190830092637.7f83d162@ceranb/
> [2] https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> v1->v2: rewrite commit message
> 
>  include/net/flow_offload.h | 1 +
>  include/net/pkt_cls.h      | 1 +
>  net/sched/cls_flower.c     | 1 +
>  net/sched/cls_matchall.c   | 1 +
>  4 files changed, 4 insertions(+)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index 292cd8f4b762..a2f688dd0447 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -692,6 +692,7 @@ struct flow_cls_offload {
>  	struct flow_cls_common_offload common;
>  	enum flow_cls_command command;
>  	bool use_act_stats;
> +	bool skip_sw;
>  	unsigned long cookie;
>  	struct flow_rule *rule;
>  	struct flow_stats stats;
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 4880b3a7aced..7b9f41f33c33 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -782,6 +782,7 @@ struct tc_cls_matchall_offload {
>  	struct flow_rule *rule;
>  	struct flow_stats stats;
>  	bool use_act_stats;
> +	bool skip_sw;
>  	unsigned long cookie;
>  };
>  
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index e280c27cb9f9..8f7c60805f85 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -480,6 +480,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
>  	cls_flower.rule->match.mask = &f->mask->key;
>  	cls_flower.rule->match.key = &f->mkey;
>  	cls_flower.classid = f->res.classid;
> +	cls_flower.skip_sw = skip_sw;
>  
>  	err = tc_setup_offload_action(&cls_flower.rule->action, &f->exts,
>  				      cls_flower.common.extack);
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 9f1e62ca508d..9bd598f8a46c 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -98,6 +98,7 @@ static int mall_replace_hw_filter(struct tcf_proto *tp,
>  	tc_cls_common_offload_init(&cls_mall.common, tp, head->flags, extack);
>  	cls_mall.command = TC_CLSMATCHALL_REPLACE;
>  	cls_mall.cookie = cookie;
> +	cls_mall.skip_sw = skip_sw;
>  
>  	err = tc_setup_offload_action(&cls_mall.rule->action, &head->exts,
>  				      cls_mall.common.extack);
> -- 
> 2.43.0
>