netdev - Re: [PATCH] net/sched: Always pass notifications when child class becomes empty

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]

Message-ID: <CAM0EoMnDSJTWuxw6P3=oOAjjOe9Pbn6FbCZmByx3B9qfPceVeg@mail.gmail.com>
Date: Mon, 30 Jun 2025 10:56:55 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Lion Ackermann <nnamrec@...il.com>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>, 
	Cong Wang <xiyou.wangcong@...il.com>, Victor Nogueira <victor@...atatu.com>, 
	Mingi Cho <mincho@...ori.io>
Subject: Re: [PATCH] net/sched: Always pass notifications when child class
 becomes empty

On Mon, Jun 30, 2025 at 9:27 AM Lion Ackermann <nnamrec@...il.com> wrote:
>
> 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@...il.com>

Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>

cheers,
jamal
> ---
>  net/sched/sch_api.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index c5e3673aadbe..d8a33486c511 100644
> --- 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);
>                 }
> --
> 2.49.0
>
>