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

Commit Message

Simon Horman Oct. 28, 2021, 11:06 a.m. UTC

From: Baowen Zheng <[e-mail]>

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 <[e-mail]>
Signed-off-by: Louis Peens <[e-mail]>
Signed-off-by: Simon Horman <[e-mail]>
---
 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(-)

Comments

Vlad Buslov Oct. 29, 2021, 6:01 p.m. UTC | #1

On Thu 28 Oct 2021 at 14:06, Simon Horman <[e-mail]> wrote:
> From: Baowen Zheng <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Louis Peens <[e-mail]>
> Signed-off-by: Simon Horman <[e-mail]>
> ---
>  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;

Jamal Hadi Salim Oct. 30, 2021, 10:54 a.m. UTC | #2

On 2021-10-29 14:01, Vlad Buslov wrote:
> On Thu 28 Oct 2021 at 14:06, Simon Horman <[e-mail]> wrote:
>> From: Baowen Zheng <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Louis Peens <[e-mail]>
>> Signed-off-by: Simon Horman <[e-mail]>
>> ---
>>   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.

If the hardware can independently accept an action offload then
skip_sw per action makes total sense. BTW, my understanding is
_your_ hardware is capable as such at least for policers ;->
And such policers are then shared across filters.
Other than the architectural reason I may have missed something
because I dont see much complexity added as a result.
Are you more worried about slowing down the update rate?

cheers,
jamal

Vlad Buslov Oct. 30, 2021, 2:45 p.m. UTC | #3

On Sat 30 Oct 2021 at 13:54, Jamal Hadi Salim <[e-mail]> wrote:
> On 2021-10-29 14:01, Vlad Buslov wrote:
>> On Thu 28 Oct 2021 at 14:06, Simon Horman <[e-mail]> wrote:
>>> From: Baowen Zheng <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Louis Peens <[e-mail]>
>>> Signed-off-by: Simon Horman <[e-mail]>
>>> ---
>>>   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.
>
> If the hardware can independently accept an action offload then
> skip_sw per action makes total sense. BTW, my understanding is

Example configuration that seems bizarre to me is when offloaded shared
action has skip_sw flag set but filter doesn't. Then behavior of
classifier that points to such action diverges between hardware and
software (different lists of actions are applied). We always try to make
offloaded TC data path behave exactly the same as software and, even
though here it would be explicit and deliberate, I don't see any
practical use-case for this.

> _your_ hardware is capable as such at least for policers ;->
> And such policers are then shared across filters.

True, but why do you need skip_sw action flag for that?

> Other than the architectural reason I may have missed something
> because I dont see much complexity added as a result.

Well, other part of my email was about how I don't understand what is
going on in the flags handling code here. This patch and parts of other
patches in the series would be unnecessary, if we forgo the action
skip_sw flag. I guess we can just make the code nicer by folding the
validation into tcf_action_init(), for example.

> Are you more worried about slowing down the update rate?

I don't expect the validation code to significantly impact the update
rate.

Jamal Hadi Salim Oct. 31, 2021, 1:30 p.m. UTC | #4

On 2021-10-30 22:27, Baowen Zheng wrote:
> Thanks for your review, after some considerarion, I think I understand what you are meaning.
> 

[..]

>>>> 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.
>>>
>>> If the hardware can independently accept an action offload then
>>> skip_sw per action makes total sense. BTW, my understanding is
>>
>> Example configuration that seems bizarre to me is when offloaded shared
>> action has skip_sw flag set but filter doesn't. Then behavior of
>> classifier that points to such action diverges between hardware and
>> software (different lists of actions are applied). We always try to make
>> offloaded TC data path behave exactly the same as software and, even
>> though here it would be explicit and deliberate, I don't see any
>> practical use-case for this.
> We add the skip_sw to keep compatible with the filter flags and give the user an
> option to specify if the action should run in software. I understand what you mean,
> maybe our example is not proper, we need to prevent the filter to run in software if the
> actions it applies is skip_sw, so we need to add more validation to check about this.
> Also I think your suggestion makes full sense if there is no use case to specify the action
> should not run in sw and indeed it will make our implement more simple if we omit the
> skip_sw option.
> Jamal, WDYT?


Let me use an example to illustrate my concern:

#add a policer offload it
tc actions add action police skip_sw rate ... index 20
#now add filter1 which is offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_sw ip_proto tcp action police index 20
#add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_sw ip_proto udp action police index 20

All good so far...
#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_hw ip_proto icmp action police index 20

filter3 should not be allowed.

If we had added the policer without skip_sw and without
skip_hw then i think filter3 should have been legal
(we just need to account for stats in_hw vs in_sw).

Not sure if that makes sense (and addresses Vlad's earlier
comment).


cheers,
jamal

Baowen Zheng Nov. 1, 2021, 3:29 a.m. UTC | #5

On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
>On 2021-10-30 22:27, Baowen Zheng wrote:
>> Thanks for your review, after some considerarion, I think I understand what
>you are meaning.
>>
>
>[..]
>
>>>>> 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.
>>>>
>>>> If the hardware can independently accept an action offload then
>>>> skip_sw per action makes total sense. BTW, my understanding is
>>>
>>> Example configuration that seems bizarre to me is when offloaded
>>> shared action has skip_sw flag set but filter doesn't. Then behavior
>>> of classifier that points to such action diverges between hardware
>>> and software (different lists of actions are applied). We always try
>>> to make offloaded TC data path behave exactly the same as software
>>> and, even though here it would be explicit and deliberate, I don't
>>> see any practical use-case for this.
>> We add the skip_sw to keep compatible with the filter flags and give
>> the user an option to specify if the action should run in software. I
>> understand what you mean, maybe our example is not proper, we need to
>> prevent the filter to run in software if the actions it applies is skip_sw, so we
>need to add more validation to check about this.
>> Also I think your suggestion makes full sense if there is no use case
>> to specify the action should not run in sw and indeed it will make our
>> implement more simple if we omit the skip_sw option.
>> Jamal, WDYT?
>
>
>Let me use an example to illustrate my concern:
>
>#add a policer offload it
>tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
>tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_sw ip_proto udp action police index 20
>
>All good so far...
>#Now add a filter3 which is s/w only
>tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_hw ip_proto icmp action police index 20
>
>filter3 should not be allowed.
>
>If we had added the policer without skip_sw and without skip_hw then i think
>filter3 should have been legal (we just need to account for stats in_hw vs
>in_sw).
>
>Not sure if that makes sense (and addresses Vlad's earlier comment).
>
I think the cases you mentioned make sense to us. But what Vlad concerns is the use
case as: 
#add a policer offload it
tc actions add action police skip_sw rate ... index 20
#now add filter4 which can't be  offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
ip_proto tcp action police index 20
it is possible the filter4 can't be offloaded, then filter4 will run in software,
should this be legal? 
Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
an action? If a packet matches the filter in software, the action should not be skip_sw. 
If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work. 
Of course, we can also keep skip_sw by adding more check to avoid the above case. 

Vlad, I am not sure if I understand your idea correctly.

Vlad Buslov Nov. 1, 2021, 7:38 a.m. UTC | #6

On Mon 01 Nov 2021 at 05:29, Baowen Zheng <[e-mail]> wrote:
> On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
>>On 2021-10-30 22:27, Baowen Zheng wrote:
>>> Thanks for your review, after some considerarion, I think I understand what
>>you are meaning.
>>>
>>
>>[..]
>>
>>>>>> 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.
>>>>>
>>>>> If the hardware can independently accept an action offload then
>>>>> skip_sw per action makes total sense. BTW, my understanding is
>>>>
>>>> Example configuration that seems bizarre to me is when offloaded
>>>> shared action has skip_sw flag set but filter doesn't. Then behavior
>>>> of classifier that points to such action diverges between hardware
>>>> and software (different lists of actions are applied). We always try
>>>> to make offloaded TC data path behave exactly the same as software
>>>> and, even though here it would be explicit and deliberate, I don't
>>>> see any practical use-case for this.
>>> We add the skip_sw to keep compatible with the filter flags and give
>>> the user an option to specify if the action should run in software. I
>>> understand what you mean, maybe our example is not proper, we need to
>>> prevent the filter to run in software if the actions it applies is skip_sw, so we
>>need to add more validation to check about this.
>>> Also I think your suggestion makes full sense if there is no use case
>>> to specify the action should not run in sw and indeed it will make our
>>> implement more simple if we omit the skip_sw option.
>>> Jamal, WDYT?
>>
>>
>>Let me use an example to illustrate my concern:
>>
>>#add a policer offload it
>>tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>>offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
>>     skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
>>tc filter add dev $DEV1 proto ip parent ffff: flower \
>>     skip_sw ip_proto udp action police index 20
>>
>>All good so far...
>>#Now add a filter3 which is s/w only
>>tc filter add dev $DEV1 proto ip parent ffff: flower \
>>     skip_hw ip_proto icmp action police index 20
>>
>>filter3 should not be allowed.
>>
>>If we had added the policer without skip_sw and without skip_hw then i think
>>filter3 should have been legal (we just need to account for stats in_hw vs
>>in_sw).
>>
>>Not sure if that makes sense (and addresses Vlad's earlier comment).
>>
> I think the cases you mentioned make sense to us. But what Vlad concerns is the use
> case as: 
> #add a policer offload it
> tc actions add action police skip_sw rate ... index 20
> #now add filter4 which can't be  offloaded
> tc filter add dev $DEV1 proto ip parent ffff: flower \
> ip_proto tcp action police index 20
> it is possible the filter4 can't be offloaded, then filter4 will run in software,
> should this be legal? 
> Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
> will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
> an action? If a packet matches the filter in software, the action should not be skip_sw. 
> If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work. 
> Of course, we can also keep skip_sw by adding more check to avoid the above case. 
>
> Vlad, I am not sure if I understand your idea correctly. 

My suggestion was to forgo the skip_sw flag for shared action offload
and, consecutively, remove the validation code, not to add even more
checks. I still don't see a practical case where skip_sw shared action
is useful. But I don't have any strong feelings about this flag, so if
Jamal thinks it is necessary, then fine by me.

Simon Horman Nov. 2, 2021, 12:39 p.m. UTC | #7

On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
> On Mon 01 Nov 2021 at 05:29, Baowen Zheng <[e-mail]> wrote:
> > On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
> >>On 2021-10-30 22:27, Baowen Zheng wrote:
> >>> Thanks for your review, after some considerarion, I think I understand what

..

> >>Let me use an example to illustrate my concern:
> >>
> >>#add a policer offload it
> >>tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
> >>offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
> >>     skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
> >>tc filter add dev $DEV1 proto ip parent ffff: flower \
> >>     skip_sw ip_proto udp action police index 20
> >>
> >>All good so far...
> >>#Now add a filter3 which is s/w only
> >>tc filter add dev $DEV1 proto ip parent ffff: flower \
> >>     skip_hw ip_proto icmp action police index 20
> >>
> >>filter3 should not be allowed.
> >>
> >>If we had added the policer without skip_sw and without skip_hw then i think
> >>filter3 should have been legal (we just need to account for stats in_hw vs
> >>in_sw).
> >>
> >>Not sure if that makes sense (and addresses Vlad's earlier comment).
> >>
> > I think the cases you mentioned make sense to us. But what Vlad concerns is the use
> > case as: 
> > #add a policer offload it
> > tc actions add action police skip_sw rate ... index 20
> > #now add filter4 which can't be  offloaded
> > tc filter add dev $DEV1 proto ip parent ffff: flower \
> > ip_proto tcp action police index 20
> > it is possible the filter4 can't be offloaded, then filter4 will run in software,
> > should this be legal? 
> > Originally I think this is legal, but as comments of Vlad, this should not be legal, since the action
> > will not be executed in software. I think what Vlad concerns is do we really need skip_sw flag for
> > an action? If a packet matches the filter in software, the action should not be skip_sw. 
> > If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify our work. 
> > Of course, we can also keep skip_sw by adding more check to avoid the above case. 
> >
> > Vlad, I am not sure if I understand your idea correctly. 
> 
> My suggestion was to forgo the skip_sw flag for shared action offload
> and, consecutively, remove the validation code, not to add even more
> checks. I still don't see a practical case where skip_sw shared action
> is useful. But I don't have any strong feelings about this flag, so if
> Jamal thinks it is necessary, then fine by me.

FWIIW, my feelings are the same as Vlad's.

I think these flags add complexity that would be nice to avoid.
But if Jamal thinks its necessary, then including the flags implementation
is fine by me.

Baowen Zheng Nov. 3, 2021, 7:57 a.m. UTC | #8

On November 2, 2021 8:40 PM, Simon Horman wrote:
>On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
><[e-mail]> wrote:
>> > On 2021-10-31 9:31 PM, Jamal Hadi Salim wrote:
>> >>On 2021-10-30 22:27, Baowen Zheng wrote:
>> >>> Thanks for your review, after some considerarion, I think I
>> >>> understand what
>
>..
>
>> >>Let me use an example to illustrate my concern:
>> >>
>> >>#add a policer offload it
>> >>tc actions add action police skip_sw rate ... index 20 #now add
>> >>filter1 which is offloaded tc filter add dev $DEV1 proto ip parent ffff:
>flower \
>> >>     skip_sw ip_proto tcp action police index 20 #add filter2
>> >>likewise offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
>> >>     skip_sw ip_proto udp action police index 20
>> >>
>> >>All good so far...
>> >>#Now add a filter3 which is s/w only tc filter add dev $DEV1 proto
>> >>ip parent ffff: flower \
>> >>     skip_hw ip_proto icmp action police index 20
>> >>
>> >>filter3 should not be allowed.
>> >>
>> >>If we had added the policer without skip_sw and without skip_hw then
>> >>i think
>> >>filter3 should have been legal (we just need to account for stats
>> >>in_hw vs in_sw).
>> >>
>> >>Not sure if that makes sense (and addresses Vlad's earlier comment).
>> >>
>> > I think the cases you mentioned make sense to us. But what Vlad
>> > concerns is the use case as:
>> > #add a policer offload it
>> > tc actions add action police skip_sw rate ... index 20 #now add
>> > filter4 which can't be  offloaded tc filter add dev $DEV1 proto ip
>> > parent ffff: flower \ ip_proto tcp action police index 20 it is
>> > possible the filter4 can't be offloaded, then filter4 will run in
>> > software, should this be legal?
>> > Originally I think this is legal, but as comments of Vlad, this
>> > should not be legal, since the action will not be executed in
>> > software. I think what Vlad concerns is do we really need skip_sw flag for
>an action? If a packet matches the filter in software, the action should not be
>skip_sw.
>> > If we choose to omit the skip_sw flag and just keep skip_hw, it will simplify
>our work.
>> > Of course, we can also keep skip_sw by adding more check to avoid the
>above case.
>> >
>> > Vlad, I am not sure if I understand your idea correctly.
>>
>> My suggestion was to forgo the skip_sw flag for shared action offload
>> and, consecutively, remove the validation code, not to add even more
>> checks. I still don't see a practical case where skip_sw shared action
>> is useful. But I don't have any strong feelings about this flag, so if
>> Jamal thinks it is necessary, then fine by me.
>
>FWIIW, my feelings are the same as Vlad's.
>
>I think these flags add complexity that would be nice to avoid.
>But if Jamal thinks its necessary, then including the flags implementation is
>fine by me.
Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw flag for user to specify
the action should not run in software?

Jamal Hadi Salim Nov. 3, 2021, 10:13 a.m. UTC | #9

On 2021-11-03 03:57, Baowen Zheng wrote:
> On November 2, 2021 8:40 PM, Simon Horman wrote:
>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng

[..]
>>>
>>> My suggestion was to forgo the skip_sw flag for shared action offload
>>> and, consecutively, remove the validation code, not to add even more
>>> checks. I still don't see a practical case where skip_sw shared action
>>> is useful. But I don't have any strong feelings about this flag, so if
>>> Jamal thinks it is necessary, then fine by me.
>>
>> FWIIW, my feelings are the same as Vlad's.
>>
>> I think these flags add complexity that would be nice to avoid.
>> But if Jamal thinks its necessary, then including the flags implementation is
>> fine by me.
> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw flag for user to specify
> the action should not run in software?
> 

Just catching up with discussion...
IMO, we need the flag. Oz indicated with requirement to be able to
identify the action with an index. So if a specific action is added
for skip_sw (as standalone or alongside a filter) then it cant be
used for skip_hw. To illustrate using extended example:

#filter 1, skip_sw
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_sw ip_proto tcp action police blah index 10

#filter 2, skip_hw
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_hw ip_proto udp action police index 10

Filter2 should be illegal.
And when i dump the actions as so:
tc actions ls action police

For debugability, I should see index 10 clearly marked with
the flag as skip_sw

The other example i gave earlier which showed the sharing
of actions:

#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#now add filter1 which is offloaded using offloaded policer
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_sw ip_proto tcp action police index 20
#add filter2 likewise offloaded
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_sw ip_proto udp action police index 20

All good and filter 1 and 2 are sharing policer instance with
index 20.

#Now add a filter3 which is s/w only
tc filter add dev $DEV1 proto ip parent ffff: flower \
     skip_hw ip_proto icmp action police index 20

filter3 should not be allowed.

cheers,
jamal

Baowen Zheng Nov. 3, 2021, 11:30 a.m. UTC | #10

On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>On 2021-11-03 03:57, Baowen Zheng wrote:
>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>
>[..]
>>>>
>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>> offload and, consecutively, remove the validation code, not to add
>>>> even more checks. I still don't see a practical case where skip_sw
>>>> shared action is useful. But I don't have any strong feelings about
>>>> this flag, so if Jamal thinks it is necessary, then fine by me.
>>>
>>> FWIIW, my feelings are the same as Vlad's.
>>>
>>> I think these flags add complexity that would be nice to avoid.
>>> But if Jamal thinks its necessary, then including the flags
>>> implementation is fine by me.
>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>> flag for user to specify the action should not run in software?
>>
>
>Just catching up with discussion...
>IMO, we need the flag. Oz indicated with requirement to be able to identify
>the action with an index. So if a specific action is added for skip_sw (as
>standalone or alongside a filter) then it cant be used for skip_hw. To illustrate
>using extended example:
>
>#filter 1, skip_sw
>tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_sw ip_proto tcp action police blah index 10
>
>#filter 2, skip_hw
>tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_hw ip_proto udp action police index 10
>
>Filter2 should be illegal.
>And when i dump the actions as so:
>tc actions ls action police
>
>For debugability, I should see index 10 clearly marked with the flag as skip_sw
>
>The other example i gave earlier which showed the sharing of actions:
>
>#add a policer action and offload it
>tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>offloaded using offloaded policer tc filter add dev $DEV1 proto ip parent ffff:
>flower \
>     skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
>tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_sw ip_proto udp action police index 20
>
>All good and filter 1 and 2 are sharing policer instance with index 20.
>
>#Now add a filter3 which is s/w only
>tc filter add dev $DEV1 proto ip parent ffff: flower \
>     skip_hw ip_proto icmp action police index 20
>
>filter3 should not be allowed.
I think the use cases you mentioned above are clear for us. For the case: 

#add a policer action and offload it
tc actions add action police skip_sw rate ... index 20
#Now add a filter4 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
     ip_proto icmp action police index 20

Is filter4 legal? basically, it should be legal, but since filter4 may be offloaded failed so 
it will run in software, you know the action police should not run in software with skip_sw,
so I think filter4 should be illegal and we should not allow this case. 
That is if the action is skip_sw, then the filter refers to this action should also skip_sw. 
WDYT?

Jamal Hadi Salim Nov. 3, 2021, 12:33 p.m. UTC | #11

On 2021-11-03 07:30, Baowen Zheng wrote:
> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>
>> [..]
>>>>>
>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>> offload and, consecutively, remove the validation code, not to add
>>>>> even more checks. I still don't see a practical case where skip_sw
>>>>> shared action is useful. But I don't have any strong feelings about
>>>>> this flag, so if Jamal thinks it is necessary, then fine by me.
>>>>
>>>> FWIIW, my feelings are the same as Vlad's.
>>>>
>>>> I think these flags add complexity that would be nice to avoid.
>>>> But if Jamal thinks its necessary, then including the flags
>>>> implementation is fine by me.
>>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>>> flag for user to specify the action should not run in software?
>>>
>>
>> Just catching up with discussion...
>> IMO, we need the flag. Oz indicated with requirement to be able to identify
>> the action with an index. So if a specific action is added for skip_sw (as
>> standalone or alongside a filter) then it cant be used for skip_hw. To illustrate
>> using extended example:
>>
>> #filter 1, skip_sw
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_sw ip_proto tcp action police blah index 10
>>
>> #filter 2, skip_hw
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_hw ip_proto udp action police index 10
>>
>> Filter2 should be illegal.
>> And when i dump the actions as so:
>> tc actions ls action police
>>
>> For debugability, I should see index 10 clearly marked with the flag as skip_sw
>>
>> The other example i gave earlier which showed the sharing of actions:
>>
>> #add a policer action and offload it
>> tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>> offloaded using offloaded policer tc filter add dev $DEV1 proto ip parent ffff:
>> flower \
>>      skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_sw ip_proto udp action police index 20
>>
>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>
>> #Now add a filter3 which is s/w only
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_hw ip_proto icmp action police index 20
>>
>> filter3 should not be allowed.
> I think the use cases you mentioned above are clear for us. For the case:
> 
> #add a policer action and offload it
> tc actions add action police skip_sw rate ... index 20
> #Now add a filter4 which has no flag
> tc filter add dev $DEV1 proto ip parent ffff: flower \
>       ip_proto icmp action police index 20
> 
> Is filter4 legal? 

Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither
skip_sw nor skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have
counters for both s/w and h/w (which i think is taken care of today).


>basically, it should be legal, but since filter4 may be offloaded failed so
> it will run in software, you know the action police should not run in software with skip_sw,
> so I think filter4 should be illegal and we should not allow this case.
> That is if the action is skip_sw, then the filter refers to this action should also skip_sw.
> WDYT?

See above..

cheers,
jamal

Jamal Hadi Salim Nov. 3, 2021, 1:33 p.m. UTC | #12

On 2021-11-03 08:33, Jamal Hadi Salim wrote:
> On 2021-11-03 07:30, Baowen Zheng wrote:
>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>>
>>> [..]
>>>>>>
>>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>>> offload and, consecutively, remove the validation code, not to add
>>>>>> even more checks. I still don't see a practical case where skip_sw
>>>>>> shared action is useful. But I don't have any strong feelings about
>>>>>> this flag, so if Jamal thinks it is necessary, then fine by me.
>>>>>
>>>>> FWIIW, my feelings are the same as Vlad's.
>>>>>
>>>>> I think these flags add complexity that would be nice to avoid.
>>>>> But if Jamal thinks its necessary, then including the flags
>>>>> implementation is fine by me.
>>>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>>>> flag for user to specify the action should not run in software?
>>>>
>>>
>>> Just catching up with discussion...
>>> IMO, we need the flag. Oz indicated with requirement to be able to 
>>> identify
>>> the action with an index. So if a specific action is added for 
>>> skip_sw (as
>>> standalone or alongside a filter) then it cant be used for skip_hw. 
>>> To illustrate
>>> using extended example:
>>>
>>> #filter 1, skip_sw
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_sw ip_proto tcp action police blah index 10
>>>
>>> #filter 2, skip_hw
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_hw ip_proto udp action police index 10
>>>
>>> Filter2 should be illegal.
>>> And when i dump the actions as so:
>>> tc actions ls action police
>>>
>>> For debugability, I should see index 10 clearly marked with the flag 
>>> as skip_sw
>>>
>>> The other example i gave earlier which showed the sharing of actions:
>>>
>>> #add a policer action and offload it
>>> tc actions add action police skip_sw rate ... index 20 #now add 
>>> filter1 which is
>>> offloaded using offloaded policer tc filter add dev $DEV1 proto ip 
>>> parent ffff:
>>> flower \
>>>      skip_sw ip_proto tcp action police index 20 #add filter2 
>>> likewise offloaded
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_sw ip_proto udp action police index 20
>>>
>>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>>
>>> #Now add a filter3 which is s/w only
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_hw ip_proto icmp action police index 20
>>>
>>> filter3 should not be allowed.
>> I think the use cases you mentioned above are clear for us. For the case:
>>
>> #add a policer action and offload it
>> tc actions add action police skip_sw rate ... index 20
>> #Now add a filter4 which has no flag
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>       ip_proto icmp action police index 20
>>
>> Is filter4 legal? 
> 
> Yes it is _based on current semantics_.
> The reason is when adding a filter and specifying neither
> skip_sw nor skip_hw it defaults to allowing both.
> i.e is the same as skip_sw|skip_hw. You will need to have
> counters for both s/w and h/w (which i think is taken care of today).
> 
> 

Apologies, i will like to take this one back. Couldnt stop thinking
about it while sipping coffee;->
To be safe that should be illegal. The flags have to match _exactly_
for both  action and filter to make any sense. i.e in the above case
they are not.

cheers,
jamal

Baowen Zheng Nov. 3, 2021, 1:37 p.m. UTC | #13

On November 3, 2021 8:34 PM, Jamal Hadi Salim wrote:
>On 2021-11-03 07:30, Baowen Zheng wrote:
>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>>
>>> [..]
>>>>>>
>>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>>> offload and, consecutively, remove the validation code, not to add
>>>>>> even more checks. I still don't see a practical case where skip_sw
>>>>>> shared action is useful. But I don't have any strong feelings
>>>>>> about this flag, so if Jamal thinks it is necessary, then fine by me.
>>>>>
>>>>> FWIIW, my feelings are the same as Vlad's.
>>>>>
>>>>> I think these flags add complexity that would be nice to avoid.
>>>>> But if Jamal thinks its necessary, then including the flags
>>>>> implementation is fine by me.
>>>> Thanks Simon. Jamal, do you think it is necessary to keep the
>>>> skip_sw flag for user to specify the action should not run in software?
>>>>
>>>
>>> Just catching up with discussion...
>>> IMO, we need the flag. Oz indicated with requirement to be able to
>>> identify the action with an index. So if a specific action is added
>>> for skip_sw (as standalone or alongside a filter) then it cant be
>>> used for skip_hw. To illustrate using extended example:
>>>
>>> #filter 1, skip_sw
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_sw ip_proto tcp action police blah index 10
>>>
>>> #filter 2, skip_hw
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_hw ip_proto udp action police index 10
>>>
>>> Filter2 should be illegal.
>>> And when i dump the actions as so:
>>> tc actions ls action police
>>>
>>> For debugability, I should see index 10 clearly marked with the flag
>>> as skip_sw
>>>
>>> The other example i gave earlier which showed the sharing of actions:
>>>
>>> #add a policer action and offload it
>>> tc actions add action police skip_sw rate ... index 20 #now add
>>> filter1 which is offloaded using offloaded policer tc filter add dev $DEV1
>proto ip parent ffff:
>>> flower \
>>>      skip_sw ip_proto tcp action police index 20 #add filter2
>>> likewise offloaded tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_sw ip_proto udp action police index 20
>>>
>>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>>
>>> #Now add a filter3 which is s/w only
>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>      skip_hw ip_proto icmp action police index 20
>>>
>>> filter3 should not be allowed.
>> I think the use cases you mentioned above are clear for us. For the case:
>>
>> #add a policer action and offload it
>> tc actions add action police skip_sw rate ... index 20 #Now add a
>> filter4 which has no flag tc filter add dev $DEV1 proto ip parent
>> ffff: flower \
>>       ip_proto icmp action police index 20
>>
>> Is filter4 legal?
>
>Yes it is _based on current semantics_.
>The reason is when adding a filter and specifying neither skip_sw nor skip_hw
>it defaults to allowing both.
>i.e is the same as skip_sw|skip_hw. You will need to have counters for both
>s/w and h/w (which i think is taken care of today).
Thanks, but what we concern is not the counters but the behavior of this filter. 
Since the filter runs in software and action is skip_sw, so the action will not execute in software. 
So when the packet matches the filter, it will execute all the actions except the skip_sw action. 
I think it is not what we expect, we expect the packet execute all the actions the filter refers to. 
So I think in this case, filter4 should not be allowed.
WDYT?
>>basically, it should be legal, but since filter4 may be offloaded
>>failed so  it will run in software, you know the action police should
>>not run in software with skip_sw,  so I think filter4 should be illegal and we
>should not allow this case.
>> That is if the action is skip_sw, then the filter refers to this action should also
>skip_sw.
>> WDYT?
>
>See above..
>
>cheers,
>jamal

Simon Horman Nov. 3, 2021, 1:38 p.m. UTC | #14

On Wed, Nov 03, 2021 at 09:33:52AM -0400, Jamal Hadi Salim wrote:
> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
> > On 2021-11-03 07:30, Baowen Zheng wrote:
> > > On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
> > > > On 2021-11-03 03:57, Baowen Zheng wrote:
> > > > > On November 2, 2021 8:40 PM, Simon Horman wrote:
> > > > > > On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
> > > > > > > On Mon 01 Nov 2021 at 05:29, Baowen Zheng
> > > > 
> > > > [..]
> > > > > > > 
> > > > > > > My suggestion was to forgo the skip_sw flag for shared action
> > > > > > > offload and, consecutively, remove the validation code, not to add
> > > > > > > even more checks. I still don't see a practical case where skip_sw
> > > > > > > shared action is useful. But I don't have any strong feelings about
> > > > > > > this flag, so if Jamal thinks it is necessary, then fine by me.
> > > > > > 
> > > > > > FWIIW, my feelings are the same as Vlad's.
> > > > > > 
> > > > > > I think these flags add complexity that would be nice to avoid.
> > > > > > But if Jamal thinks its necessary, then including the flags
> > > > > > implementation is fine by me.
> > > > > Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
> > > > > flag for user to specify the action should not run in software?
> > > > > 
> > > > 
> > > > Just catching up with discussion...
> > > > IMO, we need the flag. Oz indicated with requirement to be able
> > > > to identify
> > > > the action with an index. So if a specific action is added for
> > > > skip_sw (as
> > > > standalone or alongside a filter) then it cant be used for
> > > > skip_hw. To illustrate
> > > > using extended example:
> > > > 
> > > > #filter 1, skip_sw
> > > > tc filter add dev $DEV1 proto ip parent ffff: flower \
> > > >      skip_sw ip_proto tcp action police blah index 10
> > > > 
> > > > #filter 2, skip_hw
> > > > tc filter add dev $DEV1 proto ip parent ffff: flower \
> > > >      skip_hw ip_proto udp action police index 10
> > > > 
> > > > Filter2 should be illegal.
> > > > And when i dump the actions as so:
> > > > tc actions ls action police
> > > > 
> > > > For debugability, I should see index 10 clearly marked with the
> > > > flag as skip_sw
> > > > 
> > > > The other example i gave earlier which showed the sharing of actions:
> > > > 
> > > > #add a policer action and offload it
> > > > tc actions add action police skip_sw rate ... index 20 #now add
> > > > filter1 which is
> > > > offloaded using offloaded policer tc filter add dev $DEV1 proto
> > > > ip parent ffff:
> > > > flower \
> > > >      skip_sw ip_proto tcp action police index 20 #add filter2
> > > > likewise offloaded
> > > > tc filter add dev $DEV1 proto ip parent ffff: flower \
> > > >      skip_sw ip_proto udp action police index 20
> > > > 
> > > > All good and filter 1 and 2 are sharing policer instance with index 20.
> > > > 
> > > > #Now add a filter3 which is s/w only
> > > > tc filter add dev $DEV1 proto ip parent ffff: flower \
> > > >      skip_hw ip_proto icmp action police index 20
> > > > 
> > > > filter3 should not be allowed.
> > > I think the use cases you mentioned above are clear for us. For the case:
> > > 
> > > #add a policer action and offload it
> > > tc actions add action police skip_sw rate ... index 20
> > > #Now add a filter4 which has no flag
> > > tc filter add dev $DEV1 proto ip parent ffff: flower \
> > >       ip_proto icmp action police index 20
> > > 
> > > Is filter4 legal?
> > 
> > Yes it is _based on current semantics_.
> > The reason is when adding a filter and specifying neither
> > skip_sw nor skip_hw it defaults to allowing both.
> > i.e is the same as skip_sw|skip_hw. You will need to have
> > counters for both s/w and h/w (which i think is taken care of today).
> > 
> > 
> 
> Apologies, i will like to take this one back. Couldnt stop thinking
> about it while sipping coffee;->
> To be safe that should be illegal. The flags have to match _exactly_
> for both  action and filter to make any sense. i.e in the above case
> they are not.

I could be wrong, but I would have thought that in this case the flow
is legal but is only added to hw (because the action doesn't exist in sw).

But if you prefer to make it illegal I guess that is ok too.

Baowen Zheng Nov. 3, 2021, 2:03 p.m. UTC | #15

Thanks for your reply.
On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
>On 2021-11-03 08:33, Jamal Hadi Salim wrote:
>> On 2021-11-03 07:30, Baowen Zheng wrote:
>>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>>>
>>>> [..]
>>>>>>>
>>>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>>>> offload and, consecutively, remove the validation code, not to
>>>>>>> add even more checks. I still don't see a practical case where
>>>>>>> skip_sw shared action is useful. But I don't have any strong
>>>>>>> feelings about this flag, so if Jamal thinks it is necessary, then fine by
>me.
>>>>>>
>>>>>> FWIIW, my feelings are the same as Vlad's.
>>>>>>
>>>>>> I think these flags add complexity that would be nice to avoid.
>>>>>> But if Jamal thinks its necessary, then including the flags
>>>>>> implementation is fine by me.
>>>>> Thanks Simon. Jamal, do you think it is necessary to keep the
>>>>> skip_sw flag for user to specify the action should not run in software?
>>>>>
>>>>
>>>> Just catching up with discussion...
>>>> IMO, we need the flag. Oz indicated with requirement to be able to
>>>> identify the action with an index. So if a specific action is added
>>>> for skip_sw (as standalone or alongside a filter) then it cant be
>>>> used for skip_hw.
>>>> To illustrate
>>>> using extended example:
>>>>
>>>> #filter 1, skip_sw
>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>      skip_sw ip_proto tcp action police blah index 10
>>>>
>>>> #filter 2, skip_hw
>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>      skip_hw ip_proto udp action police index 10
>>>>
>>>> Filter2 should be illegal.
>>>> And when i dump the actions as so:
>>>> tc actions ls action police
>>>>
>>>> For debugability, I should see index 10 clearly marked with the flag
>>>> as skip_sw
>>>>
>>>> The other example i gave earlier which showed the sharing of actions:
>>>>
>>>> #add a policer action and offload it tc actions add action police
>>>> skip_sw rate ... index 20 #now add
>>>> filter1 which is
>>>> offloaded using offloaded policer tc filter add dev $DEV1 proto ip
>>>> parent ffff:
>>>> flower \
>>>>      skip_sw ip_proto tcp action police index 20 #add filter2
>>>> likewise offloaded tc filter add dev $DEV1 proto ip parent ffff:
>>>> flower \
>>>>      skip_sw ip_proto udp action police index 20
>>>>
>>>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>>>
>>>> #Now add a filter3 which is s/w only tc filter add dev $DEV1 proto
>>>> ip parent ffff: flower \
>>>>      skip_hw ip_proto icmp action police index 20
>>>>
>>>> filter3 should not be allowed.
>>> I think the use cases you mentioned above are clear for us. For the case:
>>>
>>> #add a policer action and offload it
>>> tc actions add action police skip_sw rate ... index 20 #Now add a
>>> filter4 which has no flag tc filter add dev $DEV1 proto ip parent
>>> ffff: flower \
>>>       ip_proto icmp action police index 20
>>>
>>> Is filter4 legal?
>>
>> Yes it is _based on current semantics_.
>> The reason is when adding a filter and specifying neither skip_sw nor
>> skip_hw it defaults to allowing both.
>> i.e is the same as skip_sw|skip_hw. You will need to have counters for
>> both s/w and h/w (which i think is taken care of today).
>>
>>
>
>Apologies, i will like to take this one back. Couldnt stop thinking about it while
>sipping coffee;-> To be safe that should be illegal. The flags have to match
>_exactly_ for both  action and filter to make any sense. i.e in the above case
>they are not.
Thanks. I think we have get agreement that filter4 is illegal. 
Sorry for more clarification about another case that Vlad mentioned: 
#add a policer action with skip_hw
tc actions add action police skip_hw rate ... index 20
#Now add a  filter5 which has no flag
tc filter add dev $DEV1 proto ip parent ffff: flower \
       ip_proto icmp action police index 20
I think the filter5 could be legal, since it will not run in hardware. 
Driver will check failed when try to offload this filter. So the filter5 will only run in software.
WDYT?

Jamal Hadi Salim Nov. 3, 2021, 2:05 p.m. UTC | #16

On 2021-11-03 09:38, Simon Horman wrote:
> On Wed, Nov 03, 2021 at 09:33:52AM -0400, Jamal Hadi Salim wrote:
>> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
>>> On 2021-11-03 07:30, Baowen Zheng wrote:
>>>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>>>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>>>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>>>>
>>>>> [..]
>>>>>>>>
>>>>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>>>>> offload and, consecutively, remove the validation code, not to add
>>>>>>>> even more checks. I still don't see a practical case where skip_sw
>>>>>>>> shared action is useful. But I don't have any strong feelings about
>>>>>>>> this flag, so if Jamal thinks it is necessary, then fine by me.
>>>>>>>
>>>>>>> FWIIW, my feelings are the same as Vlad's.
>>>>>>>
>>>>>>> I think these flags add complexity that would be nice to avoid.
>>>>>>> But if Jamal thinks its necessary, then including the flags
>>>>>>> implementation is fine by me.
>>>>>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>>>>>> flag for user to specify the action should not run in software?
>>>>>>
>>>>>
>>>>> Just catching up with discussion...
>>>>> IMO, we need the flag. Oz indicated with requirement to be able
>>>>> to identify
>>>>> the action with an index. So if a specific action is added for
>>>>> skip_sw (as
>>>>> standalone or alongside a filter) then it cant be used for
>>>>> skip_hw. To illustrate
>>>>> using extended example:
>>>>>
>>>>> #filter 1, skip_sw
>>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>>       skip_sw ip_proto tcp action police blah index 10
>>>>>
>>>>> #filter 2, skip_hw
>>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>>       skip_hw ip_proto udp action police index 10
>>>>>
>>>>> Filter2 should be illegal.
>>>>> And when i dump the actions as so:
>>>>> tc actions ls action police
>>>>>
>>>>> For debugability, I should see index 10 clearly marked with the
>>>>> flag as skip_sw
>>>>>
>>>>> The other example i gave earlier which showed the sharing of actions:
>>>>>
>>>>> #add a policer action and offload it
>>>>> tc actions add action police skip_sw rate ... index 20 #now add
>>>>> filter1 which is
>>>>> offloaded using offloaded policer tc filter add dev $DEV1 proto
>>>>> ip parent ffff:
>>>>> flower \
>>>>>       skip_sw ip_proto tcp action police index 20 #add filter2
>>>>> likewise offloaded
>>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>>       skip_sw ip_proto udp action police index 20
>>>>>
>>>>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>>>>
>>>>> #Now add a filter3 which is s/w only
>>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>>       skip_hw ip_proto icmp action police index 20
>>>>>
>>>>> filter3 should not be allowed.
>>>> I think the use cases you mentioned above are clear for us. For the case:
>>>>
>>>> #add a policer action and offload it
>>>> tc actions add action police skip_sw rate ... index 20
>>>> #Now add a filter4 which has no flag
>>>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>>>        ip_proto icmp action police index 20
>>>>
>>>> Is filter4 legal?
>>>
>>> Yes it is _based on current semantics_.
>>> The reason is when adding a filter and specifying neither
>>> skip_sw nor skip_hw it defaults to allowing both.
>>> i.e is the same as skip_sw|skip_hw. You will need to have
>>> counters for both s/w and h/w (which i think is taken care of today).
>>>
>>>
>>
>> Apologies, i will like to take this one back. Couldnt stop thinking
>> about it while sipping coffee;->
>> To be safe that should be illegal. The flags have to match _exactly_
>> for both  action and filter to make any sense. i.e in the above case
>> they are not.
> 
> I could be wrong, but I would have thought that in this case the flow
> is legal but is only added to hw (because the action doesn't exist in sw).
> 

I was worried what would show up in a dump of the filter.
Would it show only the h/w counter? And if yes, is the s/w
version mutated with no policer (since the policer is only
in h/w)?

> But if you prefer to make it illegal I guess that is ok too.

It just seemed easier from manageability pov to make it illegal, no?
i.e if flags dont match exactly it is illegal is a simple check.

cheers,
jamal

Jamal Hadi Salim Nov. 3, 2021, 2:16 p.m. UTC | #17

On 2021-11-03 10:03, Baowen Zheng wrote:
> Thanks for your reply.
> On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
>> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
>>> On 2021-11-03 07:30, Baowen Zheng wrote:
>>>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:


[..]

> Sorry for more clarification about another case that Vlad mentioned:
> #add a policer action with skip_hw
> tc actions add action police skip_hw rate ... index 20
> #Now add a  filter5 which has no flag
> tc filter add dev $DEV1 proto ip parent ffff: flower \
>         ip_proto icmp action police index 20
> I think the filter5 could be legal, since it will not run in hardware.
> Driver will check failed when try to offload this filter. So the filter5 will only run in software.
> WDYT?
> 

I think this one also has ambiguity. If the filter doesnt specify 
skip_sw or skip_hw it will run both in s/w and h/w. I am worried if
that looks suprising to someone debugging after because in h/w
there is filter 5 but no policer but in s/w twin we have filter 5
and policer index 20.
It could be design intent, but in my opinion we have priorities
to resolve such ambiguities in policies.

If we use the rule which says the flags have to match exactly then we
can simplify resolving any ambiguity - which will make it illegal, no?

cheers,
jamal

Baowen Zheng Nov. 3, 2021, 2:48 p.m. UTC | #18

On November 3, 2021 10:16 PM, Jamal Hadi Salim wrote:
>On 2021-11-03 10:03, Baowen Zheng wrote:
>> Thanks for your reply.
>> On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
>>> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
>>>> On 2021-11-03 07:30, Baowen Zheng wrote:
>>>>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>
>
>[..]
>
>> Sorry for more clarification about another case that Vlad mentioned:
>> #add a policer action with skip_hw
>> tc actions add action police skip_hw rate ... index 20 #Now add a
>> filter5 which has no flag tc filter add dev $DEV1 proto ip parent
>> ffff: flower \
>>         ip_proto icmp action police index 20 I think the filter5 could
>> be legal, since it will not run in hardware.
>> Driver will check failed when try to offload this filter. So the filter5 will only
>run in software.
>> WDYT?
>>
>
>I think this one also has ambiguity. If the filter doesnt specify skip_sw or
>skip_hw it will run both in s/w and h/w. I am worried if that looks suprising to
>someone debugging after because in h/w there is filter 5 but no policer but in
>s/w twin we have filter 5 and policer index 20.
In this case, the filter will not in h/w because when the driver tries to offload the filter,
It will found the action is not in h/w and return failed, then the filter will not in h/w, so the filter will only
In software.
>It could be design intent, but in my opinion we have priorities to resolve such
>ambiguities in policies.
>
>If we use the rule which says the flags have to match exactly then we can
>simplify resolving any ambiguity - which will make it illegal, no?
When you mentioned " match exactly ", do you mean the flags of the filter and the actions should be
exactly same? 
Please consider the case that filter has flag and the action does not have any flag. I think we should allow this case.
Because it is legal before our patch, we do not expect to break this use case, yes?
So maybe the "match exactly" just limits action flags, when action has flags(skip_sw or skip_hw), the filter must have
exactly the same flags. 
WDYT?

Jamal Hadi Salim Nov. 3, 2021, 3:35 p.m. UTC | #19

On 2021-11-03 10:48, Baowen Zheng wrote:
> On November 3, 2021 10:16 PM, Jamal Hadi Salim wrote:
>> On 2021-11-03 10:03, Baowen Zheng wrote:
>>> Thanks for your reply.
>>> On November 3, 2021 9:34 PM, Jamal Hadi Salim wrote:
>>>> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
>>>>> On 2021-11-03 07:30, Baowen Zheng wrote:
>>>>>> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>>
>>
>> [..]
>>
>>> Sorry for more clarification about another case that Vlad mentioned:
>>> #add a policer action with skip_hw
>>> tc actions add action police skip_hw rate ... index 20 #Now add a
>>> filter5 which has no flag tc filter add dev $DEV1 proto ip parent
>>> ffff: flower \
>>>          ip_proto icmp action police index 20 I think the filter5 could
>>> be legal, since it will not run in hardware.
>>> Driver will check failed when try to offload this filter. So the filter5 will only
>> run in software.
>>> WDYT?
>>>
>>
>> I think this one also has ambiguity. If the filter doesnt specify skip_sw or
>> skip_hw it will run both in s/w and h/w. I am worried if that looks suprising to
>> someone debugging after because in h/w there is filter 5 but no policer but in
>> s/w twin we have filter 5 and policer index 20.
> In this case, the filter will not in h/w because when the driver tries to offload the filter,
> It will found the action is not in h/w and return failed, then the filter will not in h/w, so the filter will only
> In software.

So you have partial failure? That doesnt sound good. What do you return
to the user - "success" or "somehow success"?
I worry it is still ambigous. Did the user really intend to do that?
If they did maybe they should have just added it to s/w instead of h/w
and s/w and then get saved by the driver?

>> It could be design intent, but in my opinion we have priorities to resolve such
>> ambiguities in policies.
>>
>> If we use the rule which says the flags have to match exactly then we can
>> simplify resolving any ambiguity - which will make it illegal, no?
> When you mentioned " match exactly ", do you mean the flags of the filter and the actions should be
> exactly same?
> Please consider the case that filter has flag and the action does not have any flag. 

See above.

> I think we should allow this case.
> Because it is legal before our patch, we do not expect to break this use case, yes?
> So maybe the "match exactly" just limits action flags, when action has flags(skip_sw or skip_hw), the filter must have
> exactly the same flags.

Maybe i am missing something but nothing should break.
I think what you mean is when the action is specified with
the filter. The flags should be the same in that case.

Example, filter 1:
tc filter add dev $DEV1 proto ip paren ffff: flower \
ip_proto icmp action police blah

where flag is 0 implies this filter goes both in h/w and s/w.
If i dump the policer or the filter i will see some index provided by
the kernel and i should be able to see both s/w and h/w
counters.

Same thing if i did:

Example filter 2:
tc filter add dev $DEV1 proto ip paren ffff: flower \
skip_sw ip_proto udp action police blah

both filter + action will have where flag of skip_sw when i dump
implies this filter goes only in h/w and any displayed index
is allocated by the kernel.


Our challenge is when someone specifies a specific action by index
and tries to use it ambigously.

cheers,
jamal

Baowen Zheng Nov. 4, 2021, 2:30 a.m. UTC | #20

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 <[e-mail]>
>wrote:
>> From: Baowen Zheng <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Louis Peens <[e-mail]>
>> Signed-off-by: Simon Horman <[e-mail]>
>> ---
>>  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. 
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? 
>
>>  	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;

Baowen Zheng Nov. 4, 2021, 5:51 a.m. UTC | #21

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 <[e-mail]>
>>wrote:
>>> From: Baowen Zheng <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Louis Peens <[e-mail]>
>>> Signed-off-by: Simon Horman <[e-mail]>
>>> ---
>>>  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.
>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.
>>
>>>  	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;

Vlad Buslov Nov. 4, 2021, 9:07 a.m. UTC | #22

On Thu 04 Nov 2021 at 07:51, Baowen Zheng <[e-mail]> 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 <[e-mail]>
>>>wrote:
>>>> From: Baowen Zheng <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Louis Peens <[e-mail]>
>>>> Signed-off-by: Simon Horman <[e-mail]>
>>>> ---
>>>>  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;

Baowen Zheng Nov. 4, 2021, 11:15 a.m. UTC | #23

On November 4, 2021 5:07 PM, Vlad Buslov wrote:
>On Thu 04 Nov 2021 at 07:51, Baowen Zheng <[e-mail]>
>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
><[e-mail]>
>>>>wrote:
>>>>> From: Baowen Zheng <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Louis Peens <[e-mail]>
>>>>> Signed-off-by: Simon Horman <[e-mail]>
>>>>> ---
>>>>>  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.
Thanks, we will use a single parameter as the filter flag.
>
>>>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.
Thanks, we will consider to implement the validation inside tcf_action_init().
>
>>>>
>>>>>  	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;

Patch

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