RE: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions

Sorry for reply this message again.
On November 4, 2021 10:31 AM, Baowen Zheng wrote:

Thanks for your review and sorry for delay in responding.
On October 30, 2021 2:01 AM, Vlad Buslov wrote:

quoted

On Thu 28 Oct 2021 at 14:06, Simon Horman [off-list ref]
wrote:

quoted

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?

As we discussed with Jamal, we will keep the flag of skip_sw and we need to
make exactly match for the actions with flags and the filter specific action with
index.

quoted

quoted

 int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr

**tb,

quoted

quoted

 		      struct nlattr *rate_tlv, struct tcf_exts *exts,
 		      u32 flags, struct netlink_ext_ack *extack) @@ -3066,6

+3089,9

quoted

@@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp,

struct nlattr

**tb,

quoted

 				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,

quoted

 	}

 	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.

Yes, as you said we use TCA_CLS_FLAGS_* or TCA_ACT_FLAGS_* flags to
validate the action flags.
As you know, the TCA_ACT_FLAGS_* in flags are system flags(in high 16 bits)
and the TCA_CLS_FLAGS_* are user flags(in low 16 bits), so they will not be
conflict.
But I think you suggestion also makes sense to us, do you think we need to
pass a single filter flag to make the process more clear?

After consideration, I think it is better to separate CLS flags and ACT flags. 
So we will pass CLS flags as a separate flags, thanks.

quoted

quoted

 	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,

quoted

 		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,

quoted

 			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,

quoted

 	}
 #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;