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: Windows password security audit tool. GUI, reports in PDF.

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

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



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 )

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9481f2c142e26ee1174653d673e6134edd9851da..3a9e442fcaaf2ea48ae65bc87ee95f59cd7100c8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -51,7 +51,10 @@ struct qdisc_size_table {
 struct qdisc_skb_head {
        struct sk_buff  *head;
        struct sk_buff  *tail;
-       __u32           qlen;
+       union {
+               __u32           qlen;
+               atomic_t        atomic_qlen;
+       };
        spinlock_t      lock;
 };
 
@@ -408,27 +411,19 @@ static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
        BUILD_BUG_ON(sizeof(qcb->data) < sz);
 }
 
-static inline int qdisc_qlen_cpu(const struct Qdisc *q)
-{
-       return this_cpu_ptr(q->cpu_qstats)->qlen;
-}
-
 static inline int qdisc_qlen(const struct Qdisc *q)
 {
        return q->q.qlen;
 }
 
-static inline int qdisc_qlen_sum(const struct Qdisc *q)
+static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
 {
-       __u32 qlen = q->qstats.qlen;
-       int i;
+       u32 qlen = q->qstats.qlen;
 
-       if (q->flags & TCQ_F_NOLOCK) {
-               for_each_possible_cpu(i)
-                       qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen;
-       } else {
+       if (q->flags & TCQ_F_NOLOCK)
+               qlen += atomic_read(&q->q.atomic_qlen);
+       else
                qlen += q->q.qlen;
-       }
 
        return qlen;
 }
@@ -827,12 +822,12 @@ static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
 
 static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
 {
-       this_cpu_inc(sch->cpu_qstats->qlen);
+       atomic_inc(&sch->q.atomic_qlen);
 }
 
 static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
 {
-       this_cpu_dec(sch->cpu_qstats->qlen);
+       atomic_dec(&sch->q.atomic_qlen);
 }
 
 static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)