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: <aGMD1S0F9sTXexBo@pop-os.localdomain>
Date: Mon, 30 Jun 2025 14:38:29 -0700
From: Cong Wang <xiyou.wangcong@...il.com>
To: Lion Ackermann <nnamrec@...il.com>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
	Jamal Hadi Salim <jhs@...atatu.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 03:27:30PM +0200, Lion Ackermann 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>

Acked-by: Cong Wang <xiyou.wangcong@...il.com>

> ---
>  net/sched/sch_api.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
 
I love to see fixing bugs by removing code. :)

Thanks.