| Message ID | 5710761e4915690523906201bcb30987162a4fd2.1532437050.git.pabeni@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | TC: refactor act_mirred packets re-injection | expand |
Tue, Jul 24, 2018 at 10:06:40PM CEST, pabeni@redhat.com wrote: >Currently, when initializing an action, the user-space can specify >and use arbitrary values for the tcfa_action field. If the value >is unknown by the kernel, is implicitly threaded as TC_ACT_UNSPEC. > >This change explicitly checks for unknown values at action creation >time, and explicitly convert them to TC_ACT_UNSPEC. No functional >changes are introduced, but this will allow introducing tcfa_action >values not exposed to user-space in a later patch. > >Note: we can't use the above to hide TC_ACT_REDIRECT from user-space, >as the latter is already part of uAPI. > >Signed-off-by: Paolo Abeni <pabeni@redhat.com> >--- > include/uapi/linux/pkt_cls.h | 6 ++++-- > net/sched/act_api.c | 10 +++++++++- > 2 files changed, 13 insertions(+), 3 deletions(-) > >diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h >index c4262d911596..c8a24861d4c8 100644 >--- a/include/uapi/linux/pkt_cls.h >+++ b/include/uapi/linux/pkt_cls.h >@@ -45,6 +45,7 @@ enum { > * the skb and act like everything > * is alright. > */ >+#define TC_ACT_VALUE_MAX TC_ACT_TRAP > > /* There is a special kind of actions called "extended actions", > * which need a value parameter. These have a local opcode located in >@@ -55,11 +56,12 @@ enum { > #define __TC_ACT_EXT_SHIFT 28 > #define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT) > #define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1) >-#define TC_ACT_EXT_CMP(combined, opcode) \ >- (((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode) >+#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK)) >+#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode) > > #define TC_ACT_JUMP __TC_ACT_EXT(1) > #define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2) >+#define TC_ACT_EXT_OPCODE_MAX TC_ACT_GOTO_CHAIN > > /* Action type identifiers*/ > enum { >diff --git a/net/sched/act_api.c b/net/sched/act_api.c >index 24b5534967fe..5044f4809b37 100644 >--- a/net/sched/act_api.c >+++ b/net/sched/act_api.c >@@ -798,6 +798,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > char act_name[IFNAMSIZ]; > struct nlattr *tb[TCA_ACT_MAX + 1]; > struct nlattr *kind; >+ int opcode; > int err; > > if (name == NULL) { >@@ -884,7 +885,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > if (err != ACT_P_CREATED) > module_put(a_o->owner); > >- if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) { >+ opcode = TC_ACT_EXT_OPCODE(a->tcfa_action); >+ if (opcode == TC_ACT_GOTO_CHAIN) { > err = tcf_action_goto_chain_init(a, tp); > if (err) { > struct tc_action *actions[] = { a, NULL }; >@@ -898,6 +900,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > if (a->tcfa_action == TC_ACT_REDIRECT) { > net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly"); > a->tcfa_action = TC_ACT_UNSPEC; >+ } else if ((!opcode && a->tcfa_action > TC_ACT_VALUE_MAX) || >+ (opcode && opcode > TC_ACT_EXT_OPCODE_MAX && >+ a->tcfa_action != TC_ACT_UNSPEC)) { >+ net_warn_ratelimited("invalid %d action value", >+ a->tcfa_action); >+ a->tcfa_action = TC_ACT_UNSPEC; Maybe this could be a separate helper function? Also, the warn might go along with extack to user too. Otherwise, this looks fine to me. > } > > return a; >-- >2.17.1 >
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index c4262d911596..c8a24861d4c8 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -45,6 +45,7 @@ enum { * the skb and act like everything * is alright. */ +#define TC_ACT_VALUE_MAX TC_ACT_TRAP /* There is a special kind of actions called "extended actions", * which need a value parameter. These have a local opcode located in @@ -55,11 +56,12 @@ enum { #define __TC_ACT_EXT_SHIFT 28 #define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT) #define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1) -#define TC_ACT_EXT_CMP(combined, opcode) \ - (((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode) +#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK)) +#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode) #define TC_ACT_JUMP __TC_ACT_EXT(1) #define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2) +#define TC_ACT_EXT_OPCODE_MAX TC_ACT_GOTO_CHAIN /* Action type identifiers*/ enum { diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 24b5534967fe..5044f4809b37 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -798,6 +798,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, char act_name[IFNAMSIZ]; struct nlattr *tb[TCA_ACT_MAX + 1]; struct nlattr *kind; + int opcode; int err; if (name == NULL) { @@ -884,7 +885,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, if (err != ACT_P_CREATED) module_put(a_o->owner); - if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) { + opcode = TC_ACT_EXT_OPCODE(a->tcfa_action); + if (opcode == TC_ACT_GOTO_CHAIN) { err = tcf_action_goto_chain_init(a, tp); if (err) { struct tc_action *actions[] = { a, NULL }; @@ -898,6 +900,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, if (a->tcfa_action == TC_ACT_REDIRECT) { net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly"); a->tcfa_action = TC_ACT_UNSPEC; + } else if ((!opcode && a->tcfa_action > TC_ACT_VALUE_MAX) || + (opcode && opcode > TC_ACT_EXT_OPCODE_MAX && + a->tcfa_action != TC_ACT_UNSPEC)) { + net_warn_ratelimited("invalid %d action value", + a->tcfa_action); + a->tcfa_action = TC_ACT_UNSPEC; } return a;
Currently, when initializing an action, the user-space can specify and use arbitrary values for the tcfa_action field. If the value is unknown by the kernel, is implicitly threaded as TC_ACT_UNSPEC. This change explicitly checks for unknown values at action creation time, and explicitly convert them to TC_ACT_UNSPEC. No functional changes are introduced, but this will allow introducing tcfa_action values not exposed to user-space in a later patch. Note: we can't use the above to hide TC_ACT_REDIRECT from user-space, as the latter is already part of uAPI. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- include/uapi/linux/pkt_cls.h | 6 ++++-- net/sched/act_api.c | 10 +++++++++- 2 files changed, 13 insertions(+), 3 deletions(-)