mirror of
https://github.com/torvalds/linux.git
synced 2024-11-21 19:46:16 +00:00
net/sched: act_api: deny mismatched skip_sw/skip_hw flags for actions created by classifiers
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 commitc86e0209dc
("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:7adc576512
("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: Simon Horman <horms@kernel.org> Reviewed-by: Ido Schimmel <idosch@nvidia.com> Tested-by: Ido Schimmel <idosch@nvidia.com> Link: https://patch.msgid.link/20241017161049.3570037-1-vladimir.oltean@nxp.com Signed-off-by: Paolo Abeni <pabeni@redhat.com>
This commit is contained in:
parent
8a7d12d674
commit
34d35b4edb
@ -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,
|
||||
|
Loading…
Reference in New Issue
Block a user