From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mail.toke.dk; dkim=pass header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20251104 header.b=GANuxTnY; arc=pass; dmarc=pass (Used From Domain Record) header.from=google.com policy.dmarc=reject Received: from mail-qk2-x0c.google.com (mail-qk2-x0c.google.com [IPv6:2607:f8b0:4864:34::c]) by mail.toke.dk (Postfix) with ESMTPS id 2183317125C6 for ; Mon, 28 Sep 2026 14:34:06 +0200 (CEST) Received: by mail-qk2-x0c.google.com with SMTP id d75a77b69052e-5332b967eb4so16500651cf.3 for ; Mon, 28 Sep 2026 05:34:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1790598845; cv=none; d=google.com; s=arc-20260327; b=XM6TIn7BEUZQtYCS1ari9k9FcRNuZGSEfvm3z8FlMToc6O3j43qwp1SO25TGSmvqc4 OUtgPaKC4OysWigdKqZuoPHNw0wNIyDw4oEBVDgk3zsDseOKwNFhqwWcijKGqkdRAEPc RMIFRk8a0xZk/5hop9JA0IyPzkRdr1vL6VKYv7dcj1JP+SN0Usf8qCTZaONpwBqRUhj2 j+N09WMfPuebCw59/Ue+qWpzKKxR8UhtOJHOHIBxyWjURl5U+24cAzUAdqBM2aMBMbKN H7HM26hOthrURcAkJiQTLhqfi1N7Hy0u1lzYpJNK6fX/eG75prSd27U73XFt1ics2KRk +LHQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20260327; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=U7l+r5+9U0CBVRhBEzoj9hWYpVtHLgr+opW35WkRHB0=; fh=Slv5psDtcmN3lyW21Z9z65M/4ybUSyF0yS6xyfoo2FQ=; b=aHR92OYYdC7r9iuX1DmbxuvPzc9a644u0Fsq+JvCyVwuCu+1/ckCZyd82F145+e68A 6WVY4APyHsq2B1ekX4QLYsgzwGZkBsJFnJsBtqLWMN8qyntQcdVmcluxa3As833W7ech 1ZrplN1ZtPwP+qLUTW/+huWeXvhXOEeaoECayGgpmkUDZzjTc02kF9hP8XpAFUKwmGjL Ku1byfIhhKrMKNJbHigJP69ZxdJ9MgRilypC5FPBVOw6Awk62MACPOYmEDOxTRBNAt2Z Emmk7bO2c2ZjeGImiPn1VobzwQU9OvuxV8WkKceLfUmIn6S3mEkbcXCoeSGioDW4Cd6b pINQ==; darn=lists.bufferbloat.net ARC-Authentication-Results: i=1; mx.google.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1790598845; x=1791203645; darn=lists.bufferbloat.net; h=content-transfer-encoding:content-type:cc:to:subject:message-id :date:from:in-reply-to:references:mime-version:from:to:cc:subject :date:message-id:reply-to:content-type; bh=U7l+r5+9U0CBVRhBEzoj9hWYpVtHLgr+opW35WkRHB0=; b=GANuxTnYU1eYArf0MtXM3f9N2Uae0sj7jcy5X4f8AuLxoPTeTguvz07FMEBu5WzguN IxCDH2FFO0xGoiPs8tOffqTMzscvQBMEtjdsL49SLkWq86qsVM++1Ccc/0mXtTM4S0WA 8XUz8lVa6By+epIffMULzsZaV73mjSPT6VrGbud1ZKujneNpxfuZ/Cr3mLpBc0bCpSTw IXBeoIUSNPMu/TSQ/ElwB6ZDHdNd+s+5pg/XjaELSkizwwoSKQAfdGQ6Y3f627YJhqj3 gOGDVI9vGRjQvNTl27iScXGorKUVZ0PaBcyJeFvQRbH4qOthozZ1fsxAPkjB03wRH5sh nFMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1790598845; x=1791203645; h=content-transfer-encoding:content-type: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 :content-type; bh=U7l+r5+9U0CBVRhBEzoj9hWYpVtHLgr+opW35WkRHB0=; b=0s0INkwFVdUnyVP64R/CTYqJ8pwe/aGkEpxIhWfGz7OY6IbJLykWaWexO3uQrOQ8mt Z4O4OTGdqRjAmg94/ESXulGXofqD21T/TtlSsBBRtqDfPXpD6GP41ZSvMQ/PLE1/eCUn r7Y6+XyDit/Wwyvo7AV6psIq76KCHI1mUWoWqjftzDMOFlgpXig6vNhFMyPV3yjMCvI/ 9dfJ3EqnD6Tfd1SOITl1TpWIyyjk6trxW1Q1VD+0N9FPNq/K1WvPotvfXGo4VT7z6mj4 v8VNzobzAmWZ9Mu9iJvSOBLG4MqZ0EKtJPcGeKQ7bTCc6ydYnHOuP87qXOuzVHhVIQcS Co/Q== X-Forwarded-Encrypted: i=1; AKwUvBySSgCsZ5lqAMJo02Ne0tPA0gb/7GKPVQNPBqVuDGO25iTYRet7VlPyyjnhHkLwVPq4Qh15@lists.bufferbloat.net X-Gm-Message-State: AFuF++kmAXBgl2UAGGAyGaiHOh1VhANpuwGz+yhCVF2Wf5Y68uXqWkei 9CzJpeEQ0qRhCinEOCIOSl00BsEBsEVBznyV6YbQvkSOB1VZqdVknEgYTSIph2yVEJhB/pdA4Ca TlOoRYsfAJU7zg+kqefeBBq3QEy9eURzt09wE7Fet X-Gm-Gg: AYBFou3EE8+SbXiMPsxgC9nQmoTKmDntQffQdvVtzWNSP+bbiOPIO6VSTYuXcI1AP03 xcuNZ2CXuft/oeYPXKscMxLVVbHRMsz3vF7Jtdlt8Et6kX4jvX2LCC1mkuKopK+xDT5DPP9ohP0 TaGiUv+D2JnkeN+uyceCj5XJarXhgCBjsNJ4s0cNxzMfY+Sjg2AVSXI2IVS8Eylgcctrz1NnsZ0 V3UW9CvXOgyWFKhX0LdQA5oi8b0GwDqYDcX6LMueVeHdo2e2Kxf+ac1J/JrK5Y5tYLkIcult+Xx G5uPzb0XGA2Rjo3VIOjMuOvpO5+stRQ6vu+x2yE+WSKu3qrIt1RSqR7o+IWhjBZ/Lz8A7iVxRbs b9cXXcoEARRexrfg8H/TmFXjQukd6BcsbLEY3xn00QM6pKUxRgwvOsUaURXw582BUhh1Z X-Received: by 2002:a05:622a:5588:b0:531:218b:ee3f with SMTP id d75a77b69052e-5330dc5ca37mr213290801cf.25.1790598843947; Mon, 28 Sep 2026 05:34:03 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Eric Dumazet Date: Mon, 28 Sep 2026 14:33:50 +0200 X-Gm-Features: AclHuK8RHBXbb0dCnDGM6uMwoUbwRNmxLUMXWKrgWCkdRRhvNALrs_kLAxdH6oU Message-ID: To: Jamal Hadi Salim Cc: netdev@vger.kernel.org, Jiri Pirko , "David S . Miller" , Jakub Kicinski , Paolo Abeni , Simon Horman , Victor Nogueira , hybris , =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= , moeller0@gmx.de, cake@lists.bufferbloat.net, Sashiko Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 3WHGERV67RPU4G2TS35PW5XCOVC7DCCK X-Message-ID-Hash: 3WHGERV67RPU4G2TS35PW5XCOVC7DCCK X-MailFrom: edumazet@google.com 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 net-next v2] net/sched: cap the accounted backlog before it can wrap List-Id: Cake - FQ_codel the next generation Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Sat, Sep 26, 2026 at 7:49=E2=80=AFPM Jamal Hadi Salim = wrote: > > fq_codel, cake, codel, pie, fq_pie and dualpi2 account every enqueued > packet's stab-adjusted length into a 32-bit sch->qstats.backlog. > fq_codel/codel uses it do decide if they should drop a packet at deq; > cake uses it to prune the longest-flow heap from per-flow backlogs; > pie and fq_pie use it to make early drop decisions and, dualpi2 decides > must_drop() on it. RED can can decide on a child's backlog based on it. > A crafted tc size table (TC_STAB) inflates qdisc_pkt_len() up to > QDISC_PKT_LEN_MAX (1 MiB), so a few thousand packets wrap the counter > mod 2^32. The AQM algo then reads a small backlog and makes the wrong > drop decision, and the dequeue-side subtractions keep the counter corrupt= . > > Fix: > Drop at enqueue once the accounted backlog would cross QDISC_MAX_BACKLOG > (U32_MAX - QDISC_PKT_LEN_MAX), the largest backlog one more maximum-size > packet cannot wrap. This follows the existing bfifo/gred approach > (safe because its limit is checked against the accounted packet length); > the fixed qdiscs' limits are packet counts or otherwise do not bound > the aggregate bytes, so they need the byte bound here. Hi Jamal, Thanks for reworking this for v2. A few comments on the implementation: 1. QDISC_MAX_BACKLOG check Since QDISC_MAX_BACKLOG is already defined as (U32_MAX - QDISC_PKT_LEN_MAX), it already reserves QDISC_PKT_LEN_MAX of headroom below U32_MAX for the incoming packet. Doing: if (unlikely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) > QDISC_MAX_BACKLOG)) accounts for the incoming packet size twice and forces a 64-bit addition at every call site. Why not just check unlikely(sch->qstats.backlog > QDISC_MAX_BACKLOG) (or provide a small helper in include/net/sch_generic.h)? 2. sch_cake.c (cake_enqueue) There are a few issues with how cake_enqueue() is handled: - The first check is placed after cake_classify(), which has already modified the packet's DSCP (cake_handle_diffserv()) and updated set-associative hash state and host bulk-flow counters in cake_hash() (srchost_bulk_flow_count / dsthost_bulk_flow_count) assuming the packet will be enqueued into flow. - In the CAKE_FLAG_SPLIT_GSO branch, the second check runs after lines 1780-1801 have already updated b->max_skblen, shaper timestamps (time_next_packet), qstats.overlimits, and scheduled &q->watchdog. - Walking segs a second time on every GSO packet just to sum slen is unnecessary, and calling both kfree_skb_list_reason(segs, ...) (under the qdisc lock instead of via to_free) and qdisc_drop_reason(skb, ...) triggers duplicate drop tracepoints for both the segments and the parent GSO skb. Since QDISC_MAX_BACKLOG already leaves QDISC_PKT_LEN_MAX (1 MiB) of headroom, a single check at the very beginning of cake_enqueue() before cake_classify() is sufficient and avoids touching the GSO split path altogether. 3. sch_fq_codel.c (fq_codel_enqueue) Can we move the backlog check before fq_codel_classify() (or in the !q->filter_list fast path before fq_codel_hash()) so we do not compute the flow hash for packets we are about to drop? Also, please mention in the commit message that wrapping q->backlogs[i] to 0 in fq_codel causes fq_codel_drop() to leave idx =3D 0 and dereference a NULL flow->head if flow 0 is empty. 4. sch_red.c and sch_fifo.c (pfifo) In red_enqueue(), red_calc_qavg() reads child->qstats.backlog. In your commit message example, child->qstats.backlog wrapped because the child was a packet-limited pfifo (limit 100000). Shouldn't pfifo_enqueue() and pfifo_tail_enqueue() in net/sched/sch_fifo.c also guard against qstats.backlog wrapping when used standalone or under other classful qdiscs? Thanks!