[Cake] Re: [PATCH net-next v2] net/sched: cap the accounted backlog before it can wrap

[Eric Dumazet <edumazet@google.com>]

On Sat, Sep 26, 2026 at 7:49 PM Jamal Hadi Salim <jhs@mojatatu.com> 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 = 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!