From: Vlad Buslov <vladbu@nvidia•com>
To: Simon Horman <simon.horman@corigine•com>
Cc: <netdev@vger•kernel.org>, Jamal Hadi Salim <jhs@mojatatu•com>,
Roi Dayan <roid@nvidia•com>, Ido Schimmel <idosch@nvidia•com>,
Cong Wang <xiyou.wangcong@gmail•com>,
Jiri Pirko <jiri@resnulli•us>,
Baowen Zheng <notifications@github•com>,
Louis Peens <louis.peens@corigine•com>,
<oss-drivers@corigine•com>,
Baowen Zheng <baowen.zheng@corigine•com>
Subject: Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions
Date: Fri, 29 Oct 2021 21:01:05 +0300 [thread overview]
Message-ID: <ygnhilxfaexq.fsf@nvidia.com> (raw)
In-Reply-To: <20211028110646.13791-9-simon.horman@corigine.com>
On Thu 28 Oct 2021 at 14:06, Simon Horman <simon.horman@corigine•com> wrote:
> From: Baowen Zheng <baowen.zheng@corigine•com>
>
> Add process to validate flags of filter and actions when adding
> a tc filter.
>
> We need to prevent adding filter with flags conflicts with its actions.
>
> Signed-off-by: Baowen Zheng <baowen.zheng@corigine•com>
> Signed-off-by: Louis Peens <louis.peens@corigine•com>
> Signed-off-by: Simon Horman <simon.horman@corigine•com>
> ---
> net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
> net/sched/cls_flower.c | 3 ++-
> net/sched/cls_matchall.c | 4 ++--
> net/sched/cls_u32.c | 7 ++++---
> 4 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 351d93988b8b..80647da9713a 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3025,6 +3025,29 @@ void tcf_exts_destroy(struct tcf_exts *exts)
> }
> EXPORT_SYMBOL(tcf_exts_destroy);
>
> +static bool tcf_exts_validate_actions(const struct tcf_exts *exts, u32 flags)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> + bool skip_sw = tc_skip_sw(flags);
> + bool skip_hw = tc_skip_hw(flags);
> + int i;
> +
> + if (!(skip_sw | skip_hw))
> + return true;
> +
> + for (i = 0; i < exts->nr_actions; i++) {
> + struct tc_action *a = exts->actions[i];
> +
> + if ((skip_sw && tc_act_skip_hw(a->tcfa_flags)) ||
> + (skip_hw && tc_act_skip_sw(a->tcfa_flags)))
> + return false;
> + }
> + return true;
> +#else
> + return true;
> +#endif
> +}
> +
I know Jamal suggested to have skip_sw for actions, but it complicates
the code and I'm still not entirely understand why it is necessary.
After all, action can only get applied to a packet if the packet has
been matched by some filter and filters already have skip sw/hw
controls. Forgoing action skip_sw flag would:
- Alleviate the need to validate that filter and action flags are
compatible. (trying to offload filter that points to existing skip_hw
action would just fail because the driver wouldn't find the action with
provided id in its tables)
- Remove the need to add more conditionals into TC software data path in
patch 4.
WDYT?
> int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
> struct nlattr *rate_tlv, struct tcf_exts *exts,
> u32 flags, struct netlink_ext_ack *extack)
> @@ -3066,6 +3089,9 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
> return err;
> exts->nr_actions = err;
> }
> +
> + if (!tcf_exts_validate_actions(exts, flags))
> + return -EINVAL;
> }
> #else
> if ((exts->action && tb[exts->action]) ||
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index eb6345a027e1..55f89f0e393e 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -2035,7 +2035,8 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
> }
>
> err = fl_set_parms(net, tp, fnew, mask, base, tb, tca[TCA_RATE],
> - tp->chain->tmplt_priv, flags, extack);
> + tp->chain->tmplt_priv, flags | fnew->flags,
> + extack);
Aren't you or-ing flags from two different ranges (TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_*) that map to same bits, or am I missing something? This
isn't explained in commit message so it is hard for me to understand the
idea here.
> if (err)
> goto errout;
>
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 24f0046ce0b3..00b76fbc1dce 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -226,8 +226,8 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
> goto err_alloc_percpu;
> }
>
> - err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE], flags,
> - extack);
> + err = mall_set_parms(net, tp, new, base, tb, tca[TCA_RATE],
> + flags | new->flags, extack);
> if (err)
> goto err_set_parms;
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 4272814487f0..fc670cc45122 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -895,7 +895,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> return -ENOMEM;
>
> err = u32_set_parms(net, tp, base, new, tb,
> - tca[TCA_RATE], flags, extack);
> + tca[TCA_RATE], flags | new->flags,
> + extack);
>
> if (err) {
> u32_destroy_key(new, false);
> @@ -1060,8 +1061,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> }
> #endif
>
> - err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE], flags,
> - extack);
> + err = u32_set_parms(net, tp, base, n, tb, tca[TCA_RATE],
> + flags | n->flags, extack);
> if (err == 0) {
> struct tc_u_knode __rcu **ins;
> struct tc_u_knode *pins;
next prev parent reply other threads:[~2021-10-29 18:01 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-28 11:06 [RFC/PATCH net-next v3 0/8] allow user to offload tc action to net device Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 1/8] flow_offload: fill flags to action structure Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 2/8] flow_offload: reject to offload tc actions in offload drivers Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 3/8] flow_offload: allow user to offload tc action to net device Simon Horman
2021-10-29 16:59 ` Vlad Buslov
2021-11-01 9:44 ` Baowen Zheng
2021-11-01 12:05 ` Vlad Buslov
2021-11-02 1:38 ` Baowen Zheng
2021-10-31 9:50 ` Oz Shlomo
2021-11-01 2:30 ` Baowen Zheng
2021-11-01 10:07 ` Oz Shlomo
2021-11-01 10:27 ` Baowen Zheng
2021-10-28 11:06 ` [RFC/PATCH net-next v3 4/8] flow_offload: add skip_hw and skip_sw to control if offload the action Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 5/8] flow_offload: add process to update action stats from hardware Simon Horman
2021-10-29 17:11 ` Vlad Buslov
2021-11-01 10:07 ` Baowen Zheng
2021-10-28 11:06 ` [RFC/PATCH net-next v3 6/8] net: sched: save full flags for tc action Simon Horman
2021-10-28 11:06 ` [RFC/PATCH net-next v3 7/8] flow_offload: add reoffload process to update hw_count Simon Horman
2021-10-29 17:31 ` Vlad Buslov
2021-11-02 9:20 ` Baowen Zheng
2021-10-28 11:06 ` [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions Simon Horman
2021-10-29 18:01 ` Vlad Buslov [this message]
2021-10-30 10:54 ` Jamal Hadi Salim
2021-10-30 14:45 ` Vlad Buslov
[not found] ` <DM5PR1301MB21722A85B19EE97EFE27A5BBE7899@DM5PR1301MB2172.namprd13.prod.outlook.com>
2021-10-31 13:30 ` Jamal Hadi Salim
2021-11-01 3:29 ` Baowen Zheng
2021-11-01 7:38 ` Vlad Buslov
2021-11-02 12:39 ` Simon Horman
2021-11-03 7:57 ` Baowen Zheng
2021-11-03 10:13 ` Jamal Hadi Salim
2021-11-03 11:30 ` Baowen Zheng
2021-11-03 12:33 ` Jamal Hadi Salim
2021-11-03 13:33 ` Jamal Hadi Salim
2021-11-03 13:38 ` Simon Horman
2021-11-03 14:05 ` Jamal Hadi Salim
2021-11-03 14:03 ` Baowen Zheng
2021-11-03 14:16 ` Jamal Hadi Salim
2021-11-03 14:48 ` Baowen Zheng
2021-11-03 15:35 ` Jamal Hadi Salim
2021-11-03 13:37 ` Baowen Zheng
2021-11-04 2:30 ` Baowen Zheng
2021-11-04 5:51 ` Baowen Zheng
2021-11-04 9:07 ` Vlad Buslov
2021-11-04 11:15 ` Baowen Zheng
2021-10-28 14:23 ` [RFC/PATCH net-next v3 0/8] allow user to offload tc action to net device Jamal Hadi Salim
2021-10-28 14:39 ` Jamal Hadi Salim
2021-10-31 9:50 ` Oz Shlomo
2021-10-31 12:03 ` Dave Taht
2021-10-31 14:14 ` Jamal Hadi Salim
2021-10-31 14:19 ` Jamal Hadi Salim
2021-11-01 14:27 ` Dave Taht
2021-10-31 13:40 ` Jamal Hadi Salim
2021-11-01 8:01 ` Vlad Buslov
2021-11-02 12:51 ` Simon Horman
2021-11-02 15:33 ` Vlad Buslov
2021-11-02 16:15 ` Simon Horman
2021-11-03 10:56 ` Oz Shlomo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ygnhilxfaexq.fsf@nvidia.com \
--to=vladbu@nvidia$(echo .)com \
--cc=baowen.zheng@corigine$(echo .)com \
--cc=idosch@nvidia$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=louis.peens@corigine$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=notifications@github$(echo .)com \
--cc=oss-drivers@corigine$(echo .)com \
--cc=roid@nvidia$(echo .)com \
--cc=simon.horman@corigine$(echo .)com \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox