Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On 31/07/18 10:40 AM, Paolo Abeni wrote: If we choose to reject unknown opcodes, such user-space configuration will fail. I think that is a good thing. The kernel should not be accepting things it doesnt understand. This is a good opportunity to enforce that. What would happen before this patch is that configurations using such TC_ACT_ value would be successful. This is why I proposed to keep the fixup. Note: Such behavior can only occur if tc(user space) allows you to pass illegitimate values which today can only happen when you have a new user space but older kernel (with "old" starting with your current changes). iow, fixing a policy in a kernel which has no support for TC_ACT_ to translate intent to be TC_ACT_OK/PIPE is problematic (as i was showing earlier). I initially thought the kernel behavior in the above scenario would match exactly TC_ACT_UNSPEC processing, but as you noted with the example in your previous email, TC_ACT_UNSPEC processing is actually a bit different. I worry: I dont think we can get a good default for most use cases. No point in maintaining faulty expectations (because IMO: the user will - eventually - fix their scripts if they dont see expected behavior). cheers, jamal
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On Tue, 2018-07-31 at 09:53 -0400, Jamal Hadi Salim wrote: > BTW, I asked this earlier and Jiri said it was addressed in patch 2. > I just looked again and i may be missing something basic: > Lets say tomorrow in a new kernel we add new TC_ACT_XXX that then gets > exposed to uapi - so user space tc is updated. > You then use the new tc specifying TC_ACT_XXX policy on kernel with your > changes. > If i read correctly because TC_ACT_XXX is out of bounds for current > kernel(which has your changes) you will fix it to be UNSPEC, no? You are right. If we choose to reject unknown opcodes, such user-space configuration will fail. What would happen before this patch is that configurations using such TC_ACT_ value would be successful. This is why I proposed to keep the fixup. I initially thought the kernel behavior in the above scenario would match exactly TC_ACT_UNSPEC processing, but as you noted with the example in your previous email, TC_ACT_UNSPEC processing is actually a bit different. Cheers, Paolo
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On 31/07/18 05:41 AM, Paolo Abeni wrote: Before this patch, the kernel exposed the same behaviour for negative value of 'bar', while, for positive 'bar' values, the overall behaviour was more complex (some classifier always stops with unknown positive action value, others go to lower prio). > Overall, the kernel behavior should be more well-defined now, but yes, there is a change of behavior under some circumstances. What about instead mapping undefined/unknown actions value to TC_ACT_OK (still at initialization time)? > this is what is already done by tcf_action_exec() for faulty opcodes/graphs and by tcf_ipt() and would handle the above example more conistently. I think PIPE maybe more reasonable. You still continue the action graph even on such a mis-configuration. But let me see if i can make a convincing arguement for rejecting at init time: I would be _very suprised_ if someone is depending on a misconfiguration such as this in a deployment because they would get different results than what they are expecting and sooner or later fix it after a lot of debugging and cursing. Your patch helps them notice it sooner. And a rejection even much much sooner. With a rejection you dont get to execute a "fixup" the kernel assumes for you. BTW, I asked this earlier and Jiri said it was addressed in patch 2. I just looked again and i may be missing something basic: Lets say tomorrow in a new kernel we add new TC_ACT_XXX that then gets exposed to uapi - so user space tc is updated. You then use the new tc specifying TC_ACT_XXX policy on kernel with your changes. If i read correctly because TC_ACT_XXX is out of bounds for current kernel(which has your changes) you will fix it to be UNSPEC, no? cheers, jamal
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On Mon, 2018-07-30 at 15:31 -0400, Jamal Hadi Salim wrote:
> On 30/07/18 12:41 PM, Paolo Abeni wrote:
> > On Mon, 2018-07-30 at 10:03 -0400, Jamal Hadi Salim wrote:
> > > On 30/07/18 08:30 AM, Paolo Abeni wrote:
> > > > }
> > > >
> > > > + if (!tcf_action_valid(a->tcfa_action)) {
> > > > + NL_SET_ERR_MSG(extack, "invalid action value, using
> > > > TC_ACT_UNSPEC instead");
> > > > + a->tcfa_action = TC_ACT_UNSPEC;
> > > > + }
> > > > +
> > > > return a;
> > > >
> > >
> > >
> > > I think it would make a lot more sense to just reject the entry than
> > > changing it underneath the user to a default value. Least element of
> > > suprise.
> >
> > I fear that would break existing (bad) users ?!? This way, such users
> > are notified they are doing something uncorrect, but still continue to
> > work.
>
>
> By "bad users" I think you mean someone setting a policy expecting
> one behavior but getting a different one?
Generally speaking I thought about user-space pushing to the kernel
some uninitialized/random value for 'tcfa_action'.
> If yes, that policy was
> already wrong/buggy. As an example, if i configured:
>
> match xxx action foo action goo action bar action gah
>
> where action goo has a bad opcode
> If you "fix it" with TC_ACT_UNSPEC then basically the above
> policy is now equivalent to:
>
> match xxx action foo action goo
>
> Infact if there was a lower prio rule in the chain
> then lookup will continue there and produce even stranger
> results.
I see.
Before this patch, the kernel exposed the same behaviour for negative
value of 'bar', while, for positive 'bar' values, the overall behaviour
was more complex (some classifier always stops with unknown positive
action value, others go to lower prio).
Overall, the kernel behavior should be more well-defined now, but yes,
there is a change of behavior under some circumstances.
What about instead mapping undefined/unknown actions value to TC_ACT_OK
(still at initialization time)? this is what is already done by
tcf_action_exec() for faulty opcodes/graphs and by tcf_ipt() and would
handle the above example more conistently.
Cheers,
Paolo
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On 30/07/18 12:41 PM, Paolo Abeni wrote:
On Mon, 2018-07-30 at 10:03 -0400, Jamal Hadi Salim wrote:
On 30/07/18 08:30 AM, Paolo Abeni wrote:
}
+ if (!tcf_action_valid(a->tcfa_action)) {
+ NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC
instead");
+ a->tcfa_action = TC_ACT_UNSPEC;
+ }
+
return a;
I think it would make a lot more sense to just reject the entry than
changing it underneath the user to a default value. Least element of
suprise.
I fear that would break existing (bad) users ?!? This way, such users
are notified they are doing something uncorrect, but still continue to
work.
By "bad users" I think you mean someone setting a policy expecting
one behavior but getting a different one? If yes, that policy was
already wrong/buggy. As an example, if i configured:
match xxx action foo action goo action bar action gah
where action goo has a bad opcode
If you "fix it" with TC_ACT_UNSPEC then basically the above
policy is now equivalent to:
match xxx action foo action goo
Infact if there was a lower prio rule in the chain
then lookup will continue there and produce even stranger
results.
cheers,
jamal
The patch can be changed to reject bad actions, if there is agreement,
but it would not look as the safest way to me.
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On Mon, 2018-07-30 at 10:03 -0400, Jamal Hadi Salim wrote:
> On 30/07/18 08:30 AM, Paolo Abeni wrote:
> > }
> >
> > + if (!tcf_action_valid(a->tcfa_action)) {
> > + NL_SET_ERR_MSG(extack, "invalid action value, using
> > TC_ACT_UNSPEC instead");
> > + a->tcfa_action = TC_ACT_UNSPEC;
> > + }
> > +
> > return a;
> >
>
>
> I think it would make a lot more sense to just reject the entry than
> changing it underneath the user to a default value. Least element of
> suprise.
I fear that would break existing (bad) users ?!? This way, such users
are notified they are doing something uncorrect, but still continue to
work.
The patch can be changed to reject bad actions, if there is agreement,
but it would not look as the safest way to me.
Thanks,
Paolo
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
Mon, Jul 30, 2018 at 04:03:50PM CEST, j...@mojatatu.com wrote:
>On 30/07/18 08:30 AM, Paolo Abeni wrote:
>> }
>> +if (!tcf_action_valid(a->tcfa_action)) {
>> +NL_SET_ERR_MSG(extack, "invalid action value, using
>> TC_ACT_UNSPEC instead");
>> +a->tcfa_action = TC_ACT_UNSPEC;
>> +}
>> +
>> return a;
>
>
>I think it would make a lot more sense to just reject the entry than
>changing it underneath the user to a default value. Least element of
>suprise.
It might break existing user who is incorrectly doing it. But I'm okay
with both solutions.
>
>cheers,
>jamal
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
On 30/07/18 08:30 AM, Paolo Abeni wrote:
}
+ if (!tcf_action_valid(a->tcfa_action)) {
+ NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC
instead");
+ a->tcfa_action = TC_ACT_UNSPEC;
+ }
+
return a;
I think it would make a lot more sense to just reject the entry than
changing it underneath the user to a default value. Least element of
suprise.
cheers,
jamal
Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
Mon, Jul 30, 2018 at 02:30:42PM CEST, pab...@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. > >v3 -> v4: > - use an helper to check for action validity (JiriP) > - emit an extack for invalid actions (JiriP) >v4 -> v5: > - keep messages on a single line, drop net_warn (Marcelo) Little nitpick: The changelog should be in the reverse order. > >Signed-off-by: Paolo Abeni Acked-by: Jiri Pirko
[PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values
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.
v3 -> v4:
- use an helper to check for action validity (JiriP)
- emit an extack for invalid actions (JiriP)
v4 -> v5:
- keep messages on a single line, drop net_warn (Marcelo)
Signed-off-by: Paolo Abeni
---
include/uapi/linux/pkt_cls.h | 6 --
net/sched/act_api.c | 14 ++
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index b4512254036b..48e5b5d49a34 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 b43df1e25c6d..229d63c99be2 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -786,6 +786,15 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr
**tb)
return c;
}
+static bool tcf_action_valid(int action)
+{
+ int opcode = TC_ACT_EXT_OPCODE(action);
+
+ if (!opcode)
+ return action <= TC_ACT_VALUE_MAX;
+ return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
+}
+
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
@@ -895,6 +904,11 @@ struct tc_action *tcf_action_init_1(struct net *net,
struct tcf_proto *tp,
}
}
+ if (!tcf_action_valid(a->tcfa_action)) {
+ NL_SET_ERR_MSG(extack, "invalid action value, using
TC_ACT_UNSPEC instead");
+ a->tcfa_action = TC_ACT_UNSPEC;
+ }
+
return a;
err_mod:
--
2.17.1
