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

[Vlad Buslov <vladbu@nvidia.com>]

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);