netdev - Re: [Patch net v2 1/2] net_sched: Flush gso

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: <9ad2d46f-7746-45e3-b5c3-e53d079d1b8e@redhat.com>
Date: Thu, 8 May 2025 12:26:32 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc: jiri@...nulli.us, jhs@...atatu.com, willsroot@...tonmail.com,
 savy@...t3mfailure.io
Subject: Re: [Patch net v2 1/2] net_sched: Flush gso_skb list too during
 ->change()

On 5/7/25 6:35 AM, Cong Wang wrote:
> Previously, when reducing a qdisc's limit via the ->change() operation, only
> the main skb queue was trimmed, potentially leaving packets in the gso_skb
> list. This could result in NULL pointer dereference when we only check
> sch->limit against sch->q.qlen.
> 
> This patch introduces a new helper, qdisc_dequeue_internal(), which ensures
> both the gso_skb list and the main queue are properly flushed when trimming
> excess packets. All relevant qdiscs (codel, fq, fq_codel, fq_pie, hhf, pie)
> are updated to use this helper in their ->change() routines.

A side effect is that now queue reduction will incur in an indirect call
per packet, but that should be irrelevant as it happens only in the
control path.

> Fixes: 76e3cc126bb2 ("codel: Controlled Delay AQM")
> Fixes: 4b549a2ef4be ("fq_codel: Fair Queue Codel AQM")
> Fixes: afe4fd062416 ("pkt_sched: fq: Fair Queue packet scheduler")
> Fixes: ec97ecf1ebe4 ("net: sched: add Flow Queue PIE packet scheduler")
> Fixes: 10239edf86f1 ("net-qdisc-hhf: Heavy-Hitter Filter (HHF) qdisc")
> Fixes: d4b36210c2e6 ("net: pkt_sched: PIE AQM scheme")
> Reported-by: Will <willsroot@...tonmail.com>
> Reported-by: Savy <savy@...t3mfailure.io>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>

LGTM, but it would be great if any of the reporters could explicitly
test it.

> diff --git a/net/sched/sch_codel.c b/net/sched/sch_codel.c
> index 12dd71139da3..c93761040c6e 100644
> --- a/net/sched/sch_codel.c
> +++ b/net/sched/sch_codel.c
> @@ -144,7 +144,7 @@ static int codel_change(struct Qdisc *sch, struct nlattr *opt,
>  
>  	qlen = sch->q.qlen;
>  	while (sch->q.qlen > sch->limit) {
> -		struct sk_buff *skb = __qdisc_dequeue_head(&sch->q);
> +		struct sk_buff *skb = qdisc_dequeue_internal(sch, true);
>  
>  		dropped += qdisc_pkt_len(skb);
>  		qdisc_qstats_backlog_dec(sch, skb);

Side note: it looks like there is room for a possible net-next follow-up
de-duplicating this loop code from most of the involved qdisc via a
shared helper.

Thanks,

Paolo