| 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
| ||
|
Message-ID: <aRVZJmTAWyrnXpCJ@p1>
Date: Wed, 12 Nov 2025 21:05:58 -0700
From: Xiang Mei <xmei5@....edu>
To: security@...nel.org
Cc: netdev@...r.kernel.org, toke@...e.dk, cake@...ts.bufferbloat.net,
bestswngs@...il.com
Subject: Re: [PATCH net v3] net/sched: sch_cake: Fix incorrect qlen reduction
in cake_drop
On Wed, Nov 12, 2025 at 08:53:03PM -0700, Xiang Mei wrote:
> 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 (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;
> +
> + /* Compute the droppped qlen and pkt length */
> + prev_qlen -= sch->q.qlen;
> + prev_backlog -= sch->qstats.backlog;
>
> - if (same_flow)
> - return NET_XMIT_CN;
> + if (same_flow) {
> + qdisc_tree_reduce_backlog(sch, prev_qlen - 1,
> + prev_backlog - len);
> + return NET_XMIT_CN;
> }
> + qdisc_tree_reduce_backlog(sch, prev_qlen, prev_backlog);
> return NET_XMIT_SUCCESS;
> }
>
> --
> 2.43.0
>
Thank Toke for the suggestion to move qdisc_tree_reduce_backlog out of
cake_drop. It makes the logic cleaner.
The patch passed CAKE's self-test:
```log
ok 1 1212 - Create CAKE with default setting
ok 2 3281 - Create CAKE with bandwidth limit
ok 3 c940 - Create CAKE with autorate-ingress flag
ok 4 2310 - Create CAKE with rtt time
ok 5 2385 - Create CAKE with besteffort flag
ok 6 a032 - Create CAKE with diffserv8 flag
ok 7 2349 - Create CAKE with diffserv4 flag
ok 8 8472 - Create CAKE with flowblind flag
ok 9 2341 - Create CAKE with dsthost and nat flag
ok 10 5134 - Create CAKE with wash flag
ok 11 2302 - Create CAKE with flowblind and no-split-gso flag
ok 12 0768 - Create CAKE with dual-srchost and ack-filter flag
ok 13 0238 - Create CAKE with dual-dsthost and ack-filter-aggressive flag
ok 14 6572 - Create CAKE with memlimit and ptm flag
ok 15 2436 - Create CAKE with fwmark and atm flag
ok 16 3984 - Create CAKE with overhead and mpu
ok 17 5421 - Create CAKE with conservative and ingress flag
ok 18 6854 - Delete CAKE with conservative and ingress flag
ok 19 2342 - Replace CAKE with mpu
ok 20 2313 - Change CAKE with mpu
ok 21 4365 - Show CAKE class
```
There is still one problem I am not very sure since I am not very
experienced with cake and gso. It's about the gso branch [1]. The slen
is the lenth added to the cake sch and that branch uses
`qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);` to inform the
parent sched. However, when we drop the packet, it could be probmatic
since we should reduce slen instead of len. Is this a potential problem?
[1] https://elixir.bootlin.com/linux/v6.6.116/source/net/sched/sch_cake.c#L1803
Thanks,
Xiang
Powered by blists - more mailing lists