Message-ID: <CAM0EoMkrfFqjfUbEK5dSmTMj8sseO=w4SJsp=8mLDpMcER5eug@mail.gmail.com>
Date:   Wed, 18 Jan 2023 12:00:57 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Davide Caratti <dcaratti@...hat.com>
Cc:     Kyle Zeng <zengyhkyle@...il.com>,
        shaozhengchao <shaozhengchao@...wei.com>,
        David Miller <davem@...emloft.net>,
        Sasha Levin <sashal@...nel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: Question: Patch:("net: sched: cbq: dont intepret cls results when
 asked to drop") may be not bug for branch LTS 5.10

On Wed, Jan 18, 2023 at 11:00 AM Davide Caratti <dcaratti@...hat.com> wrote:
>
> all good points!
>
> On Wed, Jan 18, 2023 at 08:06:12AM -0500, Jamal Hadi Salim wrote:
>
> > The main issue is bigger than tcf_classify: It has to do with
> > interpretation of tcf_result and the return codes.
>
> Some classful qdiscs ignore the value of res.class and just lookup the
> configured classes based on the value of res.classid. In this case, the qdisc
> code validates tcf_results, and the resulting class is always a valid pointer
> with correct layout and size.

It is upto the qdisc implementation to decide how to operate. The
danger is always
in a hierarchy of calls any of which may go badly to assume that the
pointer is valid.
If we switch to returning proper error codes i think it will be easier
to relay such
information to the consumer of tcf_result.

> With HTB, CBQ, DRR, HFSC, QFQ and ATM it's possible that the TC classifier
> selects a traffic class for a given packet and writes that pointer in 'res.class'.

It's their choice of how to implement. I think actually cbq (which is
quiet a complex
scheduling algorithm) started this and the rest cutnpasted from it.

> When/if this happens, the qdisc doesn't need to use res.classid and then lookup for
> the traffic class: it assumes that res.class is already a good pointer to a
> struct of the correct type (struct hfsc_class for HFSC, for instance).
>

If all goes well, it is a good pointer.

> > The main environmental rule that was at stake here is the return
> > (verdict) code said to drop the packet. The validity of tcf_result in
> > such a case is questionable and setting it to 0 was irrelevant.
>
> I remember that the first matchall implementation forgot the assignment
> of 'res' and this caused a problem similar to the reported one [1]. In my opinion
> we should consider to eliminate 'res.class' and its usage in the above qdiscs,
> if we find out that current TC filters just write res.classid (not sure of what
> cls_bpf does, though) and constantly write 0 to res.class.
>
> [1] https://lore.kernel.org/netdev/b930159de5531a4d216a1cd2c2ef03aa41f421f9.1505562794.git.dcaratti@redhat.com/

Yikes. That should have been caught during the review.

> > The current return code is a "verdict" on what happened. Given that
> > there is potential to misinterpret - as was seen here - a safer approach is to get the
> > return code to be either an error/success code(eg ELOOP for the example being quoted) since
> > that is a more common pattern and we store the "verdict code" in tcf_result (TC_ACT_SHOT).
> > I was planning to send an RFC patch for that.
>
> well, the implementation of "goto_chain" actually abuses tcf_result:
> since it's going to pass the packet to another classifier, it
> temporarily stores a handle to the next filter in the tcf_result -
> instead of passing it through the stack. That is not a problem, unless
> a packet hits the max_reclassify_loop and a CBQ qdisc that dereferences
> 'res.class' even with a packet drop :)
>

The rule is tcf_results should be returning the results and execution
state to the
caller. The goto_chain maybe ok in this case. Also the mirred state
that i cant remember
who added was also reasonable if you wanted to defer redirect to the
caller instead
of returning ACT_STOLEN and friends.

[..]
> > On Wed, Jan 18, 2023 at 6:06 AM Davide Caratti <dcaratti@...hat.com> wrote:
> > >
> > >
> > > The merged patch looks good to me; however, I wonder if it's sufficient.
> > > If I well read the code, there is still the possibility of hitting the
> > > same problem on a patched kernel when TC_ACT_TRAP / TC_ACT_STOLEN is
> > > returned after a 'goto chain' when the qdisc is CBQ.
>
> I think it is sufficient to avoid the crash, since the value of 'res.class'
> should be a valid pointer (NULL most of the times?) after each call to
>
> tp->classify(skb, tp, res);
>
> if the return value is different than TC_ACT_UNSPEC and TC_ACT_SHOT.
> However, I still don't understand why cbq should charge classes when the
> packet has been 'stolen' or 'trapped'...

ACT_SHOT means (unfortunately ambiguous) "this is bad" - It means you
really cannot interpret the results.
That is not the case for the others.

BTW, I did create a patch initially when this issue surfaced but we
needed to get something
to net first. See attached. The proper way to do this is to have the
small surgery that
returns the errcode instead of verdict code and store TC_ACT_XXX in tcf_result
(in place of errcode).

cheers,
jamal

Download attachment "patchlet-fix-errcode-result" of type "application/octet-stream" (19147 bytes)

Powered by blists - more mailing lists