[PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[Vladimir Oltean]

tcf_action_init() has logic for checking mismatches between action and
filter offload flags (skip_sw/skip_hw). AFAIU, this is intended to run
on the transition between the new tc_act_bind(flags) returning true (aka
now gets bound to classifier) and tc_act_bind(act->tcfa_flags) returning
false (aka action was not bound to classifier before). Otherwise, the
check is skipped.

For the case where an action is not standalone, but rather it was
created by a classifier and is bound to it, tcf_action_init() skips the
check entirely, and this means it allows mismatched flags to occur.

Taking the matchall classifier code path as an example (with mirred as
an action), the reason is the following:

 1 | mall_change()
 2 | -> mall_replace_hw_filter()
 3 |   -> tcf_exts_validate_ex()
 4 |      -> flags |= TCA_ACT_FLAGS_BIND;
 5 |      -> tcf_action_init()
 6 |         -> tcf_action_init_1()
 7 |            -> a_o->init()
 8 |               -> tcf_mirred_init()
 9 |                  -> tcf_idr_create_from_flags()
10 |                     -> tcf_idr_create()
11 |                        -> p->tcfa_flags = flags;
12 |         -> tc_act_bind(flags))
13 |         -> tc_act_bind(act->tcfa_flags)

When invoked from tcf_exts_validate_ex() like matchall does (but other
classifiers validate their extensions as well), tcf_action_init() runs
in a call path where "flags" always contains TCA_ACT_FLAGS_BIND (set by
line 4). So line 12 is always true, and line 13 is always true as well.
No transition ever takes place, and the check is skipped.

The code was added in this form in commit c86e0209dc77 ("flow_offload:
validate flags of filter and actions"), but I'm attributing the blame
even earlier in that series, to when TCA_ACT_FLAGS_SKIP_HW and
TCA_ACT_FLAGS_SKIP_SW were added to the UAPI.

Following the development process of this change, the check did not
always exist in this form. A change took place between v3 [1] and v4 [2],
AFAIU due to review feedback that it doesn't make sense for action flags
to be different than classifier flags. I think I agree with that
feedback, but it was translated into code that omits enforcing this for
"classic" actions created at the same time with the filters themselves.

There are 3 more important cases to discuss. First there is this command:

$ tc qdisc add dev eth0 clasct
$ tc filter add dev eth0 ingress matchall skip_sw \
	action mirred ingress mirror dev eth1

which should be allowed, because prior to the concept of dedicated
action flags, it used to work and it used to mean the action inherited
the skip_sw/skip_hw flags from the classifier. It's not a mismatch.

Then we have this command:

$ tc qdisc add dev eth0 clasct
$ tc filter add dev eth0 ingress matchall skip_sw \
	action mirred ingress mirror dev eth1 skip_hw

where there is a mismatch and it should be rejected.

Finally, we have:

$ tc qdisc add dev eth0 clasct
$ tc filter add dev eth0 ingress matchall skip_sw \
	action mirred ingress mirror dev eth1 skip_sw

where the offload flags coincide, and this should be treated the same as
the first command based on inheritance, and accepted.

[1]: https://lore.kernel.org/netdev/20211028110646.13791-9-simon.horman@corigine.com/
[2]: https://lore.kernel.org/netdev/20211118130805.23897-10-simon.horman@corigine.com/
Fixes: 7adc57651211 ("flow_offload: add skip_hw and skip_sw to control if offload the action")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/sched/act_api.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 5bbfb83ed600..8e705b212c14 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1498,8 +1498,29 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 			bool skip_sw = tc_skip_sw(fl_flags);
 			bool skip_hw = tc_skip_hw(fl_flags);
 
-			if (tc_act_bind(act->tcfa_flags))
+			if (tc_act_bind(act->tcfa_flags)) {
+				/* Action is created by classifier and is not
+				 * standalone. Check that the user did not set
+				 * any action flags different than the
+				 * classifier flags, and inherit the flags from
+				 * the classifier for the compatibility case
+				 * where no flags were specified at all.
+				 */
+				if ((tc_act_skip_sw(act->tcfa_flags) && !skip_sw) ||
+				    (tc_act_skip_hw(act->tcfa_flags) && !skip_hw)) {
+					NL_SET_ERR_MSG(extack,
+						       "Mismatch between action and filter offload flags");
+					err = -EINVAL;
+					goto err;
+				}
+				if (skip_sw)
+					act->tcfa_flags |= TCA_ACT_FLAGS_SKIP_SW;
+				if (skip_hw)
+					act->tcfa_flags |= TCA_ACT_FLAGS_SKIP_HW;
 				continue;
+			}
+
+			/* Action is standalone */
 			if (skip_sw != tc_act_skip_sw(act->tcfa_flags) ||
 			    skip_hw != tc_act_skip_hw(act->tcfa_flags)) {
 				NL_SET_ERR_MSG(extack,
-- 
2.43.0

Re: [PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[Simon Horman]

On Thu, Oct 17, 2024 at 07:10:48PM +0300, Vladimir Oltean wrote:
> tcf_action_init() has logic for checking mismatches between action and
> filter offload flags (skip_sw/skip_hw). AFAIU, this is intended to run
> on the transition between the new tc_act_bind(flags) returning true (aka
> now gets bound to classifier) and tc_act_bind(act->tcfa_flags) returning
> false (aka action was not bound to classifier before). Otherwise, the
> check is skipped.
> 
> For the case where an action is not standalone, but rather it was
> created by a classifier and is bound to it, tcf_action_init() skips the
> check entirely, and this means it allows mismatched flags to occur.
> 
> Taking the matchall classifier code path as an example (with mirred as
> an action), the reason is the following:
> 
>  1 | mall_change()
>  2 | -> mall_replace_hw_filter()
>  3 |   -> tcf_exts_validate_ex()
>  4 |      -> flags |= TCA_ACT_FLAGS_BIND;
>  5 |      -> tcf_action_init()
>  6 |         -> tcf_action_init_1()
>  7 |            -> a_o->init()
>  8 |               -> tcf_mirred_init()
>  9 |                  -> tcf_idr_create_from_flags()
> 10 |                     -> tcf_idr_create()
> 11 |                        -> p->tcfa_flags = flags;
> 12 |         -> tc_act_bind(flags))
> 13 |         -> tc_act_bind(act->tcfa_flags)
> 
> When invoked from tcf_exts_validate_ex() like matchall does (but other
> classifiers validate their extensions as well), tcf_action_init() runs
> in a call path where "flags" always contains TCA_ACT_FLAGS_BIND (set by
> line 4). So line 12 is always true, and line 13 is always true as well.
> No transition ever takes place, and the check is skipped.
> 
> The code was added in this form in commit c86e0209dc77 ("flow_offload:
> validate flags of filter and actions"), but I'm attributing the blame
> even earlier in that series, to when TCA_ACT_FLAGS_SKIP_HW and
> TCA_ACT_FLAGS_SKIP_SW were added to the UAPI.
> 
> Following the development process of this change, the check did not
> always exist in this form. A change took place between v3 [1] and v4 [2],
> AFAIU due to review feedback that it doesn't make sense for action flags
> to be different than classifier flags. I think I agree with that
> feedback, but it was translated into code that omits enforcing this for
> "classic" actions created at the same time with the filters themselves.
> 
> There are 3 more important cases to discuss. First there is this command:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1
> 
> which should be allowed, because prior to the concept of dedicated
> action flags, it used to work and it used to mean the action inherited
> the skip_sw/skip_hw flags from the classifier. It's not a mismatch.
> 
> Then we have this command:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1 skip_hw
> 
> where there is a mismatch and it should be rejected.
> 
> Finally, we have:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1 skip_sw
> 
> where the offload flags coincide, and this should be treated the same as
> the first command based on inheritance, and accepted.
> 
> [1]: https://lore.kernel.org/netdev/20211028110646.13791-9-simon.horman@corigine.com/
> [2]: https://lore.kernel.org/netdev/20211118130805.23897-10-simon.horman@corigine.com/
> Fixes: 7adc57651211 ("flow_offload: add skip_hw and skip_sw to control if offload the action")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Thanks Vladimir,

This looks like an oversight to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[Ido Schimmel]

On Thu, Oct 17, 2024 at 07:10:48PM +0300, Vladimir Oltean wrote:
> tcf_action_init() has logic for checking mismatches between action and
> filter offload flags (skip_sw/skip_hw). AFAIU, this is intended to run
> on the transition between the new tc_act_bind(flags) returning true (aka
> now gets bound to classifier) and tc_act_bind(act->tcfa_flags) returning
> false (aka action was not bound to classifier before). Otherwise, the
> check is skipped.
> 
> For the case where an action is not standalone, but rather it was
> created by a classifier and is bound to it, tcf_action_init() skips the
> check entirely, and this means it allows mismatched flags to occur.
> 
> Taking the matchall classifier code path as an example (with mirred as
> an action), the reason is the following:
> 
>  1 | mall_change()
>  2 | -> mall_replace_hw_filter()
>  3 |   -> tcf_exts_validate_ex()
>  4 |      -> flags |= TCA_ACT_FLAGS_BIND;
>  5 |      -> tcf_action_init()
>  6 |         -> tcf_action_init_1()
>  7 |            -> a_o->init()
>  8 |               -> tcf_mirred_init()
>  9 |                  -> tcf_idr_create_from_flags()
> 10 |                     -> tcf_idr_create()
> 11 |                        -> p->tcfa_flags = flags;
> 12 |         -> tc_act_bind(flags))
> 13 |         -> tc_act_bind(act->tcfa_flags)
> 
> When invoked from tcf_exts_validate_ex() like matchall does (but other
> classifiers validate their extensions as well), tcf_action_init() runs
> in a call path where "flags" always contains TCA_ACT_FLAGS_BIND (set by
> line 4). So line 12 is always true, and line 13 is always true as well.
> No transition ever takes place, and the check is skipped.
> 
> The code was added in this form in commit c86e0209dc77 ("flow_offload:
> validate flags of filter and actions"), but I'm attributing the blame
> even earlier in that series, to when TCA_ACT_FLAGS_SKIP_HW and
> TCA_ACT_FLAGS_SKIP_SW were added to the UAPI.
> 
> Following the development process of this change, the check did not
> always exist in this form. A change took place between v3 [1] and v4 [2],
> AFAIU due to review feedback that it doesn't make sense for action flags
> to be different than classifier flags. I think I agree with that
> feedback, but it was translated into code that omits enforcing this for
> "classic" actions created at the same time with the filters themselves.
> 
> There are 3 more important cases to discuss. First there is this command:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1
> 
> which should be allowed, because prior to the concept of dedicated
> action flags, it used to work and it used to mean the action inherited
> the skip_sw/skip_hw flags from the classifier. It's not a mismatch.
> 
> Then we have this command:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1 skip_hw
> 
> where there is a mismatch and it should be rejected.
> 
> Finally, we have:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1 skip_sw
> 
> where the offload flags coincide, and this should be treated the same as
> the first command based on inheritance, and accepted.
> 
> [1]: https://lore.kernel.org/netdev/20211028110646.13791-9-simon.horman@corigine.com/
> [2]: https://lore.kernel.org/netdev/20211118130805.23897-10-simon.horman@corigine.com/
> Fixes: 7adc57651211 ("flow_offload: add skip_hw and skip_sw to control if offload the action")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Tested-by: Ido Schimmel <idosch@nvidia.com>

Verified that after the patch the second case fails and the other two
pass.

Re: [PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Thu, 17 Oct 2024 19:10:48 +0300 you wrote:
> tcf_action_init() has logic for checking mismatches between action and
> filter offload flags (skip_sw/skip_hw). AFAIU, this is intended to run
> on the transition between the new tc_act_bind(flags) returning true (aka
> now gets bound to classifier) and tc_act_bind(act->tcfa_flags) returning
> false (aka action was not bound to classifier before). Otherwise, the
> check is skipped.
> 
> [...]

Here is the summary with links:
  - [net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers
    https://git.kernel.org/netdev/net/c/34d35b4edbbe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[Cong Wang]

On Thu, Oct 17, 2024 at 07:10:48PM +0300, Vladimir Oltean wrote:
> There are 3 more important cases to discuss. First there is this command:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1
> 
> which should be allowed, because prior to the concept of dedicated
> action flags, it used to work and it used to mean the action inherited
> the skip_sw/skip_hw flags from the classifier. It's not a mismatch.
> 
> Then we have this command:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1 skip_hw
> 
> where there is a mismatch and it should be rejected.
> 
> Finally, we have:
> 
> $ tc qdisc add dev eth0 clasct
> $ tc filter add dev eth0 ingress matchall skip_sw \
> 	action mirred ingress mirror dev eth1 skip_sw
> 
> where the offload flags coincide, and this should be treated the same as
> the first command based on inheritance, and accepted.
> 

Can we add some selftests to cover the above cases?

Thanks.

Re: [PATCH net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers

[Vladimir Oltean]

Hi Cong,

On Sat, Oct 26, 2024 at 10:03:18AM -0700, Cong Wang wrote:
> Can we add some selftests to cover the above cases?
> 
> Thanks.

I'm sorry, but I don't have the necessary time (either in a professional
or personal capacity) to write new tdc selftests for this.