netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 net-next 0/6] Mirroring to DSA CPU port
@ 2024-10-23 13:52 Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload Vladimir Oltean
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

Users of the NXP LS1028A SoC (drivers/net/dsa/ocelot L2 switch inside)
have requested to mirror packets from the ingress of a switch port to
software. Both port-based and flow-based mirroring is required.

The simplest way I could come up with was to set up tc mirred actions
towards a dummy net_device, and make the offloading of that be accepted
by the driver. Currently, the pattern in drivers is to reject mirred
towards ports they don't know about, but I'm now permitting that,
precisely by mirroring "to the CPU".

For testers, this series depends on commit 34d35b4edbbe ("net/sched:
act_api: deny mismatched skip_sw/skip_hw flags for actions created by
classifiers") from net/main, which is absent from net-next as of the
day of posting (Oct 23). Without the bug fix it is possible to create
invalid configurations which are not rejected by the kernel.

Changes from v2:
- Move skip_sw from struct flow_cls_offload and struct
  tc_cls_matchall_offload to struct flow_cls_common_offload.

Changes from RFC:
- Sent the bug fix separately, now merged as commit 8c924369cb56 ("net:
  dsa: refuse cross-chip mirroring operations") in the "net" tree
- Allow mirroring to the ingress of another switch port (using software)
  both for matchall in DSA and flower offload in ocelot
- Patch 3/6 is new

Link to v2:
https://lore.kernel.org/netdev/20241017165215.3709000-1-vladimir.oltean@nxp.com/

Link to previous RFC:
https://lore.kernel.org/netdev/20240913152915.2981126-1-vladimir.oltean@nxp.com/

For historical purposes, link to a much older (and much different) attempt:
https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/

Vladimir Oltean (6):
  net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload
  net: dsa: clean up dsa_user_add_cls_matchall()
  net: dsa: use "extack" as argument to
    flow_action_basic_hw_stats_check()
  net: dsa: add more extack messages in
    dsa_user_add_cls_matchall_mirred()
  net: dsa: allow matchall mirroring rules towards the CPU
  net: mscc: ocelot: allow tc-flower mirred action towards foreign
    interfaces

 drivers/net/ethernet/mscc/ocelot_flower.c | 54 ++++++++++++----
 include/net/flow_offload.h                |  1 +
 include/net/pkt_cls.h                     |  1 +
 net/dsa/user.c                            | 78 +++++++++++++++++------
 4 files changed, 103 insertions(+), 31 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
@ 2024-10-23 13:52 ` Vladimir Oltean
  2024-10-27  7:29   ` Ido Schimmel
  2024-10-31  0:47   ` Jakub Kicinski
  2024-10-23 13:52 ` [PATCH v3 net-next 2/6] net: dsa: clean up dsa_user_add_cls_matchall() Vladimir Oltean
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

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.

[1] https://lore.kernel.org/netdev/20190830092637.7f83d162@ceranb/
[2] https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/
Suggested-by: Ido Schimmel <idosch@nvidia.com>
Link: https://lore.kernel.org/netdev/ZxUo0Dc0M5Y6l9qF@shredder.mtl.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: move skip_sw from struct flow_cls_offload and struct
        tc_cls_matchall_offload to struct flow_cls_common_offload.
v1->v2: rewrite commit message

 include/net/flow_offload.h | 1 +
 include/net/pkt_cls.h      | 1 +
 2 files changed, 2 insertions(+)

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;
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 net-next 2/6] net: dsa: clean up dsa_user_add_cls_matchall()
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload Vladimir Oltean
@ 2024-10-23 13:52 ` Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 3/6] net: dsa: use "extack" as argument to flow_action_basic_hw_stats_check() Vladimir Oltean
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

The body is a bit hard to read, hard to extend, and has duplicated
conditions.

Clean up the "if (many conditions) else if (many conditions, some
of them repeated)" pattern by:

- Moving the repeated conditions out
- Replacing the repeated tests for the same variable with a switch/case
- Moving the protocol check inside the dsa_user_add_cls_matchall_mirred()
  function call.

This is pure refactoring, no logic has been changed, though some tests
were reordered. The order does not matter - they are independent things
to be tested for.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: none
v1->v2: add some more info in commit message

 net/dsa/user.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index 64f660d2334b..5d78a881ddc2 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1377,6 +1377,9 @@ dsa_user_add_cls_matchall_mirred(struct net_device *dev,
 	struct dsa_port *to_dp;
 	int err;
 
+	if (cls->common.protocol != htons(ETH_P_ALL))
+		return -EOPNOTSUPP;
+
 	if (!ds->ops->port_mirror_add)
 		return -EOPNOTSUPP;
 
@@ -1486,17 +1489,21 @@ static int dsa_user_add_cls_matchall(struct net_device *dev,
 				     struct tc_cls_matchall_offload *cls,
 				     bool ingress)
 {
-	int err = -EOPNOTSUPP;
+	const struct flow_action *action = &cls->rule->action;
 
-	if (cls->common.protocol == htons(ETH_P_ALL) &&
-	    flow_offload_has_one_action(&cls->rule->action) &&
-	    cls->rule->action.entries[0].id == FLOW_ACTION_MIRRED)
-		err = dsa_user_add_cls_matchall_mirred(dev, cls, ingress);
-	else if (flow_offload_has_one_action(&cls->rule->action) &&
-		 cls->rule->action.entries[0].id == FLOW_ACTION_POLICE)
-		err = dsa_user_add_cls_matchall_police(dev, cls, ingress);
+	if (!flow_offload_has_one_action(action))
+		return -EOPNOTSUPP;
 
-	return err;
+	switch (action->entries[0].id) {
+	case FLOW_ACTION_MIRRED:
+		return dsa_user_add_cls_matchall_mirred(dev, cls, ingress);
+	case FLOW_ACTION_POLICE:
+		return dsa_user_add_cls_matchall_police(dev, cls, ingress);
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
 }
 
 static void dsa_user_del_cls_matchall(struct net_device *dev,
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 net-next 3/6] net: dsa: use "extack" as argument to flow_action_basic_hw_stats_check()
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 2/6] net: dsa: clean up dsa_user_add_cls_matchall() Vladimir Oltean
@ 2024-10-23 13:52 ` Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 4/6] net: dsa: add more extack messages in dsa_user_add_cls_matchall_mirred() Vladimir Oltean
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

We already have an "extack" stack variable in
dsa_user_add_cls_matchall_police() and
dsa_user_add_cls_matchall_mirred(), there is no need to retrieve
it again from cls->common.extack.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: none
v1->v2: patch is new

 net/dsa/user.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index 5d78a881ddc2..c398a4479b36 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1383,8 +1383,7 @@ dsa_user_add_cls_matchall_mirred(struct net_device *dev,
 	if (!ds->ops->port_mirror_add)
 		return -EOPNOTSUPP;
 
-	if (!flow_action_basic_hw_stats_check(&cls->rule->action,
-					      cls->common.extack))
+	if (!flow_action_basic_hw_stats_check(&cls->rule->action, extack))
 		return -EOPNOTSUPP;
 
 	act = &cls->rule->action.entries[0];
@@ -1450,8 +1449,7 @@ dsa_user_add_cls_matchall_police(struct net_device *dev,
 		return -EOPNOTSUPP;
 	}
 
-	if (!flow_action_basic_hw_stats_check(&cls->rule->action,
-					      cls->common.extack))
+	if (!flow_action_basic_hw_stats_check(&cls->rule->action, extack))
 		return -EOPNOTSUPP;
 
 	list_for_each_entry(mall_tc_entry, &p->mall_tc_list, list) {
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 net-next 4/6] net: dsa: add more extack messages in dsa_user_add_cls_matchall_mirred()
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
                   ` (2 preceding siblings ...)
  2024-10-23 13:52 ` [PATCH v3 net-next 3/6] net: dsa: use "extack" as argument to flow_action_basic_hw_stats_check() Vladimir Oltean
@ 2024-10-23 13:52 ` Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 5/6] net: dsa: allow matchall mirroring rules towards the CPU Vladimir Oltean
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

Do not leave -EOPNOTSUPP errors without an explanation. It is confusing
for the user to figure out what is wrong otherwise.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v3: none

 net/dsa/user.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index c398a4479b36..2fead3a4fa84 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1377,11 +1377,17 @@ dsa_user_add_cls_matchall_mirred(struct net_device *dev,
 	struct dsa_port *to_dp;
 	int err;
 
-	if (cls->common.protocol != htons(ETH_P_ALL))
+	if (cls->common.protocol != htons(ETH_P_ALL)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Can only offload \"protocol all\" matchall filter");
 		return -EOPNOTSUPP;
+	}
 
-	if (!ds->ops->port_mirror_add)
+	if (!ds->ops->port_mirror_add) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Switch does not support mirroring operation");
 		return -EOPNOTSUPP;
+	}
 
 	if (!flow_action_basic_hw_stats_check(&cls->rule->action, extack))
 		return -EOPNOTSUPP;
@@ -1488,9 +1494,13 @@ static int dsa_user_add_cls_matchall(struct net_device *dev,
 				     bool ingress)
 {
 	const struct flow_action *action = &cls->rule->action;
+	struct netlink_ext_ack *extack = cls->common.extack;
 
-	if (!flow_offload_has_one_action(action))
+	if (!flow_offload_has_one_action(action)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Cannot offload matchall filter with more than one action");
 		return -EOPNOTSUPP;
+	}
 
 	switch (action->entries[0].id) {
 	case FLOW_ACTION_MIRRED:
@@ -1498,6 +1508,7 @@ static int dsa_user_add_cls_matchall(struct net_device *dev,
 	case FLOW_ACTION_POLICE:
 		return dsa_user_add_cls_matchall_police(dev, cls, ingress);
 	default:
+		NL_SET_ERR_MSG_MOD(extack, "Unknown action");
 		break;
 	}
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 net-next 5/6] net: dsa: allow matchall mirroring rules towards the CPU
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
                   ` (3 preceding siblings ...)
  2024-10-23 13:52 ` [PATCH v3 net-next 4/6] net: dsa: add more extack messages in dsa_user_add_cls_matchall_mirred() Vladimir Oltean
@ 2024-10-23 13:52 ` Vladimir Oltean
  2024-10-23 13:52 ` [PATCH v3 net-next 6/6] net: mscc: ocelot: allow tc-flower mirred action towards foreign interfaces Vladimir Oltean
  2024-10-31  0:50 ` [PATCH v3 net-next 0/6] Mirroring to DSA CPU port patchwork-bot+netdevbpf
  6 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

If the CPU bandwidth capacity permits, it may be useful to mirror the
entire ingress of a user port to software.

This is in fact possible to express even if there is no net_device
representation for the CPU port. In fact, that approach was already
exhausted and that representation wouldn't have even helped [1].

The idea behind implementing this is that currently, we refuse to
offload any mirroring towards a non-DSA target net_device. But if we
acknowledge the fact that to reach any foreign net_device, the switch
must send the packet to the CPU anyway, then we can simply offload just
that part, and let the software do the rest. There is only one condition
we need to uphold: the filter needs to be present in the software data
path as well (no skip_sw).

There are 2 actions to consider: FLOW_ACTION_MIRRED (redirect to egress
of target interface) and FLOW_ACTION_MIRRED_INGRESS (redirect to ingress
of target interface). We don't have the ability/API to offload
FLOW_ACTION_MIRRED_INGRESS when the target port is also a DSA user port,
but we could also permit that through mirred to the CPU + software.

Example:

$ ip link add dummy0 type dummy; ip link set dummy0 up
$ tc qdisc add dev swp0 clsact
$ tc filter add dev swp0 ingress matchall action mirred ingress mirror dev dummy0

Any DSA driver with a ds->ops->port_mirror_add() implementation can now
make use of this with no additional change.

[1] https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: s/cls->skip_sw/cls->common.skip_sw/
v1->v2: allow mirroring to the ingress of another DSA user port
        (using software)

 net/dsa/user.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/net/dsa/user.c b/net/dsa/user.c
index 2fead3a4fa84..6b718960f40d 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1365,7 +1365,7 @@ dsa_user_mall_tc_entry_find(struct net_device *dev, unsigned long cookie)
 static int
 dsa_user_add_cls_matchall_mirred(struct net_device *dev,
 				 struct tc_cls_matchall_offload *cls,
-				 bool ingress)
+				 bool ingress, bool ingress_target)
 {
 	struct netlink_ext_ack *extack = cls->common.extack;
 	struct dsa_port *dp = dsa_user_to_port(dev);
@@ -1397,10 +1397,30 @@ dsa_user_add_cls_matchall_mirred(struct net_device *dev,
 	if (!act->dev)
 		return -EINVAL;
 
-	if (!dsa_user_dev_check(act->dev))
-		return -EOPNOTSUPP;
-
-	to_dp = dsa_user_to_port(act->dev);
+	if (dsa_user_dev_check(act->dev)) {
+		if (ingress_target) {
+			/* We can only fulfill this using software assist */
+			if (cls->common.skip_sw) {
+				NL_SET_ERR_MSG_MOD(extack,
+						   "Can only mirred to ingress of DSA user port if filter also runs in software");
+				return -EOPNOTSUPP;
+			}
+			to_dp = dp->cpu_dp;
+		} else {
+			to_dp = dsa_user_to_port(act->dev);
+		}
+	} else {
+		/* Handle mirroring to foreign target ports as a mirror towards
+		 * the CPU. The software tc rule will take the packets from
+		 * there.
+		 */
+		if (cls->common.skip_sw) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Can only mirred to CPU if filter also runs in software");
+			return -EOPNOTSUPP;
+		}
+		to_dp = dp->cpu_dp;
+	}
 
 	if (dp->ds != to_dp->ds) {
 		NL_SET_ERR_MSG_MOD(extack,
@@ -1504,7 +1524,11 @@ static int dsa_user_add_cls_matchall(struct net_device *dev,
 
 	switch (action->entries[0].id) {
 	case FLOW_ACTION_MIRRED:
-		return dsa_user_add_cls_matchall_mirred(dev, cls, ingress);
+		return dsa_user_add_cls_matchall_mirred(dev, cls, ingress,
+							false);
+	case FLOW_ACTION_MIRRED_INGRESS:
+		return dsa_user_add_cls_matchall_mirred(dev, cls, ingress,
+							true);
 	case FLOW_ACTION_POLICE:
 		return dsa_user_add_cls_matchall_police(dev, cls, ingress);
 	default:
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 net-next 6/6] net: mscc: ocelot: allow tc-flower mirred action towards foreign interfaces
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
                   ` (4 preceding siblings ...)
  2024-10-23 13:52 ` [PATCH v3 net-next 5/6] net: dsa: allow matchall mirroring rules towards the CPU Vladimir Oltean
@ 2024-10-23 13:52 ` Vladimir Oltean
  2024-10-31  0:50 ` [PATCH v3 net-next 0/6] Mirroring to DSA CPU port patchwork-bot+netdevbpf
  6 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2024-10-23 13:52 UTC (permalink / raw)
  To: netdev
  Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Andrew Lunn, Florian Fainelli, Petr Machata, Ido Schimmel,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

Debugging certain flows in the offloaded switch data path can be done by
installing two tc-mirred filters for mirroring: one in the hardware data
path, which copies the frames to the CPU, and one which takes the frame
from there and mirrors it to a virtual interface like a dummy device,
where it can be seen with tcpdump.

The effect of having 2 filters run on the same packet can be obtained by
default using tc, by not specifying either the 'skip_sw' or 'skip_hw'
keywords.

Instead of refusing to offload mirroring/redirecting packets towards
interfaces that aren't switch ports, just treat every other destination
for what it is: something that is handled in software, behind the CPU
port.

Usage:

$ ip link add dummy0 type dummy; ip link set dummy0 up
$ tc qdisc add dev swp0 clsact
$ tc filter add dev swp0 ingress protocol ip flower action mirred ingress mirror dev dummy0

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: s/f->skip_sw/f->common.skip_sw/
v1->v2: allow mirroring to the ingress of another ocelot port
        (using software)

 drivers/net/ethernet/mscc/ocelot_flower.c | 54 ++++++++++++++++++-----
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index a057ec3dab97..986b1f150e3b 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -228,6 +228,32 @@ ocelot_flower_parse_egress_vlan_modify(struct ocelot_vcap_filter *filter,
 	return 0;
 }
 
+static int
+ocelot_flower_parse_egress_port(struct ocelot *ocelot, struct flow_cls_offload *f,
+				const struct flow_action_entry *a, bool mirror,
+				struct netlink_ext_ack *extack)
+{
+	const char *act_string = mirror ? "mirror" : "redirect";
+	int egress_port = ocelot->ops->netdev_to_port(a->dev);
+	enum flow_action_id offloadable_act_id;
+
+	offloadable_act_id = mirror ? FLOW_ACTION_MIRRED : FLOW_ACTION_REDIRECT;
+
+	/* Mirroring towards foreign interfaces is handled in software */
+	if (egress_port < 0 || a->id != offloadable_act_id) {
+		if (f->common.skip_sw) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "Can only %s to %s if filter also runs in software",
+					   act_string, egress_port < 0 ?
+					   "CPU" : "ingress of ocelot port");
+			return -EOPNOTSUPP;
+		}
+		egress_port = ocelot->num_phys_ports;
+	}
+
+	return egress_port;
+}
+
 static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 				      bool ingress, struct flow_cls_offload *f,
 				      struct ocelot_vcap_filter *filter)
@@ -356,6 +382,7 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 			filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
 			break;
 		case FLOW_ACTION_REDIRECT:
+		case FLOW_ACTION_REDIRECT_INGRESS:
 			if (filter->block_id != VCAP_IS2) {
 				NL_SET_ERR_MSG_MOD(extack,
 						   "Redirect action can only be offloaded to VCAP IS2");
@@ -366,17 +393,19 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 						   "Last action must be GOTO");
 				return -EOPNOTSUPP;
 			}
-			egress_port = ocelot->ops->netdev_to_port(a->dev);
-			if (egress_port < 0) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Destination not an ocelot port");
-				return -EOPNOTSUPP;
-			}
+
+			egress_port = ocelot_flower_parse_egress_port(ocelot, f,
+								      a, false,
+								      extack);
+			if (egress_port < 0)
+				return egress_port;
+
 			filter->action.mask_mode = OCELOT_MASK_MODE_REDIRECT;
 			filter->action.port_mask = BIT(egress_port);
 			filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
 			break;
 		case FLOW_ACTION_MIRRED:
+		case FLOW_ACTION_MIRRED_INGRESS:
 			if (filter->block_id != VCAP_IS2) {
 				NL_SET_ERR_MSG_MOD(extack,
 						   "Mirror action can only be offloaded to VCAP IS2");
@@ -387,12 +416,13 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
 						   "Last action must be GOTO");
 				return -EOPNOTSUPP;
 			}
-			egress_port = ocelot->ops->netdev_to_port(a->dev);
-			if (egress_port < 0) {
-				NL_SET_ERR_MSG_MOD(extack,
-						   "Destination not an ocelot port");
-				return -EOPNOTSUPP;
-			}
+
+			egress_port = ocelot_flower_parse_egress_port(ocelot, f,
+								      a, true,
+								      extack);
+			if (egress_port < 0)
+				return egress_port;
+
 			filter->egress_port.value = egress_port;
 			filter->action.mirror_ena = true;
 			filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload
  2024-10-23 13:52 ` [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload Vladimir Oltean
@ 2024-10-27  7:29   ` Ido Schimmel
  2024-10-31  0:47   ` Jakub Kicinski
  1 sibling, 0 replies; 10+ messages in thread
From: Ido Schimmel @ 2024-10-27  7:29 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Andrew Lunn, Florian Fainelli, Petr Machata,
	Claudiu Manoil, Alexandre Belloni, UNGLinuxDriver,
	Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov,
	Simon Horman, Christian Marangi, Arun Ramadoss,
	Arınç ÜNAL, linux-kernel

On Wed, Oct 23, 2024 at 04:52:46PM +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.
> 
> [1] https://lore.kernel.org/netdev/20190830092637.7f83d162@ceranb/
> [2] https://lore.kernel.org/netdev/20191002233750.13566-1-olteanv@gmail.com/
> Suggested-by: Ido Schimmel <idosch@nvidia.com>
> Link: https://lore.kernel.org/netdev/ZxUo0Dc0M5Y6l9qF@shredder.mtl.com/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload
  2024-10-23 13:52 ` [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload Vladimir Oltean
  2024-10-27  7:29   ` Ido Schimmel
@ 2024-10-31  0:47   ` Jakub Kicinski
  1 sibling, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2024-10-31  0:47 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, David S. Miller, Eric Dumazet, Paolo Abeni, Andrew Lunn,
	Florian Fainelli, Petr Machata, Ido Schimmel, Claudiu Manoil,
	Alexandre Belloni, UNGLinuxDriver, Jamal Hadi Salim, Cong Wang,
	Jiri Pirko, Vlad Buslov, Simon Horman, Christian Marangi,
	Arun Ramadoss, Arınç ÜNAL, linux-kernel

On Wed, 23 Oct 2024 16:52:46 +0300 Vladimir Oltean wrote:
> 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.

FTR devices implementing OVS offload can attach metadata to such
packets, and then the driver does the redirect. The SW TC path is 
not engaged. IIRC OVS has a concept of internal ports which are
"stack facing" ports in addition to the main bridge device.
And in some offloaded configurations people want HW to redirect 
to those.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 net-next 0/6] Mirroring to DSA CPU port
  2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
                   ` (5 preceding siblings ...)
  2024-10-23 13:52 ` [PATCH v3 net-next 6/6] net: mscc: ocelot: allow tc-flower mirred action towards foreign interfaces Vladimir Oltean
@ 2024-10-31  0:50 ` patchwork-bot+netdevbpf
  6 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-10-31  0:50 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: netdev, davem, edumazet, kuba, pabeni, andrew, f.fainelli, petrm,
	idosch, claudiu.manoil, alexandre.belloni, UNGLinuxDriver, jhs,
	xiyou.wangcong, jiri, vladbu, horms, ansuelsmth, arun.ramadoss,
	arinc.unal, linux-kernel

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 23 Oct 2024 16:52:45 +0300 you wrote:
> Users of the NXP LS1028A SoC (drivers/net/dsa/ocelot L2 switch inside)
> have requested to mirror packets from the ingress of a switch port to
> software. Both port-based and flow-based mirroring is required.
> 
> The simplest way I could come up with was to set up tc mirred actions
> towards a dummy net_device, and make the offloading of that be accepted
> by the driver. Currently, the pattern in drivers is to reject mirred
> towards ports they don't know about, but I'm now permitting that,
> precisely by mirroring "to the CPU".
> 
> [...]

Here is the summary with links:
  - [v3,net-next,1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload
    https://git.kernel.org/netdev/net-next/c/2748697225c3
  - [v3,net-next,2/6] net: dsa: clean up dsa_user_add_cls_matchall()
    https://git.kernel.org/netdev/net-next/c/a0af7162ccb5
  - [v3,net-next,3/6] net: dsa: use "extack" as argument to flow_action_basic_hw_stats_check()
    https://git.kernel.org/netdev/net-next/c/c11ace14d9db
  - [v3,net-next,4/6] net: dsa: add more extack messages in dsa_user_add_cls_matchall_mirred()
    https://git.kernel.org/netdev/net-next/c/4cc4394a897e
  - [v3,net-next,5/6] net: dsa: allow matchall mirroring rules towards the CPU
    https://git.kernel.org/netdev/net-next/c/3535d70df9c8
  - [v3,net-next,6/6] net: mscc: ocelot: allow tc-flower mirred action towards foreign interfaces
    https://git.kernel.org/netdev/net-next/c/49a09073cb23

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-10-31  0:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-23 13:52 [PATCH v3 net-next 0/6] Mirroring to DSA CPU port Vladimir Oltean
2024-10-23 13:52 ` [PATCH v3 net-next 1/6] net: sched: propagate "skip_sw" flag to struct flow_cls_common_offload Vladimir Oltean
2024-10-27  7:29   ` Ido Schimmel
2024-10-31  0:47   ` Jakub Kicinski
2024-10-23 13:52 ` [PATCH v3 net-next 2/6] net: dsa: clean up dsa_user_add_cls_matchall() Vladimir Oltean
2024-10-23 13:52 ` [PATCH v3 net-next 3/6] net: dsa: use "extack" as argument to flow_action_basic_hw_stats_check() Vladimir Oltean
2024-10-23 13:52 ` [PATCH v3 net-next 4/6] net: dsa: add more extack messages in dsa_user_add_cls_matchall_mirred() Vladimir Oltean
2024-10-23 13:52 ` [PATCH v3 net-next 5/6] net: dsa: allow matchall mirroring rules towards the CPU Vladimir Oltean
2024-10-23 13:52 ` [PATCH v3 net-next 6/6] net: mscc: ocelot: allow tc-flower mirred action towards foreign interfaces Vladimir Oltean
2024-10-31  0:50 ` [PATCH v3 net-next 0/6] Mirroring to DSA CPU port patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).