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

[Vlad Buslov <vladbu@nvidia.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;