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: <CAM0EoMngoh9hMr363XNiVxpKCu3Y+C4QkBmu0brYncx3YgPF=Q@mail.gmail.com>
Date: Tue, 1 Jul 2025 10:03:15 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: Lion Ackermann <nnamrec@...il.com>, netdev@...r.kernel.org, 
	Jiri Pirko <jiri@...nulli.us>, 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 5:38 PM Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> 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>
>

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

cheers,
jamal

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