Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
On 07/26/2018 09:43 AM, Jiri Pirko wrote:
> Wed, Jul 25, 2018 at 06:29:54PM CEST, dan...@iogearbox.net wrote:
>> On 07/25/2018 05:48 PM, Paolo Abeni wrote:
>>> On Wed, 2018-07-25 at 15:03 +0200, Jiri Pirko wrote:
Wed, Jul 25, 2018 at 02:54:04PM CEST, pab...@redhat.com wrote:
> On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote:
>> Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
>>> Only cls_bpf and act_bpf can safely use such value. If a generic
>>> action is configured by user space to return TC_ACT_REDIRECT,
>>> the usually visible behavior is passing the skb up the stack - as
>>> for unknown action, but, with complex configuration, more random
>>> results can be obtained.
>>>
>>> This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
>>> at action init time, making the kernel behavior more consistent.
>>>
>>> v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
>>>
>>> Signed-off-by: Paolo Abeni
>>> ---
>>> net/sched/act_api.c | 5 +
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>> index 148a89ab789b..24b5534967fe 100644
>>> --- a/net/sched/act_api.c
>>> +++ b/net/sched/act_api.c
>>> @@ -895,6 +895,11 @@ 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");
>>
>> Can't you push this warning through extack?
>>
>> But, wouldn't it be more appropriate to fail here? User is passing
>> invalid configuration
>
> Jiri, Jamal, thank you for the feedback.
>
> Please allow me to answer both of you here, since you raised similar
> concers.
>
> I thought about rejecting the action, but that change of behavior could
> break some users, as currently most kind of invalid tcfa_action values
> are simply accepted.
>
> If there is consensus about it, I can simply fail.
Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it
really has no meaning for anyone to use it throughout its whole history.
>>
>> That claim is completely wrong.
>
> Why? Does addition of TC_ACT_REDIRECT to uapi have any meaning?
BPF programs return TC_ACT_* as a verdict which is then further processed,
including TC_ACT_REDIRECT. Hence, it's a contract for them similarly as e.g.
helper enums (BPF_FUNC_*) and other things they make use of.
Cheers,
Daniel
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
Wed, Jul 25, 2018 at 06:29:54PM CEST, dan...@iogearbox.net wrote:
>On 07/25/2018 05:48 PM, Paolo Abeni wrote:
>> On Wed, 2018-07-25 at 15:03 +0200, Jiri Pirko wrote:
>>> Wed, Jul 25, 2018 at 02:54:04PM CEST, pab...@redhat.com wrote:
On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote:
> Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
>> Only cls_bpf and act_bpf can safely use such value. If a generic
>> action is configured by user space to return TC_ACT_REDIRECT,
>> the usually visible behavior is passing the skb up the stack - as
>> for unknown action, but, with complex configuration, more random
>> results can be obtained.
>>
>> This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
>> at action init time, making the kernel behavior more consistent.
>>
>> v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
>>
>> Signed-off-by: Paolo Abeni
>> ---
>> net/sched/act_api.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 148a89ab789b..24b5534967fe 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -895,6 +895,11 @@ 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");
>
> Can't you push this warning through extack?
>
> But, wouldn't it be more appropriate to fail here? User is passing
> invalid configuration
Jiri, Jamal, thank you for the feedback.
Please allow me to answer both of you here, since you raised similar
concers.
I thought about rejecting the action, but that change of behavior could
break some users, as currently most kind of invalid tcfa_action values
are simply accepted.
If there is consensus about it, I can simply fail.
>>>
>>> Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it
>>> really has no meaning for anyone to use it throughout its whole history.
>
>That claim is completely wrong.
Why? Does addition of TC_ACT_REDIRECT to uapi have any meaning?
>
>>> I would vote for "fail", yet I admit that I am usually alone in opinion
>>> about similar uapi changes :)
>>
>> Since even Jamal suggested the same, unless someone else voice some
>> opposition soon, in v4 I'll opt for rejecting actions using
>> TC_ACT_REDIRECT.
>
>You should probably leave out act_bpf from that rejection as there may be
>a small chance that users could potentially use it as default action.
>
>Thanks,
>Daniel
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
On 07/25/2018 05:48 PM, Paolo Abeni wrote:
> On Wed, 2018-07-25 at 15:03 +0200, Jiri Pirko wrote:
>> Wed, Jul 25, 2018 at 02:54:04PM CEST, pab...@redhat.com wrote:
>>> On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote:
Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
> Only cls_bpf and act_bpf can safely use such value. If a generic
> action is configured by user space to return TC_ACT_REDIRECT,
> the usually visible behavior is passing the skb up the stack - as
> for unknown action, but, with complex configuration, more random
> results can be obtained.
>
> This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
> at action init time, making the kernel behavior more consistent.
>
> v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
>
> Signed-off-by: Paolo Abeni
> ---
> net/sched/act_api.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 148a89ab789b..24b5534967fe 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -895,6 +895,11 @@ 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");
Can't you push this warning through extack?
But, wouldn't it be more appropriate to fail here? User is passing
invalid configuration
>>>
>>> Jiri, Jamal, thank you for the feedback.
>>>
>>> Please allow me to answer both of you here, since you raised similar
>>> concers.
>>>
>>> I thought about rejecting the action, but that change of behavior could
>>> break some users, as currently most kind of invalid tcfa_action values
>>> are simply accepted.
>>>
>>> If there is consensus about it, I can simply fail.
>>
>> Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it
>> really has no meaning for anyone to use it throughout its whole history.
That claim is completely wrong.
>> I would vote for "fail", yet I admit that I am usually alone in opinion
>> about similar uapi changes :)
>
> Since even Jamal suggested the same, unless someone else voice some
> opposition soon, in v4 I'll opt for rejecting actions using
> TC_ACT_REDIRECT.
You should probably leave out act_bpf from that rejection as there may be
a small chance that users could potentially use it as default action.
Thanks,
Daniel
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
On Wed, 2018-07-25 at 17:48 +0200, Paolo Abeni wrote: > On Wed, 2018-07-25 at 15:03 +0200, Jiri Pirko wrote: > > Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it > > really has no meaning for anyone to use it throughout its whole history. > > I would vote for "fail", yet I admit that I am usually alone in opinion > > about similar uapi changes :) > > Since even Jamal suggested the same, unless someone else voice some > opposition soon, in v4 I'll opt for rejecting actions using > TC_ACT_REDIRECT. Thinking again about it, I'm going to drop this patch from this series. Since v2 is not strictly needed anymore and actually quite unrelated. Thanks and sorry for the reiterated noise ;) Paolo
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
On Wed, 2018-07-25 at 15:03 +0200, Jiri Pirko wrote:
> Wed, Jul 25, 2018 at 02:54:04PM CEST, pab...@redhat.com wrote:
> > Hi,
> >
> > On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote:
> > > Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
> > > > Only cls_bpf and act_bpf can safely use such value. If a generic
> > > > action is configured by user space to return TC_ACT_REDIRECT,
> > > > the usually visible behavior is passing the skb up the stack - as
> > > > for unknown action, but, with complex configuration, more random
> > > > results can be obtained.
> > > >
> > > > This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
> > > > at action init time, making the kernel behavior more consistent.
> > > >
> > > > v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
> > > >
> > > > Signed-off-by: Paolo Abeni
> > > > ---
> > > > net/sched/act_api.c | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> > > > index 148a89ab789b..24b5534967fe 100644
> > > > --- a/net/sched/act_api.c
> > > > +++ b/net/sched/act_api.c
> > > > @@ -895,6 +895,11 @@ 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");
> > >
> > > Can't you push this warning through extack?
> > >
> > > But, wouldn't it be more appropriate to fail here? User is passing
> > > invalid configuration
> >
> > Jiri, Jamal, thank you for the feedback.
> >
> > Please allow me to answer both of you here, since you raised similar
> > concers.
> >
> > I thought about rejecting the action, but that change of behavior could
> > break some users, as currently most kind of invalid tcfa_action values
> > are simply accepted.
> >
> > If there is consensus about it, I can simply fail.
>
> Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it
> really has no meaning for anyone to use it throughout its whole history.
> I would vote for "fail", yet I admit that I am usually alone in opinion
> about similar uapi changes :)
Since even Jamal suggested the same, unless someone else voice some
opposition soon, in v4 I'll opt for rejecting actions using
TC_ACT_REDIRECT.
Thanks,
Paolo
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
Wed, Jul 25, 2018 at 02:54:04PM CEST, pab...@redhat.com wrote:
>Hi,
>
>On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote:
>> Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
>> > Only cls_bpf and act_bpf can safely use such value. If a generic
>> > action is configured by user space to return TC_ACT_REDIRECT,
>> > the usually visible behavior is passing the skb up the stack - as
>> > for unknown action, but, with complex configuration, more random
>> > results can be obtained.
>> >
>> > This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
>> > at action init time, making the kernel behavior more consistent.
>> >
>> > v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
>> >
>> > Signed-off-by: Paolo Abeni
>> > ---
>> > net/sched/act_api.c | 5 +
>> > 1 file changed, 5 insertions(+)
>> >
>> > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> > index 148a89ab789b..24b5534967fe 100644
>> > --- a/net/sched/act_api.c
>> > +++ b/net/sched/act_api.c
>> > @@ -895,6 +895,11 @@ 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");
>>
>> Can't you push this warning through extack?
>>
>> But, wouldn't it be more appropriate to fail here? User is passing
>> invalid configuration
>
>Jiri, Jamal, thank you for the feedback.
>
>Please allow me to answer both of you here, since you raised similar
>concers.
>
>I thought about rejecting the action, but that change of behavior could
>break some users, as currently most kind of invalid tcfa_action values
>are simply accepted.
>
>If there is consensus about it, I can simply fail.
Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it
really has no meaning for anyone to use it throughout its whole history.
I would vote for "fail", yet I admit that I am usually alone in opinion
about similar uapi changes :)
>
>Thanks,
>
>Paolo
>
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
Hi,
On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote:
> Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
> > Only cls_bpf and act_bpf can safely use such value. If a generic
> > action is configured by user space to return TC_ACT_REDIRECT,
> > the usually visible behavior is passing the skb up the stack - as
> > for unknown action, but, with complex configuration, more random
> > results can be obtained.
> >
> > This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
> > at action init time, making the kernel behavior more consistent.
> >
> > v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
> >
> > Signed-off-by: Paolo Abeni
> > ---
> > net/sched/act_api.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> > index 148a89ab789b..24b5534967fe 100644
> > --- a/net/sched/act_api.c
> > +++ b/net/sched/act_api.c
> > @@ -895,6 +895,11 @@ 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");
>
> Can't you push this warning through extack?
>
> But, wouldn't it be more appropriate to fail here? User is passing
> invalid configuration
Jiri, Jamal, thank you for the feedback.
Please allow me to answer both of you here, since you raised similar
concers.
I thought about rejecting the action, but that change of behavior could
break some users, as currently most kind of invalid tcfa_action values
are simply accepted.
If there is consensus about it, I can simply fail.
Thanks,
Paolo
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
Tue, Jul 24, 2018 at 10:06:39PM CEST, pab...@redhat.com wrote:
>Only cls_bpf and act_bpf can safely use such value. If a generic
>action is configured by user space to return TC_ACT_REDIRECT,
>the usually visible behavior is passing the skb up the stack - as
>for unknown action, but, with complex configuration, more random
>results can be obtained.
>
>This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
>at action init time, making the kernel behavior more consistent.
>
>v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
>
>Signed-off-by: Paolo Abeni
>---
> net/sched/act_api.c | 5 +
> 1 file changed, 5 insertions(+)
>
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 148a89ab789b..24b5534967fe 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -895,6 +895,11 @@ 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");
Can't you push this warning through extack?
But, wouldn't it be more appropriate to fail here? User is passing
invalid configuration
>+ a->tcfa_action = TC_ACT_UNSPEC;
>+ }
>+
> return a;
>
> err_mod:
>--
>2.17.1
>
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
On 24/07/18 04:06 PM, Paolo Abeni wrote:
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -895,6 +895,11 @@ 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;
+ }
+
Why not just reject the rule instead of changing the code underneath the
user?
cheers,
jamal
[PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
Only cls_bpf and act_bpf can safely use such value. If a generic
action is configured by user space to return TC_ACT_REDIRECT,
the usually visible behavior is passing the skb up the stack - as
for unknown action, but, with complex configuration, more random
results can be obtained.
This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC
at action init time, making the kernel behavior more consistent.
v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value
Signed-off-by: Paolo Abeni
---
net/sched/act_api.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 148a89ab789b..24b5534967fe 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -895,6 +895,11 @@ 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;
+ }
+
return a;
err_mod:
--
2.17.1
