From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; dkim=pass header.d=toke.dk; arc=none (Message is not ARC signed); dmarc=pass (Used From Domain Record) header.from=toke.dk policy.dmarc=reject From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=toke.dk; s=20161023; t=1762858711; bh=SGEtQRTXbaAjEO3Xhru3pRr1LyZx7uhVks4PV8yKfv4=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=qHQb63ujOCT8xknOAUaVDchjeviKs6RZiVO0LvgZhVHlfrKTF55n6+/aieLq+ow+w APXzL+H4qFUJGUTGEKMU0TuHbNwNQQawf5dvdAuVAI2rEzJTR0qqC9bzqSP7HyDL0H EC1vfQPpGdfyApOejNlF3JqIOT2zFGbo5hHFLbBLrHue6psYPbr81r4lGq8XJhkPtK 8KARxHzk3djGwpr3Dhz80kVs238yeBIo73bKwvmq8ul9roLvimL4EttiZccqEjZ9fE k4RQ61bJUVoNWXsrzob5Jw8u+hmxnf9s5e50lA75ViOTR8ZFNwe06l06IuD50bt7xB HXh9uSkdprIpA== To: Xiang Mei , security@kernel.org Cc: cake@lists.bufferbloat.net, bestswngs@gmail.com, Xiang Mei In-Reply-To: <20251111072709.336809-1-xmei5@asu.edu> References: <20251111072709.336809-1-xmei5@asu.edu> Date: Tue, 11 Nov 2025 11:58:28 +0100 X-Clacks-Overhead: GNU Terry Pratchett Message-ID: <878qgcn8dn.fsf@toke.dk> MIME-Version: 1.0 Content-Type: text/plain Message-ID-Hash: SCRJCBEPBPHPDCHC5D4I5LQDT773WCX6 X-Message-ID-Hash: SCRJCBEPBPHPDCHC5D4I5LQDT773WCX6 X-MailFrom: toke@toke.dk X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list Subject: [Cake] Re: [PATCH V2 RESEND] net/sched: sch_cake: Fix incorrect qlen reduction in cake_drop List-Id: Cake - FQ_codel the next generation Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Xiang Mei writes: > In cake_drop(), the function qdisc_tree_reduce_backlog() is called to > decrement the qlen of the qdisc hierarchy. However, it may incorrectly > reduce the qlen when the dropped packet was not enqueued, leading to > a potential null-pointer dereference (e.g., when using qfq sched as > the parent qdisc). > > This issue occurs when the caller (cake_enqueue()) returns NET_XMIT_CN, > causing the parent qdisc not to enqueue the current packet, while > qdisc_tree_reduce_backlog() still decrements the backlog. > > This patch prevents invalid qlen reduction by verifying that the > dropped packet was actually enqueued before adjusting the backlog. > > Fixes: 15de71d06a40 ("net/sched: Make cake_enqueue return NET_XMIT_CN when past buffer_limit") > Signed-off-by: Xiang Mei Thank you for the patch! I think the issue is valid, but I don't think the fix is quite right - see below. Also, this should probably go through netdev? Please include netdev@vger.kernel.org and add a 'net' tag to the patch subject when you respin (so it'll look like [PATCH net v3]). > --- > v2: add missing cc > > net/sched/sch_cake.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c > index 32bacfc314c2..3a2ba9dfc22d 100644 > --- a/net/sched/sch_cake.c > +++ b/net/sched/sch_cake.c > @@ -1548,7 +1548,7 @@ static int cake_advance_shaper(struct cake_sched_data *q, > return len; > } > > -static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free) > +static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free, u32 current_flow) > { > struct cake_sched_data *q = qdisc_priv(sch); > ktime_t now = ktime_get(); > @@ -1597,7 +1597,8 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free) > > qdisc_drop_reason(skb, sch, to_free, SKB_DROP_REASON_QDISC_OVERLIMIT); > sch->q.qlen--; > - qdisc_tree_reduce_backlog(sch, 1, len); > + if (likely(current_flow != idx + (tin << 16))) > + qdisc_tree_reduce_backlog(sch, 1, len); Since cake_drop() is called in a loop, the number of bytes/packets dropped may not match the packet coming in. So we can't just skip this. I think the simplest would probably be to move the qdisc_tree_reduce_backlog() out of cake_drop entirely, and then replicate the logic from fq_codel in the caller. I.e., the bit where it saves the qlen and backlog before dropping and calls qdisc_tree_reduce_backlog() once after. So we'll end up with something like: prev_backlog = sch->qstats.backlog; prev_qlen = sch->q.qlen; while (q->buffer_used > q->buffer_limit) { /*...*/ } prev_qlen -= sch->q.qlen; prev_backlog -= sch->qstats.backlog; if (same_flow) qdisc_tree_reduce_backlog(sch, prev_qlen - 1, prev_backlog - pkt_len); return NET_XMIT_CN; } qdisc_tree_reduce_backlog(sch, prev_qlen,b prev_backlog); /* etc */ -Toke