| Message ID | 20241017161049.3570037-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 34d35b4edbbe890a91bec939bfd29ad92517a52b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers | expand |
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>
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.
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!
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.
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.
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,
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(-)