netdev - Re: [BUG] net/sched : qlen can not really be per cpu ?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Feb 2019 18:46:47 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        Networking <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [BUG] net/sched : qlen can not really be per cpu ?

On Tue, Feb 26, 2019 at 4:56 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
>
>
>
> On 02/26/2019 03:51 PM, Cong Wang wrote:
> > On Tue, Feb 26, 2019 at 3:19 PM Eric Dumazet <eric.dumazet@...il.com> wrote:
> >>
> >>
> >>
> >> On 02/25/2019 10:42 PM, Eric Dumazet wrote:
> >>> HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
> >>>
> >>> WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
> >>>
> >>> This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
> >>> So cl->leaf.q->q.qlen is zero.
> >>>
> >>> HFSC, CBQ, DRR, QFQ  have the same problem.
> >>>
> >>> Any ideas how we can fix this ?
> >>
> >> What about something simple for stable ?
> >> ( I yet have to boot/test this )
> >
> > Is merely updating qlen sufficient for fixing it?
> >
> > I thought it is because of the lack of qdisc_tree_reduce_backlog()
> > in pfifo_fast.
>
> It does not seem to be the qdisc_tree_reduce_backlog() thing.
>
> HTB, HFSC, CBQ, DRR, QFQ only peek at their children 'qlen' to decide if there
> is at least one packet in them.
>
> The backlog is only reported for dumps, but the actual backlog value is not used in data path.
>

Hmm, looking into this, do we really need to check cl->leaf.q->q.qlen
in htb_activate() for pfifo_fast? htb_activate() is only called when
qdisc_enqueue() returns NET_XMIT_SUCCESS, so for pfifo_fast
that is always qlen!=0, right?

So something like below? It is ugly but should be sufficient to shut
up the warning.

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 30f9da7e1076..6d0182750f8b 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -555,7 +555,8 @@ htb_change_class_mode(struct htb_sched *q, struct
htb_class *cl, s64 *diff)
  */
 static inline void htb_activate(struct htb_sched *q, struct htb_class *cl)
 {
-       WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
+       WARN_ON(cl->level || !cl->leaf.q ||
+               (!(cl->leaf.q->flags & TCQ_F_NOLOCK) && !cl->leaf.q->q.qlen));

        if (!cl->prio_activity) {
                cl->prio_activity = 1 << cl->prio;