netdev - Re: [PATCH net v3] net/sched: sch_cake: Fix incorrect qlen reduction in cake

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: <aRhP4E7g5yJh1vP5@p1>
Date: Sat, 15 Nov 2025 03:03:12 -0700
From: Xiang Mei <xmei5@....edu>
To: Toke Høiland-Jørgensen <toke@...e.dk>
Cc: security@...nel.org, netdev@...r.kernel.org, cake@...ts.bufferbloat.net,
	bestswngs@...il.com
Subject: Re: [PATCH net v3] net/sched: sch_cake: Fix incorrect qlen reduction
 in cake_drop

On Thu, Nov 13, 2025 at 02:21:54PM +0100, Toke Høiland-Jørgensen wrote:
> Xiang Mei <xmei5@....edu> writes:
> 
> > In cake_drop(), qdisc_tree_reduce_backlog() is called to decrement
> > the qlen of the qdisc hierarchy. However, this can incorrectly reduce
> > qlen when the dropped packet was never enqueued, leading to a possible
> > NULL dereference (e.g., when QFQ is the parent qdisc).
> >
> > This happens when cake_enqueue() returns NET_XMIT_CN: the parent
> > qdisc does not enqueue the skb, but cake_drop() still reduces backlog.
> >
> > This patch avoids the extra reduction by checking whether the packet
> > was actually enqueued. It also moves qdisc_tree_reduce_backlog()
> > out of cake_drop() to keep backlog accounting consistent.
> >
> > Fixes: 15de71d06a40 ("net/sched: Make cake_enqueue return NET_XMIT_CN when past buffer_limit")
> > Signed-off-by: Xiang Mei <xmei5@....edu>
> > ---
> > v2: add missing cc
> > v3: move qdisc_tree_reduce_backlog out of cake_drop
> >
> >  net/sched/sch_cake.c | 40 ++++++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index 32bacfc314c2..179cafe05085 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
> > @@ -1597,7 +1597,6 @@ 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);
> >  
> >  	cake_heapify(q, 0);
> >  
> > @@ -1750,7 +1749,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >  	ktime_t now = ktime_get();
> >  	struct cake_tin_data *b;
> >  	struct cake_flow *flow;
> > -	u32 idx, tin;
> > +	u32 dropped = 0;
> > +	u32 idx, tin, prev_qlen, prev_backlog, drop_id;
> > +	bool same_flow = false;
> >  
> >  	/* choose flow to insert into */
> >  	idx = cake_classify(sch, &b, skb, q->flow_mode, &ret);
> > @@ -1927,24 +1928,31 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >  	if (q->buffer_used > q->buffer_max_used)
> >  		q->buffer_max_used = q->buffer_used;
> >  
> > -	if (q->buffer_used > q->buffer_limit) {
> > -		bool same_flow = false;
> > -		u32 dropped = 0;
> > -		u32 drop_id;
> > +	if (q->buffer_used <= q->buffer_limit)
> > +		return NET_XMIT_SUCCESS;
> 
> While this does reduce indentation, I don't think it's an overall
> improvement; the overflow condition is the exceptional case, so it's
> clearer to keep it inside the if statement (which also keeps the
> variable scope smaller).
> 
> > -		while (q->buffer_used > q->buffer_limit) {
> > -			dropped++;
> > -			drop_id = cake_drop(sch, to_free);
> > +	prev_qlen = sch->q.qlen;
> > +	prev_backlog = sch->qstats.backlog;
> >  
> > -			if ((drop_id >> 16) == tin &&
> > -			    (drop_id & 0xFFFF) == idx)
> > -				same_flow = true;
> > -		}
> > -		b->drop_overlimit += dropped;
> > +	while (q->buffer_used > q->buffer_limit) {
> > +		dropped++;
> > +		drop_id = cake_drop(sch, to_free);
> > +		if ((drop_id >> 16) == tin &&
> > +		    (drop_id & 0xFFFF) == idx)
> > +			same_flow = true;
> > +	}
> > +	b->drop_overlimit += dropped;
> 
> The 'dropped' variable is wholly redundant after this change, so let's
> just get rid of it and use the prev_qlen for this statistic instead.
> 
> -Toke

Thanks for the reminder, and sorry for the delayed reply.
You are right that we should remove the unused variable.

Xiang