Re: [PATCH net-next v7 3/3] net/sched: act_mirred: Allow mirred to block

[Jiri Pirko <jiri@resnulli.us>]

Fri, Dec 15, 2023 at 12:10:50PM CET, victor@mojatatu.com wrote:
>So far the mirred action has dealt with syntax that handles mirror/redirection for netdev.
>A matching packet is redirected or mirrored to a target netdev.
>
>In this patch we enable mirred to mirror to a tc block as well.
>IOW, the new syntax looks as follows:
>... mirred <ingress | egress> <mirror | redirect> [index INDEX] < <blockid BLOCKID> | <dev <devname>> >
>
>Examples of mirroring or redirecting to a tc block:
>$ tc filter add block 22 protocol ip pref 25 \
>  flower dst_ip 192.168.0.0/16 action mirred egress mirror blockid 22
>
>$ tc filter add block 22 protocol ip pref 25 \
>  flower dst_ip 10.10.10.10/32 action mirred egress redirect blockid 22
>
>Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
>Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
>Signed-off-by: Victor Nogueira <victor@mojatatu.com>
>---
> include/net/tc_act/tc_mirred.h        |   1 +
> include/uapi/linux/tc_act/tc_mirred.h |   1 +
> net/sched/act_mirred.c                | 278 +++++++++++++++++++-------
> 3 files changed, 206 insertions(+), 74 deletions(-)
>
>diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
>index 32ce8ea36950..75722d967bf2 100644
>--- a/include/net/tc_act/tc_mirred.h
>+++ b/include/net/tc_act/tc_mirred.h
>@@ -8,6 +8,7 @@
> struct tcf_mirred {
> 	struct tc_action	common;
> 	int			tcfm_eaction;
>+	u32                     tcfm_blockid;
> 	bool			tcfm_mac_header_xmit;
> 	struct net_device __rcu	*tcfm_dev;
> 	netdevice_tracker	tcfm_dev_tracker;
>diff --git a/include/uapi/linux/tc_act/tc_mirred.h b/include/uapi/linux/tc_act/tc_mirred.h
>index 2500a0005d05..54df06658bc8 100644
>--- a/include/uapi/linux/tc_act/tc_mirred.h
>+++ b/include/uapi/linux/tc_act/tc_mirred.h
>@@ -20,6 +20,7 @@ enum {
> 	TCA_MIRRED_UNSPEC,
> 	TCA_MIRRED_TM,
> 	TCA_MIRRED_PARMS,
>+	TCA_MIRRED_BLOCKID,

You just broke uapi. Make sure to add new attributes to the end.


> 	TCA_MIRRED_PAD,
> 	__TCA_MIRRED_MAX
> };
>diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
>index 0a711c184c29..8b6d04d26c5a 100644
>--- a/net/sched/act_mirred.c
>+++ b/net/sched/act_mirred.c
>@@ -85,10 +85,20 @@ static void tcf_mirred_release(struct tc_action *a)
> 
> static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
> 	[TCA_MIRRED_PARMS]	= { .len = sizeof(struct tc_mirred) },
>+	[TCA_MIRRED_BLOCKID]	= { .type = NLA_U32 },
> };
> 
> static struct tc_action_ops act_mirred_ops;
> 
>+static void tcf_mirred_replace_dev(struct tcf_mirred *m, struct net_device *ndev)
>+{
>+	struct net_device *odev;
>+
>+	odev = rcu_replace_pointer(m->tcfm_dev, ndev,
>+				   lockdep_is_held(&m->tcf_lock));
>+	netdev_put(odev, &m->tcfm_dev_tracker);
>+}
>+
> static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 			   struct nlattr *est, struct tc_action **a,
> 			   struct tcf_proto *tp,
>@@ -126,6 +136,13 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 	if (exists && bind)
> 		return 0;
> 
>+	if (tb[TCA_MIRRED_BLOCKID] && parm->ifindex) {
>+		NL_SET_ERR_MSG_MOD(extack,
>+				   "Mustn't specify Block ID and dev simultaneously");
>+		err = -EINVAL;
>+		goto release_idr;
>+	}
>+
> 	switch (parm->eaction) {
> 	case TCA_EGRESS_MIRROR:
> 	case TCA_EGRESS_REDIR:
>@@ -142,9 +159,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 	}
> 
> 	if (!exists) {
>-		if (!parm->ifindex) {
>+		if (!parm->ifindex && !tb[TCA_MIRRED_BLOCKID]) {
> 			tcf_idr_cleanup(tn, index);
>-			NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
>+			NL_SET_ERR_MSG_MOD(extack, "Must specify device or block");
> 			return -EINVAL;
> 		}
> 		ret = tcf_idr_create_from_flags(tn, index, est, a,
>@@ -170,7 +187,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 	spin_lock_bh(&m->tcf_lock);
> 
> 	if (parm->ifindex) {
>-		struct net_device *odev, *ndev;
>+		struct net_device *ndev;
> 
> 		ndev = dev_get_by_index(net, parm->ifindex);
> 		if (!ndev) {
>@@ -179,11 +196,14 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
> 			goto put_chain;
> 		}
> 		mac_header_xmit = dev_is_mac_header_xmit(ndev);
>-		odev = rcu_replace_pointer(m->tcfm_dev, ndev,
>-					  lockdep_is_held(&m->tcf_lock));
>-		netdev_put(odev, &m->tcfm_dev_tracker);
>+		tcf_mirred_replace_dev(m, ndev);

This could be a separate patch, for better readability of the patches.

Skimming thought the rest of the patch, this is hard to follow (-ETOOBIG).
What would help is to cut this patch into multiple ones. Do preparations
first, then you finally add TCA_MIRRED_BLOCKID processin and blockid
forwarding. Could you?


> 		netdev_tracker_alloc(ndev, &m->tcfm_dev_tracker, GFP_ATOMIC);
> 		m->tcfm_mac_header_xmit = mac_header_xmit;
>+		m->tcfm_blockid = 0;
>+	} else if (tb[TCA_MIRRED_BLOCKID]) {
>+		tcf_mirred_replace_dev(m, NULL);
>+		m->tcfm_mac_header_xmit = false;
>+		m->tcfm_blockid = nla_get_u32(tb[TCA_MIRRED_BLOCKID]);
> 	}
> 	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> 	m->tcfm_eaction = parm->eaction;

[...]