Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions
From: Vlad Buslov <hidden>
Date: 2021-10-29 18:01:15
On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Baowen Zheng <redacted> 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 <redacted> Signed-off-by: Louis Peens <redacted> Signed-off-by: Simon Horman <redacted> --- 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?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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;