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

[Vlad Buslov <vladbu@nvidia.com>]

On Thu 04 Nov 2021 at 07:51, Baowen Zheng <baowen.zheng@corigine.com> wrote:
> 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:
>>>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?
>>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.
>>>
>>>>  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.
>>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.

Indeed, currently available TCA_CLS_FLAGS_* fit into first 16 bits, but
the field itself is 32 bits and with addition of more flags in the
future higher bits may start to be used since TCA_CLS_FLAGS_* and
TCA_ACT_FLAGS_* are independent sets.

>>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.

Please also validate inside tcf_action_init() instead of creating new
tcf_exts_validate_actions() function, if possible. I think this will
lead to cleaner and more simple code.

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