net_sched: fix qdisc_tree_decrease_qlen() races

qdisc_tree_decrease_qlen() suffers from two problems on multiqueue
devices.

One problem is that it updates sch->q.qlen and sch->qstats.drops
on the mq/mqprio root qdisc, while it should not : Daniele
reported underflows errors :
[  681.774821] PAX: sch->q.qlen: 0 n: 1
[  681.774825] PAX: size overflow detected in function qdisc_tree_decrease_qlen net/sched/sch_api.c:769 cicus.693_49 min, count: 72, decl: qlen; num: 0; context: sk_buff_head;
[  681.774954] CPU: 2 PID: 19 Comm: ksoftirqd/2 Tainted: G           O    4.2.6.201511282239-1-grsec #1
[  681.774955] Hardware name: ASUSTeK COMPUTER INC. X302LJ/X302LJ, BIOS X302LJ.202 03/05/2015
[  681.774956]  ffffffffa9a04863 0000000000000000 0000000000000000 ffffffffa990ff7c
[  681.774959]  ffffc90000d3bc38 ffffffffa95d2810 0000000000000007 ffffffffa991002b
[  681.774960]  ffffc90000d3bc68 ffffffffa91a44f4 0000000000000001 0000000000000001
[  681.774962] Call Trace:
[  681.774967]  [<ffffffffa95d2810>] dump_stack+0x4c/0x7f
[  681.774970]  [<ffffffffa91a44f4>] report_size_overflow+0x34/0x50
[  681.774972]  [<ffffffffa94d17e2>] qdisc_tree_decrease_qlen+0x152/0x160
[  681.774976]  [<ffffffffc02694b1>] fq_codel_dequeue+0x7b1/0x820 [sch_fq_codel]
[  681.774978]  [<ffffffffc02680a0>] ? qdisc_peek_dequeued+0xa0/0xa0 [sch_fq_codel]
[  681.774980]  [<ffffffffa94cd92d>] __qdisc_run+0x4d/0x1d0
[  681.774983]  [<ffffffffa949b2b2>] net_tx_action+0xc2/0x160
[  681.774985]  [<ffffffffa90664c1>] __do_softirq+0xf1/0x200
[  681.774987]  [<ffffffffa90665ee>] run_ksoftirqd+0x1e/0x30
[  681.774989]  [<ffffffffa90896b0>] smpboot_thread_fn+0x150/0x260
[  681.774991]  [<ffffffffa9089560>] ? sort_range+0x40/0x40
[  681.774992]  [<ffffffffa9085fe4>] kthread+0xe4/0x100
[  681.774994]  [<ffffffffa9085f00>] ? kthread_worker_fn+0x170/0x170
[  681.774995]  [<ffffffffa95d8d1e>] ret_from_fork+0x3e/0x70

mq/mqprio have their own ways to report qlen/drops by folding stats on
all their queues, with appropriate locking.

A second problem is that qdisc_tree_decrease_qlen() calls qdisc_lookup()
without proper locking : concurrent qdisc updates could corrupt the list
that qdisc_match_from_root() parses to find a qdisc given its handle.

Fix first problem adding a TCQ_F_NOPARENT qdisc flag that
qdisc_tree_decrease_qlen() can use to abort its tree traversal,
as soon as it meets a mq/mqprio qdisc children.

Second problem can be fixed by RCU protection.
Qdisc are already freed after RCU grace period, so qdisc_list_add() and
qdisc_list_del() simply have to use appropriate rcu list variants.

A future patch will add a per struct netdev_queue list anchor, so that
qdisc_tree_decrease_qlen() can have more efficient lookups.

Reported-by: Daniele Fucini <dfucini@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Cong Wang <cwang@twopensource.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 4c79ce8c1f92f2d47eb87ffacd55fa01292e7378..b2a8e6338576d3e91f0906297e6b34e7db0eade3 100644 (file)
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -61,6 +61,9 @@ struct Qdisc {
                                      */
 #define TCQ_F_WARN_NONWC       (1 << 16)
 #define TCQ_F_CPUSTATS         0x20 /* run using percpu statistics */
+#define TCQ_F_NOPARENT         0x40 /* root of its hierarchy :
+                                     * qdisc_tree_decrease_qlen() should stop.
+                                     */
        u32                     limit;
        const struct Qdisc_ops  *ops;
        struct qdisc_size_table __rcu *stab;

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f43c8f33f09ef60e0f2a49ffd62f03624798c61b..7ec667dd4ce1d01deef3698af8c291da0722a657 100644 (file)
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -253,7 +253,8 @@ int qdisc_set_default(const char *name)
 }
 
 /* We know handle. Find qdisc among all qdisc's attached to device
-   (root qdisc, all its children, children of children etc.)
+ * (root qdisc, all its children, children of children etc.)
+ * Note: caller either uses rtnl or rcu_read_lock()
  */
 
 static struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle)
@@ -264,7 +265,7 @@ static struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle)
            root->handle == handle)
                return root;
 
-       list_for_each_entry(q, &root->list, list) {
+       list_for_each_entry_rcu(q, &root->list, list) {
                if (q->handle == handle)
                        return q;
        }
@@ -277,15 +278,18 @@ void qdisc_list_add(struct Qdisc *q)
                struct Qdisc *root = qdisc_dev(q)->qdisc;
 
                WARN_ON_ONCE(root == &noop_qdisc);
-               list_add_tail(&q->list, &root->list);
+               ASSERT_RTNL();
+               list_add_tail_rcu(&q->list, &root->list);
        }
 }
 EXPORT_SYMBOL(qdisc_list_add);
 
 void qdisc_list_del(struct Qdisc *q)
 {
-       if ((q->parent != TC_H_ROOT) && !(q->flags & TCQ_F_INGRESS))
-               list_del(&q->list);
+       if ((q->parent != TC_H_ROOT) && !(q->flags & TCQ_F_INGRESS)) {
+               ASSERT_RTNL();
+               list_del_rcu(&q->list);
+       }
 }
 EXPORT_SYMBOL(qdisc_list_del);
 
@@ -750,14 +754,18 @@ void qdisc_tree_decrease_qlen(struct Qdisc *sch, unsigned int n)
        if (n == 0)
                return;
        drops = max_t(int, n, 0);
+       rcu_read_lock();
        while ((parentid = sch->parent)) {
                if (TC_H_MAJ(parentid) == TC_H_MAJ(TC_H_INGRESS))
-                       return;
+                       break;
 
+               if (sch->flags & TCQ_F_NOPARENT)
+                       break;
+               /* TODO: perform the search on a per txq basis */
                sch = qdisc_lookup(qdisc_dev(sch), TC_H_MAJ(parentid));
                if (sch == NULL) {
-                       WARN_ON(parentid != TC_H_ROOT);
-                       return;
+                       WARN_ON_ONCE(parentid != TC_H_ROOT);
+                       break;
                }
                cops = sch->ops->cl_ops;
                if (cops->qlen_notify) {
@@ -768,6 +776,7 @@ void qdisc_tree_decrease_qlen(struct Qdisc *sch, unsigned int n)
                sch->q.qlen -= n;
                __qdisc_qstats_drop(sch, drops);
        }
+       rcu_read_unlock();
 }
 EXPORT_SYMBOL(qdisc_tree_decrease_qlen);
 
@@ -941,7 +950,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
                }
                lockdep_set_class(qdisc_lock(sch), &qdisc_tx_lock);
                if (!netif_is_multiqueue(dev))
-                       sch->flags |= TCQ_F_ONETXQUEUE;
+                       sch->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
        }
 
        sch->handle = handle;

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index cb5d4ad32946cf0c6f3391fe9742bceb7dd702e6..e82a1ad80aa521291fba4e704bb3828e88a009db 100644 (file)
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -737,7 +737,7 @@ static void attach_one_default_qdisc(struct net_device *dev,
                return;
        }
        if (!netif_is_multiqueue(dev))
-               qdisc->flags |= TCQ_F_ONETXQUEUE;
+               qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
        dev_queue->qdisc_sleeping = qdisc;
 }
 

diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index f3cbaecd283af4ce5a78ad90d135d008d2b542d7..3e82f047caaf40461c9f408bd0572a64147e1a8f 100644 (file)
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -63,7 +63,7 @@ static int mq_init(struct Qdisc *sch, struct nlattr *opt)
                if (qdisc == NULL)
                        goto err;
                priv->qdiscs[ntx] = qdisc;
-               qdisc->flags |= TCQ_F_ONETXQUEUE;
+               qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
        }
 
        sch->flags |= TCQ_F_MQROOT;
@@ -156,7 +156,7 @@ static int mq_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
 
        *old = dev_graft_qdisc(dev_queue, new);
        if (new)
-               new->flags |= TCQ_F_ONETXQUEUE;
+               new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
        if (dev->flags & IFF_UP)
                dev_activate(dev);
        return 0;

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 3811a745452cf402cd498ed47058fdf655d18cbd..ad70ecf57ce793d7b50b50e9220c24fa4ab30ab5 100644 (file)
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -132,7 +132,7 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt)
                        goto err;
                }
                priv->qdiscs[i] = qdisc;
-               qdisc->flags |= TCQ_F_ONETXQUEUE;
+               qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
        }
 
        /* If the mqprio options indicate that hardware should own
@@ -209,7 +209,7 @@ static int mqprio_graft(struct Qdisc *sch, unsigned long cl, struct Qdisc *new,
        *old = dev_graft_qdisc(dev_queue, new);
 
        if (new)
-               new->flags |= TCQ_F_ONETXQUEUE;
+               new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
 
        if (dev->flags & IFF_UP)
                dev_activate(dev);