| 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
| ||
|
Message-ID: <CAAFAkD8KeMi3z-AArAsp8G8qAYPTv=go0qRvvTguWxAou+fzxw@mail.gmail.com>
Date: Fri, 15 Dec 2023 09:36:37 -0500
From: Jamal Hadi Salim <hadi@...atatu.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: Victor Nogueira <victor@...atatu.com>, jhs@...atatu.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
xiyou.wangcong@...il.com, mleitner@...hat.com, vladbu@...dia.com,
paulb@...dia.com, pctammela@...atatu.com, netdev@...r.kernel.org,
kernel@...atatu.com
Subject: Re: [PATCH net-next v7 3/3] net/sched: act_mirred: Allow mirred to block
On Fri, Dec 15, 2023 at 9:19 AM Jiri Pirko <jiri@...nulli.us> wrote:
>
> Fri, Dec 15, 2023 at 02:56:48PM CET, victor@...atatu.com wrote:
> >On 15/12/2023 10:06, Jiri Pirko wrote:
> >> Fri, Dec 15, 2023 at 12:10:50PM CET, victor@...atatu.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@...atatu.com>
> >> > Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
> >> > Co-developed-by: Pedro Tammela <pctammela@...atatu.com>
> >> > Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> >> > Signed-off-by: Victor Nogueira <victor@...atatu.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
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;
> >>
> >> [...]
Powered by blists - more mailing lists