| Message ID | 82c5852909788fef3c7b5c29d6ad8c90b60c7170.1532437050.git.pabeni@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | TC: refactor act_mirred packets re-injection | expand |
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
Tue, Jul 24, 2018 at 10:06:39PM CEST, pabeni@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 <pabeni@redhat.com> >--- > 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 >
Hi, On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote: > Tue, Jul 24, 2018 at 10:06:39PM CEST, pabeni@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 <pabeni@redhat.com> > > --- > > 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
Wed, Jul 25, 2018 at 02:54:04PM CEST, pabeni@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, pabeni@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 <pabeni@redhat.com> >> > --- >> > 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 >
On Wed, 2018-07-25 at 15:03 +0200, Jiri Pirko wrote: > Wed, Jul 25, 2018 at 02:54:04PM CEST, pabeni@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, pabeni@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 <pabeni@redhat.com> > > > > --- > > > > 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
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
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, pabeni@redhat.com wrote: >>> On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote: >>>> Tue, Jul 24, 2018 at 10:06:39PM CEST, pabeni@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 <pabeni@redhat.com> >>>>> --- >>>>> 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
Wed, Jul 25, 2018 at 06:29:54PM CEST, daniel@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, pabeni@redhat.com wrote: >>>> On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote: >>>>> Tue, Jul 24, 2018 at 10:06:39PM CEST, pabeni@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 <pabeni@redhat.com> >>>>>> --- >>>>>> 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
On 07/26/2018 09:43 AM, Jiri Pirko wrote: > Wed, Jul 25, 2018 at 06:29:54PM CEST, daniel@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, pabeni@redhat.com wrote: >>>>> On Wed, 2018-07-25 at 13:56 +0200, Jiri Pirko wrote: >>>>>> Tue, Jul 24, 2018 at 10:06:39PM CEST, pabeni@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 <pabeni@redhat.com> >>>>>>> --- >>>>>>> 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
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:
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 <pabeni@redhat.com> --- net/sched/act_api.c | 5 +++++ 1 file changed, 5 insertions(+)