From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; spf=pass smtp.mailfrom=asu.edu; dkim=pass header.d=asu.edu; arc=none (Message is not ARC signed); dmarc=none Received: from mail-qk1-x732.google.com (mail-qk1-x732.google.com [IPv6:2607:f8b0:4864:20::732]) by mail.toke.dk (Postfix) with ESMTPS id 55CF09CC045 for ; Tue, 25 Nov 2025 23:04:14 +0100 (CET) Received: by mail-qk1-x732.google.com with SMTP id af79cd13be357-8b22b1d3e7fso609843785a.3 for ; Tue, 25 Nov 2025 14:04:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=asu.edu; s=google; t=1764108253; x=1764713053; darn=lists.bufferbloat.net; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=NFV5BEQO0h6WteiqsW0VzyRefRg4w03+xa45BOA9TX0=; b=feAf0P3k3bJtsj5Fdp8l46L7xfQW8Tn8d8iJR6ulRjDkMFa3ebBEcIZHg3Y1eXbc/5 B5ATVxbAsIFFjZvcSj6Yk5o8rJYYNgwpyvr720F8+HVFUA0nacXzBKur4wtEQjZMyGFt F1nDYTgtZ6xX5E05XR2ynIzdt65sjlgyTbjTZDzUZ7OUhiMB7zB6zjRkuq7h5Mky7ycU aFtW5SAkT9Zl+PbQ8Kw+mY6HVSGEVJsPgNGb8kTEGxtDdLyk9FyXvE+45znbo33sJGlH hDeGsr3XJZRPcF/4p/uhOku4lKrGcs6VrNyFtIIzPd6JOOay8T/0MQKO6T1tK8faEhRw vN1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764108253; x=1764713053; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=NFV5BEQO0h6WteiqsW0VzyRefRg4w03+xa45BOA9TX0=; b=CDquKwRQ2z9GA5kcUOg7gMspkcIUY+Zgve1lXofOB39pkKb8ysYzXQy/uxfRtQkWuj FuEidGtM24MZ9eF79IxYYTjjNUXdGFQq8VeSgVeEa0PvTf72c4RiMndW8LF5s5ykc8/e ZgOEPzZuERM0zbbfI/E4guzLbGWvVKQOamOoBqvPg+YXs36K8+aQvS3MfEKq0Mtyjl6s XRWSCmsM+D3zOrBmP0QeMlRUwVg9pUnx+Gw8osShHnbPNMizK/HlrtluE45OLXYjnFtR yilI/iZ5hGf9htzX0ikT/r8MpavqSeaOg8n6wkITM5UsNu+F1kAvvF/o6jn+BOMS7cr0 5rOA== X-Forwarded-Encrypted: i=1; AJvYcCXbltLXhl9brdmRA8P/8F4K6Cj/gk68A5srxy9tlOMXykqvqhxI1hHqoOnnGGNgIDJbW2j1@lists.bufferbloat.net X-Gm-Message-State: AOJu0Yzh2fQ/ieuJoap/wu3lsjF5cNxPBgaU6YQXDBV8AecFGyjxY1CO zU2780p14k92UXfbO3l/+Lg67HpnhMhtQl3OBRfGf9NIEfZqfWNZpcKg4GY1EeA8E0qqF0H4bge VOe/GEP35tPgg7KlInrCaQ/1IYpmtwzGTLySFSSh0 X-Gm-Gg: ASbGnctmc5DWoEWAGUTizS+v09xFXidZNN3d6hVtBv6kmr5gA2rNKZOXtNuDzmln9lA CiFM92uIF6NVwyyBGIwhqIe3cMZG8MtjPPi2gl5mmoHYInNKRUOFGL5Ks/U9UuE/vHNNnDSYN4K +gRB156p8LfdHL/7zl1+BsE+jPTQzQSFgrsvktJmddMQjEyJ3dtMpD1lwdFUZHH7Lk+Zu0rxuab wAdbr7aWLA+55c6eAyH1K0p18tWkb7vle9R5auL0riTMfIwDuaQedFQ+fK8IfkFir+JK07Q X-Google-Smtp-Source: AGHT+IESzt7BrjzYz4e8kdr8+LMVfFjZ8Q6Xq9xG+iiyoBcY83fq4TrwHsXgnDYyI7frxtco27d+yOIJ0IDn8jEzS38= X-Received: by 2002:a05:620a:4709:b0:8b2:5fa9:61 with SMTP id af79cd13be357-8b33d267d7bmr2300085085a.25.1764108252165; Tue, 25 Nov 2025 14:04:12 -0800 (PST) MIME-Version: 1.0 References: <20251121232735.1020046-1-xmei5@asu.edu> <87ms4bn1u9.fsf@toke.dk> In-Reply-To: <87ms4bn1u9.fsf@toke.dk> From: Xiang Mei Date: Tue, 25 Nov 2025 15:04:01 -0700 X-Gm-Features: AWmQ_bkZafX0FyomYdSL-ky-6_gsr8v5aKzzKvrUSz26Aqb26T7pNrCY8b1XZ7Q Message-ID: To: =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= Cc: security@kernel.org, netdev@vger.kernel.org, xiyou.wangcong@gmail.com, cake@lists.bufferbloat.net, bestswngs@gmail.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-MailFrom: xmei5@asu.edu X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation Message-ID-Hash: Q2LMIMURPQCYW5PZ532NG6RFF4DLTXCR X-Message-ID-Hash: Q2LMIMURPQCYW5PZ532NG6RFF4DLTXCR X-Mailman-Approved-At: Wed, 26 Nov 2025 13:43:24 +0100 X-Mailman-Version: 3.3.10 Precedence: list Subject: [Cake] Re: [PATCH net v5] 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: Thanks so much for reviewing and pointing out my mistake on `drop_overlimit.` New patches are sent. On Mon, Nov 24, 2025 at 3:48=E2=80=AFAM Toke H=C3=B8iland-J=C3=B8rgensen wrote: > > Xiang Mei writes: > > > In cake_drop(), qdisc_tree_reduce_backlog() is used to update the qlen > > and backlog of the qdisc hierarchy. Its caller, cake_enqueue(), assumes > > that the parent qdisc will enqueue the current packet. However, this > > assumption breaks when cake_enqueue() returns NET_XMIT_CN: the parent > > qdisc stops enqueuing current packet, leaving the tree qlen/backlog > > accounting inconsistent. This mismatch can lead to a NULL dereference > > (e.g., when the parent Qdisc is qfq_qdisc). > > > > This patch computes the qlen/backlog delta in a more robust way by > > observing the difference before and after the series of cake_drop() > > calls, and then compensates the qdisc tree accounting if cake_enqueue() > > returns NET_XMIT_CN. > > > > To ensure correct compensation when ACK thinning is enabled, a new > > variable is introduced to keep qlen unchanged. > > > > Fixes: 15de71d06a40 ("net/sched: Make cake_enqueue return NET_XMIT_CN w= hen past buffer_limit") > > Signed-off-by: Xiang Mei > > --- > > v2: add missing cc > > v3: move qdisc_tree_reduce_backlog out of cake_drop > > v4: remove redundant variable and handle ack branch correctly > > v5: add the PoC as a test case > > Please split the test case into its own patch and send both as a series. > > Otherwise, the changes LGTM apart from the few nits below: > > > --- > > net/sched/sch_cake.c | 52 +++++++++++-------- > > .../tc-testing/tc-tests/qdiscs/cake.json | 28 ++++++++++ > > 2 files changed, 58 insertions(+), 22 deletions(-) > > > > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c > > index 32bacfc314c2..cf4d6454ca9c 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_OVERLI= MIT); > > sch->q.qlen--; > > - qdisc_tree_reduce_backlog(sch, 1, len); > > > > cake_heapify(q, 0); > > > > @@ -1750,7 +1749,8 @@ static s32 cake_enqueue(struct sk_buff *skb, stru= ct Qdisc *sch, > > ktime_t now =3D ktime_get(); > > struct cake_tin_data *b; > > struct cake_flow *flow; > > - u32 idx, tin; > > + u32 idx, tin, prev_qlen, prev_backlog, drop_id; > > + bool same_flow =3D false; > > Please make sure to maintain the reverse x-mas tree ordering of the > variable declarations. > > > > > /* choose flow to insert into */ > > idx =3D cake_classify(sch, &b, skb, q->flow_mode, &ret); > > @@ -1823,6 +1823,8 @@ static s32 cake_enqueue(struct sk_buff *skb, stru= ct Qdisc *sch, > > consume_skb(skb); > > } else { > > /* not splitting */ > > + int ack_pkt_len =3D 0; > > + > > cobalt_set_enqueue_time(skb, now); > > get_cobalt_cb(skb)->adjusted_len =3D cake_overhead(q, skb= ); > > flow_queue_add(flow, skb); > > @@ -1834,7 +1836,7 @@ static s32 cake_enqueue(struct sk_buff *skb, stru= ct Qdisc *sch, > > b->ack_drops++; > > sch->qstats.drops++; > > b->bytes +=3D qdisc_pkt_len(ack); > > - len -=3D qdisc_pkt_len(ack); > > + ack_pkt_len =3D qdisc_pkt_len(ack); > > There's a qdisc_tree_reduce_backlog() that uses qdisc_pkt_len(ack) just > below this; let's also change that to use ack_pkt_len while we're at it. > > > q->buffer_used +=3D skb->truesize - ack->truesize= ; > > if (q->rate_flags & CAKE_FLAG_INGRESS) > > cake_advance_shaper(q, b, ack, now, true)= ; > > @@ -1848,11 +1850,11 @@ static s32 cake_enqueue(struct sk_buff *skb, st= ruct Qdisc *sch, > > > > /* stats */ > > b->packets++; > > - b->bytes +=3D len; > > - b->backlogs[idx] +=3D len; > > - b->tin_backlog +=3D len; > > - sch->qstats.backlog +=3D len; > > - q->avg_window_bytes +=3D len; > > + b->bytes +=3D len - ack_pkt_len; > > + b->backlogs[idx] +=3D len - ack_pkt_len; > > + b->tin_backlog +=3D len - ack_pkt_len; > > + sch->qstats.backlog +=3D len - ack_pkt_len; > > + q->avg_window_bytes +=3D len - ack_pkt_len; > > } > > > > if (q->overflow_timeout) > > @@ -1927,24 +1929,30 @@ static s32 cake_enqueue(struct sk_buff *skb, st= ruct Qdisc *sch, > > if (q->buffer_used > q->buffer_max_used) > > q->buffer_max_used =3D q->buffer_used; > > > > - if (q->buffer_used > q->buffer_limit) { > > - bool same_flow =3D false; > > - u32 dropped =3D 0; > > - u32 drop_id; > > + if (q->buffer_used <=3D q->buffer_limit) > > + return NET_XMIT_SUCCESS; > > > > - while (q->buffer_used > q->buffer_limit) { > > - dropped++; > > - drop_id =3D cake_drop(sch, to_free); > > + prev_qlen =3D sch->q.qlen; > > + prev_backlog =3D sch->qstats.backlog; > > > > - if ((drop_id >> 16) =3D=3D tin && > > - (drop_id & 0xFFFF) =3D=3D idx) > > - same_flow =3D true; > > - } > > - b->drop_overlimit +=3D dropped; > > + while (q->buffer_used > q->buffer_limit) { > > + drop_id =3D cake_drop(sch, to_free); > > + if ((drop_id >> 16) =3D=3D tin && > > + (drop_id & 0xFFFF) =3D=3D idx) > > + same_flow =3D true; > > + } > > + > > + /* Compute the droppped qlen and pkt length */ > > + prev_qlen -=3D sch->q.qlen; > > + prev_backlog -=3D sch->qstats.backlog; > > + b->drop_overlimit +=3D prev_backlog; > > drop_overlimit was accounted in packets before, so this should be +=3D pr= ev_qlen. > > -Toke