On Fri, Dec 15, 2023 at 9:19 AM Jiri Pirko [off-list ref] wrote:

Fri, Dec 15, 2023 at 02:56:48PM CET, victor@mojatatu.com wrote:

quoted

On 15/12/2023 10:06, Jiri Pirko wrote:

quoted

Fri, Dec 15, 2023 at 12:10:50PM CET, victor@mojatatu.com wrote:

quoted

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 <redacted>
Signed-off-by: Pedro Tammela <redacted>
Signed-off-by: Victor Nogueira <redacted>
---
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.

My bad, don't know how we missed this one.
Will fix in v8.

quoted

quoted

   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?

Will transform this one into two separate patches.

More please.

I see the first one as preparation and the second as usage. Can you
make suggestion as to what more/better split is?

cheers,
jamal

quoted

quoted

quoted

           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;

[...]