net/sched: Always pass notifications when child class becomes empty

Certain classful qdiscs may invoke their classes' dequeue handler on an
enqueue operation. This may unexpectedly empty the child qdisc and thus
make an in-flight class passive via qlen_notify(). Most qdiscs do not
expect such behaviour at this point in time and may re-activate the
class eventually anyways which will lead to a use-after-free.

The referenced fix commit attempted to fix this behavior for the HFSC
case by moving the backlog accounting around, though this turned out to
be incomplete since the parent's parent may run into the issue too.
The following reproducer demonstrates this use-after-free:

    tc qdisc add dev lo root handle 1: drr
    tc filter add dev lo parent 1: basic classid 1:1
    tc class add dev lo parent 1: classid 1:1 drr
    tc qdisc add dev lo parent 1:1 handle 2: hfsc def 1
    tc class add dev lo parent 2: classid 2:1 hfsc rt m1 8 d 1 m2 0
    tc qdisc add dev lo parent 2:1 handle 3: netem
    tc qdisc add dev lo parent 3:1 handle 4: blackhole

    echo 1 | socat -u STDIN UDP4-DATAGRAM:127.0.0.1:8888
    tc class delete dev lo classid 1:1
    echo 1 | socat -u STDIN UDP4-DATAGRAM:127.0.0.1:8888

Since backlog accounting issues leading to a use-after-frees on stale
class pointers is a recurring pattern at this point, this patch takes
a different approach. Instead of trying to fix the accounting, the patch
ensures that qdisc_tree_reduce_backlog always calls qlen_notify when
the child qdisc is empty. This solves the problem because deletion of
qdiscs always involves a call to qdisc_reset() and / or
qdisc_purge_queue() which ultimately resets its qlen to 0 thus causing
the following qdisc_tree_reduce_backlog() to report to the parent. Note
that this may call qlen_notify on passive classes multiple times. This
is not a problem after the recent patch series that made all the
classful qdiscs qlen_notify() handlers idempotent.

Fixes: 3f981138109f ("sch_hfsc: Fix qlen accounting bug when using peek in hfsc_enqueue()")
Signed-off-by: Lion Ackermann <nnamrec@gmail.com>
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Link: https://patch.msgid.link/d912cbd7-193b-4269-9857-525bee8bbb6a@gmail.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c5e3673aadbe78562977ee0915153406c088daef..d8a33486c511a479d63b5ebe1678c843ee82774b 100644 (file)
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -780,15 +780,12 @@ static u32 qdisc_alloc_handle(struct net_device *dev)
 
 void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
 {
-       bool qdisc_is_offloaded = sch->flags & TCQ_F_OFFLOADED;
        const struct Qdisc_class_ops *cops;
        unsigned long cl;
        u32 parentid;
        bool notify;
        int drops;
 
-       if (n == 0 && len == 0)
-               return;
        drops = max_t(int, n, 0);
        rcu_read_lock();
        while ((parentid = sch->parent)) {
@@ -797,17 +794,8 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
 
                if (sch->flags & TCQ_F_NOPARENT)
                        break;
-               /* Notify parent qdisc only if child qdisc becomes empty.
-                *
-                * If child was empty even before update then backlog
-                * counter is screwed and we skip notification because
-                * parent class is already passive.
-                *
-                * If the original child was offloaded then it is allowed
-                * to be seem as empty, so the parent is notified anyway.
-                */
-               notify = !sch->q.qlen && !WARN_ON_ONCE(!n &&
-                                                      !qdisc_is_offloaded);
+               /* Notify parent qdisc only if child qdisc becomes empty. */
+               notify = !sch->q.qlen;
                /* TODO: perform the search on a per txq basis */
                sch = qdisc_lookup_rcu(qdisc_dev(sch), TC_H_MAJ(parentid));
                if (sch == NULL) {
@@ -816,6 +804,9 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
                }
                cops = sch->ops->cl_ops;
                if (notify && cops->qlen_notify) {
+                       /* Note that qlen_notify must be idempotent as it may get called
+                        * multiple times.
+                        */
                        cl = cops->find(sch, parentid);
                        cops->qlen_notify(sch, cl);
                }