Messages in this thread | | | Date | Fri, 15 Sep 2023 19:58:18 -0300 | Subject | Re: [PATCH] net: sched: drr: dont intepret cls results when asked to drop | From | Victor Nogueira <> |
| |
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
| |