diff mbox series

[RFC/PATCH,net-next,v2,5/5] flow_offload: validate flags of filter and actions

Message ID 20211001113237.14449-6-simon.horman@corigine.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series allow user to offload tc action to net device | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 3 maintainers not CCed: davem@davemloft.net kuba@kernel.org jiri@resnulli.us
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 57 this patch: 57
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 96 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 70 this patch: 70
netdev/header_inline success Link

Commit Message

Simon Horman Oct. 1, 2021, 11:32 a.m. UTC 
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>
---
 include/net/pkt_cls.h    | 32 ++++++++++++++++++++++++++++++++
 net/sched/cls_flower.c   |  5 +++++
 net/sched/cls_matchall.c |  6 ++++++
 net/sched/cls_u32.c      | 11 +++++++++++
 4 files changed, 54 insertions(+)

Comments

Vlad Buslov Oct. 1, 2021, 5:45 p.m. UTC | #1 
On Fri 01 Oct 2021 at 14:32, 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>
> ---
>  include/net/pkt_cls.h    | 32 ++++++++++++++++++++++++++++++++
>  net/sched/cls_flower.c   |  5 +++++
>  net/sched/cls_matchall.c |  6 ++++++
>  net/sched/cls_u32.c      | 11 +++++++++++
>  4 files changed, 54 insertions(+)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 5c394f04feb5..2d51bed434d2 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -735,6 +735,38 @@ static inline bool tc_in_hw(u32 flags)
>  	return (flags & TCA_CLS_FLAGS_IN_HW) ? true : false;
>  }
>  
> +/**
> + * tcf_exts_validate_actions - check if exts actions flags are compatible with
> + * tc filter flags
> + * @exts: tc filter extensions handle
> + * @flags: tc filter flags
> + *
> + * Returns true if exts actions flags are compatible with tc filter flags
> + */
> +static inline 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
> +}
> +

There is already a function named tcf_exts_validate() that is called by
classifiers before this new one and is responsible for action validation
and initialization. Having two similarly-named functions is confusing
and additional call complicates classifier init implementations, which
are already quite complex as they are. Could you perform the necessary
validation inside existing exts initialization call chain?

>  static inline void
>  tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common,
>  			   const struct tcf_proto *tp, u32 flags,
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index eb6345a027e1..15e439349124 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -2039,6 +2039,11 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
>  	if (err)
>  		goto errout;
>  
> +	if (!tcf_exts_validate_actions(&fnew->exts, fnew->flags)) {
> +		err = -EINVAL;
> +		goto errout;
> +	}
> +
>  	err = fl_check_assign_mask(head, fnew, fold, mask);
>  	if (err)
>  		goto errout;
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 24f0046ce0b3..89dbbb9f31e8 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -231,6 +231,11 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
>  	if (err)
>  		goto err_set_parms;
>  
> +	if (!tcf_exts_validate_actions(&new->exts, new->flags)) {
> +		err = -EINVAL;
> +		goto err_validate;
> +	}
> +
>  	if (!tc_skip_hw(new->flags)) {
>  		err = mall_replace_hw_filter(tp, new, (unsigned long)new,
>  					     extack);
> @@ -246,6 +251,7 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
>  	return 0;
>  
>  err_replace_hw_filter:
> +err_validate:
>  err_set_parms:
>  	free_percpu(new->pf);
>  err_alloc_percpu:
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 4272814487f0..8bc19af18e4a 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -902,6 +902,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  			return err;
>  		}
>  
> +		if (!tcf_exts_validate_actions(&new->exts, flags)) {
> +			u32_destroy_key(new, false);
> +			return -EINVAL;
> +		}
> +
>  		err = u32_replace_hw_knode(tp, new, flags, extack);
>  		if (err) {
>  			u32_destroy_key(new, false);
> @@ -1066,6 +1071,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  		struct tc_u_knode __rcu **ins;
>  		struct tc_u_knode *pins;
>  
> +		if (!tcf_exts_validate_actions(&n->exts, n->flags)) {
> +			err = -EINVAL;
> +			goto err_validate;
> +		}
> +
>  		err = u32_replace_hw_knode(tp, n, flags, extack);
>  		if (err)
>  			goto errhw;
> @@ -1086,6 +1096,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>  		return 0;
>  	}
>  
> +err_validate:
>  errhw:
>  #ifdef CONFIG_CLS_U32_MARK
>  	free_percpu(n->pcpu_success);
Simon Horman Oct. 27, 2021, 2:42 p.m. UTC | #2 
Hi Vlad,

On Fri, Oct 01, 2021 at 08:45:18PM +0300, Vlad Buslov wrote:
> 
> On Fri 01 Oct 2021 at 14:32, 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.

As per comment on 2/4.

        Thanks for your review and sorry for the delay in responding.
        I believe that at this point we have addressed most of the points
        your raised and plan to post a v3 shortly.

        At this point I'd like to relay some responses from Baowen who
        has been working on addressing your review.
...

> > +/**
> > + * tcf_exts_validate_actions - check if exts actions flags are compatible with
> > + * tc filter flags
> > + * @exts: tc filter extensions handle
> > + * @flags: tc filter flags
> > + *
> > + * Returns true if exts actions flags are compatible with tc filter flags
> > + */
> > +static inline bool
> > +tcf_exts_validate_actions(const struct tcf_exts *exts, u32 flags)

...

> There is already a function named tcf_exts_validate() that is called by
> classifiers before this new one and is responsible for action validation
> and initialization. Having two similarly-named functions is confusing
> and additional call complicates classifier init implementations, which
> are already quite complex as they are. Could you perform the necessary
> validation inside existing exts initialization call chain?

Thanks, updated v3 to address this as per your suggestion.

...
diff mbox series

Patch

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 5c394f04feb5..2d51bed434d2 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -735,6 +735,38 @@  static inline bool tc_in_hw(u32 flags)
 	return (flags & TCA_CLS_FLAGS_IN_HW) ? true : false;
 }
 
+/**
+ * tcf_exts_validate_actions - check if exts actions flags are compatible with
+ * tc filter flags
+ * @exts: tc filter extensions handle
+ * @flags: tc filter flags
+ *
+ * Returns true if exts actions flags are compatible with tc filter flags
+ */
+static inline 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
+}
+
 static inline void
 tc_cls_common_offload_init(struct flow_cls_common_offload *cls_common,
 			   const struct tcf_proto *tp, u32 flags,
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index eb6345a027e1..15e439349124 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -2039,6 +2039,11 @@  static int fl_change(struct net *net, struct sk_buff *in_skb,
 	if (err)
 		goto errout;
 
+	if (!tcf_exts_validate_actions(&fnew->exts, fnew->flags)) {
+		err = -EINVAL;
+		goto errout;
+	}
+
 	err = fl_check_assign_mask(head, fnew, fold, mask);
 	if (err)
 		goto errout;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 24f0046ce0b3..89dbbb9f31e8 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -231,6 +231,11 @@  static int mall_change(struct net *net, struct sk_buff *in_skb,
 	if (err)
 		goto err_set_parms;
 
+	if (!tcf_exts_validate_actions(&new->exts, new->flags)) {
+		err = -EINVAL;
+		goto err_validate;
+	}
+
 	if (!tc_skip_hw(new->flags)) {
 		err = mall_replace_hw_filter(tp, new, (unsigned long)new,
 					     extack);
@@ -246,6 +251,7 @@  static int mall_change(struct net *net, struct sk_buff *in_skb,
 	return 0;
 
 err_replace_hw_filter:
+err_validate:
 err_set_parms:
 	free_percpu(new->pf);
 err_alloc_percpu:
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4272814487f0..8bc19af18e4a 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -902,6 +902,11 @@  static int u32_change(struct net *net, struct sk_buff *in_skb,
 			return err;
 		}
 
+		if (!tcf_exts_validate_actions(&new->exts, flags)) {
+			u32_destroy_key(new, false);
+			return -EINVAL;
+		}
+
 		err = u32_replace_hw_knode(tp, new, flags, extack);
 		if (err) {
 			u32_destroy_key(new, false);
@@ -1066,6 +1071,11 @@  static int u32_change(struct net *net, struct sk_buff *in_skb,
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
 
+		if (!tcf_exts_validate_actions(&n->exts, n->flags)) {
+			err = -EINVAL;
+			goto err_validate;
+		}
+
 		err = u32_replace_hw_knode(tp, n, flags, extack);
 		if (err)
 			goto errhw;
@@ -1086,6 +1096,7 @@  static int u32_change(struct net *net, struct sk_buff *in_skb,
 		return 0;
 	}
 
+err_validate:
 errhw:
 #ifdef CONFIG_CLS_U32_MARK
 	free_percpu(n->pcpu_success);