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 03:36:37PM CET, hadi@mojatatu.com wrote:
>On Fri, Dec 15, 2023 at 9:19 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Fri, Dec 15, 2023 at 02:56:48PM CET, victor@mojatatu.com wrote:
>> >On 15/12/2023 10:06, Jiri Pirko wrote:
>> >> 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.
>> >
>> >My bad, don't know how we missed this one.
>> >Will fix in v8.
>> >
>> >>
>> >> >    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

Preparation should be done in separate patches, one logical change per
patch if possible. Much easier to read and follow.


>make suggestion as to what more/better split is?
>
>cheers,
>jamal
>
>> >
>> >>
>> >> >            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;
>> >>
>> >> [...]