[v1] net: sched: drr: dont intepret cls results when asked to drop

[PATCH] net: sched: drr: dont intepret cls results when asked to drop

Posted by Ma Ke 2 years, 4 months ago

If asked to drop a packet via TC_ACT_SHOT it is unsafe to
assume res.class contains a valid pointer.

Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
 net/sched/sch_drr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 19901e77cd3b..2b854cb6edf9 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	fl = rcu_dereference_bh(q->filter_list);
 	result = tcf_classify(skb, NULL, fl, &res, false);
+	if (result == TC_ACT_SHOT)
+		return NULL;
 	if (result >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-- 
2.37.2

Re: [PATCH] net: sched: drr: dont intepret cls results when asked to drop

Posted by Eric Dumazet 2 years, 4 months ago

On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote:
>
> If asked to drop a packet via TC_ACT_SHOT it is unsafe to
> assume res.class contains a valid pointer.
>
> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> ---
>  net/sched/sch_drr.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index 19901e77cd3b..2b854cb6edf9 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
>         *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>         fl = rcu_dereference_bh(q->filter_list);
>         result = tcf_classify(skb, NULL, fl, &res, false);
> +       if (result == TC_ACT_SHOT)
> +               return NULL;
>         if (result >= 0) {
>  #ifdef CONFIG_NET_CLS_ACT
>                 switch (result) {
> --
> 2.37.2
>

 I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine
at line 320 ?

Re: [PATCH] net: sched: drr: dont intepret cls results when asked to drop

Posted by Pedro Tammela 2 years, 4 months ago

On 15/09/2023 09:55, Eric Dumazet wrote:
> On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote:
>>
>> If asked to drop a packet via TC_ACT_SHOT it is unsafe to
>> assume res.class contains a valid pointer.
>>
>> Signed-off-by: Ma Ke <make_ruc2021@163.com>
>> ---
>>   net/sched/sch_drr.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
>> index 19901e77cd3b..2b854cb6edf9 100644
>> --- a/net/sched/sch_drr.c
>> +++ b/net/sched/sch_drr.c
>> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
>>          *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>>          fl = rcu_dereference_bh(q->filter_list);
>>          result = tcf_classify(skb, NULL, fl, &res, false);
>> +       if (result == TC_ACT_SHOT)
>> +               return NULL;
>>          if (result >= 0) {
>>   #ifdef CONFIG_NET_CLS_ACT
>>                  switch (result) {
>> --
>> 2.37.2
>>
> 
>   I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine
> at line 320 ?

Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks 
like there are a couple of places which return TC_ACT_SHOT before 
calling any classifiers, which then would cause some qdiscs to look into 
a uninitialized 'struct tcf_result res'.
I could be misreading it... But if it's the problem the author is trying 
to fix, the obvious way to do it would be:
	struct tcf_result res = {};

Re: [PATCH] net: sched: drr: dont intepret cls results when asked to drop

Posted by Eric Dumazet 2 years, 4 months ago

On Fri, Sep 15, 2023 at 5:03 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>
> On 15/09/2023 09:55, Eric Dumazet wrote:
> > On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote:
> >>
> >> If asked to drop a packet via TC_ACT_SHOT it is unsafe to
> >> assume res.class contains a valid pointer.
> >>
> >> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> >> ---
> >>   net/sched/sch_drr.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> >> index 19901e77cd3b..2b854cb6edf9 100644
> >> --- a/net/sched/sch_drr.c
> >> +++ b/net/sched/sch_drr.c
> >> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
> >>          *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> >>          fl = rcu_dereference_bh(q->filter_list);
> >>          result = tcf_classify(skb, NULL, fl, &res, false);
> >> +       if (result == TC_ACT_SHOT)
> >> +               return NULL;
> >>          if (result >= 0) {
> >>   #ifdef CONFIG_NET_CLS_ACT
> >>                  switch (result) {
> >> --
> >> 2.37.2
> >>
> >
> >   I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine
> > at line 320 ?
>
> Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks
> like there are a couple of places which return TC_ACT_SHOT before
> calling any classifiers, which then would cause some qdiscs to look into
> a uninitialized 'struct tcf_result res'.
> I could be misreading it... But if it's the problem the author is trying
> to fix, the obvious way to do it would be:
>         struct tcf_result res = {};

CONFIG_NET_CLS_ACT=n, how come TC_ACT_SHOT could be used ?

Can we get rid of CONFIG_NET_CLS_ACT, this seems obfuscation to me at
this point.

Re: [PATCH] net: sched: drr: dont intepret cls results when asked to drop

Posted by Jamal Hadi Salim 2 years, 4 months ago

On Fri, Sep 15, 2023 at 11:06 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Sep 15, 2023 at 5:03 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
> >
> > On 15/09/2023 09:55, Eric Dumazet wrote:
> > > On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote:
> > >>
> > >> If asked to drop a packet via TC_ACT_SHOT it is unsafe to
> > >> assume res.class contains a valid pointer.
> > >>
> > >> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> > >> ---
> > >>   net/sched/sch_drr.c | 2 ++
> > >>   1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> > >> index 19901e77cd3b..2b854cb6edf9 100644
> > >> --- a/net/sched/sch_drr.c
> > >> +++ b/net/sched/sch_drr.c
> > >> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
> > >>          *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
> > >>          fl = rcu_dereference_bh(q->filter_list);
> > >>          result = tcf_classify(skb, NULL, fl, &res, false);
> > >> +       if (result == TC_ACT_SHOT)
> > >> +               return NULL;
> > >>          if (result >= 0) {
> > >>   #ifdef CONFIG_NET_CLS_ACT
> > >>                  switch (result) {
> > >> --
> > >> 2.37.2
> > >>
> > >
> > >   I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine
> > > at line 320 ?
> >
> > Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks
> > like there are a couple of places which return TC_ACT_SHOT before
> > calling any classifiers, which then would cause some qdiscs to look into
> > a uninitialized 'struct tcf_result res'.
> > I could be misreading it... But if it's the problem the author is trying
> > to fix, the obvious way to do it would be:
> >         struct tcf_result res = {};
>
> CONFIG_NET_CLS_ACT=n, how come TC_ACT_SHOT could be used ?
>
> Can we get rid of CONFIG_NET_CLS_ACT, this seems obfuscation to me at
> this point.

The problem is the verdict vs return code are intermixed - not saying
this was fixing anything useful.
We discussed this in the past after/during commit
caa4b35b4317d5147b3ab0fbdc9c075c7d2e9c12
Victor worked on a patch to resolve that. Victor, maybe revive that
patch and post as RFC?


cheers,
jamal

Re: [PATCH] net: sched: drr: dont intepret cls results when asked to drop

Posted by Victor Nogueira 2 years, 4 months ago


On 15/09/2023 19:55, Jamal Hadi Salim wrote:
> On Fri, Sep 15, 2023 at 11:06 AM Eric Dumazet <edumazet@google.com> wrote:
>>
>> On Fri, Sep 15, 2023 at 5:03 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>>>
>>> On 15/09/2023 09:55, Eric Dumazet wrote:
>>>> On Fri, Sep 15, 2023 at 12:42 PM Ma Ke <make_ruc2021@163.com> wrote:
>>>>>
>>>>> If asked to drop a packet via TC_ACT_SHOT it is unsafe to
>>>>> assume res.class contains a valid pointer.
>>>>>
>>>>> Signed-off-by: Ma Ke <make_ruc2021@163.com>
>>>>> ---
>>>>>    net/sched/sch_drr.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
>>>>> index 19901e77cd3b..2b854cb6edf9 100644
>>>>> --- a/net/sched/sch_drr.c
>>>>> +++ b/net/sched/sch_drr.c
>>>>> @@ -309,6 +309,8 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
>>>>>           *qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>>>>>           fl = rcu_dereference_bh(q->filter_list);
>>>>>           result = tcf_classify(skb, NULL, fl, &res, false);
>>>>> +       if (result == TC_ACT_SHOT)
>>>>> +               return NULL;
>>>>>           if (result >= 0) {
>>>>>    #ifdef CONFIG_NET_CLS_ACT
>>>>>                   switch (result) {
>>>>> --
>>>>> 2.37.2
>>>>>
>>>>
>>>>    I do not see a bug, TC_ACT_SHOT is handled in the switch (result) just fine
>>>> at line 320 ?
>>>
>>> Following the code path (with CONFIG_NET_CLS_ACT=n in mind), it looks
>>> like there are a couple of places which return TC_ACT_SHOT before
>>> calling any classifiers, which then would cause some qdiscs to look into
>>> a uninitialized 'struct tcf_result res'.
>>> I could be misreading it... But if it's the problem the author is trying
>>> to fix, the obvious way to do it would be:
>>>          struct tcf_result res = {};
>>
>> CONFIG_NET_CLS_ACT=n, how come TC_ACT_SHOT could be used ?
>>
>> Can we get rid of CONFIG_NET_CLS_ACT, this seems obfuscation to me at
>> this point.
> 
> The problem is the verdict vs return code are intermixed - not saying
> this was fixing anything useful.
> We discussed this in the past after/during commit
> caa4b35b4317d5147b3ab0fbdc9c075c7d2e9c12
> Victor worked on a patch to resolve that. Victor, maybe revive that
> patch and post as RFC?

Okk, will review, rebase on top of net-next and post.

cheers,
Victor