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

[Jamal Hadi Salim <jhs@mojatatu.com>]

On Mon, Sep 28, 2026 at 8:51 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 8 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 2 · Low: 4
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] RED's new ceiling check (red_enqueue(), sch_red.c:79-84) only
>   tests sch->qstats.backlog + qdisc_pkt_len(skb) before the skb goes to…
> - [High] The patch moves `len = qdisc_pkt_len(skb)` in red_enqueue() from
>   after the red_action() switch to the top of the function…
> - [Medium] The commit message leaves GRED untouched, saying it follows a
>   safe approach ("safe because its limit is checked against the…
> - [Medium] In the new split-GSO pre-check in cake_enqueue(), segment
>   lengths are summed into `unsigned int slen` (declared at…
> - [Low] The commit fixes a user-visible bug.
> - [Low] In the split-GSO path of cake_enqueue(), when the post-split
>   total would cross the ceiling, the patch frees every temporary segment…
> - [Low] The comment on QDISC_MAX_BACKLOG (include/net/pkt_sched.h:16-19)
>   and the commit message treat QDISC_PKT_LEN_MAX (1 MiB) as the maximum…
> - [Low] The new RED ceiling rejection in red_enqueue() (sch_red.c:80-84)
>   only calls qdisc_qstats_overlimit() and qdisc_drop_reason().
>

I will send a V3 to address some of these issues (also brought up by Eric)

cheers,
jamal

> --
>
> net/sched: cap the accounted backlog before it can wrap
>
> This patch adds an enqueue-side check to fq_codel, cake, codel, pie,
> fq_pie, dualpi2 and RED. The check drops a packet when the 32-bit
> sch->qstats.backlog would cross QDISC_MAX_BACKLOG
> (U32_MAX - QDISC_PKT_LEN_MAX). The goal is to stop a crafted stab from
> wrapping the backlog counter and corrupting AQM drop decisions.
>
> Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
>
> > This follows the existing bfifo/gred approach
> > (safe because its limit is checked against the accounted packet length);
>
> [Severity: Medium]
> Is this accurate for GRED? In non-WRED mode (plain or RIO),
> gred_backlog() returns the per-VQ q->backlog. That means gred_enqueue()
> only checks each virtual queue against its own q->limit:
>
> net/sched/sch_gred.c:gred_enqueue() {
>     ...
>         if ((u64)gred_backlog(t, q, sch) + qdisc_pkt_len(skb) <= q->limit) {
>                 q->backlog += qdisc_pkt_len(skb);
>                 return qdisc_enqueue_tail(skb, sch);
>         }
>     ...
> }
>
> Every accepted packet is still added to the shared 32-bit
> sch->qstats.backlog through qdisc_enqueue_tail(). Each q->limit is capped
> at sch->limit, but there can be up to 16 VQs.
>
> With two or more VQs whose limits add up to more than 4 GiB, plus the
> same 1 MiB stab, can the aggregate wrap?
>
> Once it wraps, the pass-through admission compares against a value that
> is too small:
>
>         if (likely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) <=
>                         sch->limit))
>                 return qdisc_enqueue_tail(skb, sch);
>
> gred_backlog() returns this same aggregate in WRED mode. The WRED idle
> check in gred_dequeue() also reads it if the table is switched to WRED
> while packets are still queued. So a wrapped value could reach
> red_calc_qavg().
>
> Should GRED get the same QDISC_MAX_BACKLOG check, or should the commit
> message be changed?
>
> > Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
> > Link: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> > Tested-by: hybris <hybris@mojatatu.ai>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> [Severity: Low]
> Should this have a Fixes: tag? The commit message describes a bug that
> can be reached from a user namespace and includes a reproducer, but
> there is no Fixes: tag, so the stable tooling will not pick it up
> automatically.
>
> QDISC_MAX_BACKLOG also depends on QDISC_PKT_LEN_MAX and on the pkt_len
> clamp in __qdisc_calculate_pkt_len(). Both come from commit 8f735d64382d
> ("net/sched: bound qdisc_pkt_len to prevent qdisc soft lockup").
>
> Could the commit message mention that dependency? A backport without it
> will not build. 8f735d64382d has its own Fixes: 1da177e4c3f4, so it will
> probably reach stable anyway.
>
> > diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
> > index 90d3e7943b19a..351b92f956efc 100644
> > --- a/include/net/pkt_sched.h
> > +++ b/include/net/pkt_sched.h
> > @@ -13,6 +13,11 @@
> >  #define DEFAULT_TX_QUEUE_LEN 1000
> >  #define STAB_SIZE_LOG_MAX    30
> >  #define QDISC_PKT_LEN_MAX    (1 << 20)       /* 1 MiB */
> > +/*
> > + * Largest accounted backlog for which enqueuing one more maximum-size
> > + * packet cannot wrap the 32-bit sch->qstats.backlog.
> > + */
> > +#define QDISC_MAX_BACKLOG    (U32_MAX - QDISC_PKT_LEN_MAX)
>
> [Severity: Low]
> Is QDISC_PKT_LEN_MAX really the largest possible accounted packet length?
> The clamp is only applied in __qdisc_calculate_pkt_len(), which runs only
> when a size table is attached:
>
>         pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
>
> Without a stab, qdisc_pkt_len_segs_init() sets the length with no clamp:
>
>         qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len;
>
> packet_snd() skips the MTU check for GSO packets and can build skbs
> larger than 500 KB. virtio_net_hdr_to_skb() only forces
> gso_size >= TCP_MIN_GSO_SIZE. So a GSO skb can carry an accounted length
> well above 1 MiB.
>
> The new checks compare in u64 against the real length, so they do not
> wrap on their own. Still, the "one more maximum-size packet cannot wrap"
> invariant in this comment and in the commit message does not hold. The
> RED child segmentation question below relies on the same headroom
> assumption.
>
> >
> >  struct qdisc_walker {
> >       int     stop;
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index dc93267029e70..6a16546aa5b0e 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
>
> [ ... ]
>
> > @@ -1801,7 +1809,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >               WRITE_ONCE(b->max_skblen, len);
> >
> >       if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> > -             struct sk_buff *segs, *nskb;
> > +             struct sk_buff *segs, *nskb, *seg;
> >               netdev_features_t features = netif_skb_features(skb);
> >               unsigned int slen = 0, numsegs = 0;
> >
> > @@ -1809,6 +1817,23 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >               if (IS_ERR_OR_NULL(segs))
> >                       return qdisc_drop(skb, sch, to_free);
> >
> > +             /* The segment list is accounted by the sum of its lengths,
> > +              * which can exceed the original packet's accounted length, so
> > +              * sum it before linking any segment and drop the whole list if
> > +              * the post-split total would cross the ceiling.
> > +              */
> > +             skb_list_walk_safe(segs, seg, nskb)
> > +                     slen += seg->len;
> > +
> > +             if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
>
> [Severity: Medium]
> Can slen wrap before the u64 comparison? slen is still an unsigned int.
> If the segment list adds up to more than 4 GiB, the sum wraps and the
> check passes with a small value.
>
> cake would then add the wrapped slen to sch->qstats.backlog,
> b->tin_backlog and b->backlogs[idx], while the queued segments keep
> their real lengths. Later dequeue or cake_drop() subtractions would then
> underflow those counters.
>
> Here is how that size might be reached:
>
>   - packet_snd() skips the MTU check for GSO packets and has no
>     GSO_MAX_SIZE cap.
>   - packet_alloc_skb() allows about 557 KB of order-3 frags plus a large
>     linear part.
>   - virtio_net_hdr_to_skb() only raises gso_size to TCP_MIN_GSO_SIZE (8).
>   - ipv6_gso_pull_exthdrs() walks any number of GSO-capable extension
>     headers.
>
> A TCPv6 DODGY skb with about 60 KB of extension headers and 560 KB or
> more of payload at gso_size 8 would split into more than 70k segments of
> about 60 KB each. pkt_segs is a truncated u16 but is still > 1, so the
> split path is taken.
>
> This needs skb_segment() to succeed at more than 4 GiB of GFP_ATOMIC
> allocations from a single packet. Not every check on the segmentation
> path has been traced. Would a u64 accumulator for this pre-check be
> simpler than working out whether it can happen?
>
> > +                     kfree_skb_list_reason(segs, SKB_DROP_REASON_QDISC_DROP);
> > +                     WRITE_ONCE(flow->dropped, flow->dropped + 1);
> > +                     WRITE_ONCE(b->tin_dropped, b->tin_dropped + 1);
> > +                     qdisc_qstats_overlimit(sch);
> > +                     return qdisc_drop_reason(skb, sch, to_free,
> > +                                              QDISC_DROP_OVERLIMIT);
> > +             }
>
> [Severity: Low]
> When this ceiling is hit, the segments are freed with
> SKB_DROP_REASON_QDISC_DROP. The original skb is then also dropped with
> QDISC_DROP_OVERLIMIT.
>
> Doesn't that produce N+1 kfree_skb tracepoint and drop-monitor events,
> under two different reasons, for one rejected packet? flow->dropped,
> tin_dropped and qstats.drops each go up by only one.
>
> The segments were never queued. Could they be consumed instead, or
> dropped with the same overlimit reason, so the event is reported once?
> That would also match the changelog's statement that this "drops with
> QDISC_DROP_OVERLIMIT".
>
> [ ... ]
>
> > diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> > index d7598214270b8..dff3d8b0556bf 100644
> > --- a/net/sched/sch_red.c
> > +++ b/net/sched/sch_red.c
> > @@ -76,6 +76,13 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >       unsigned int len;
> >       int ret;
> >
> > +     len = qdisc_pkt_len(skb);
> > +     if (unlikely((u64)sch->qstats.backlog + len > QDISC_MAX_BACKLOG)) {
> > +             qdisc_qstats_overlimit(sch);
> > +             return qdisc_drop_reason(skb, sch, to_free,
> > +                                      QDISC_DROP_OVERLIMIT);
> > +     }
>
> [Severity: High]
> Does this check still bound RED's backlog when the child segments GSO
> skbs? It only looks at the length before segmentation, before the skb
> is handed to the child.
>
> With a TBF child, tbf_enqueue() calls tbf_segment() when
> qdisc_pkt_len(skb) > q->max_size. tbf_segment() re-accounts every
> segment at its raw length and pushes the difference up to RED:
>
> net/sched/sch_tbf.c:tbf_segment() {
>     ...
>                 seg_len = segs->len;
>                 qdisc_skb_cb(segs)->pkt_len = seg_len;
>     ...
>         qstats_backlog_add(sch, len);
>         if (nb > 0) {
>                 qdisc_tree_reduce_backlog(sch, 1 - nb, prev_len - len);
>     ...
> }
>
> qdisc_tree_reduce_backlog() calls qstats_backlog_sub() on each ancestor.
> A negative adjustment therefore grows RED's backlog by (len - prev_len)
> with no ceiling check. netem_enqueue() does the same through its
> netem_segment() path:
>
>         qdisc_tree_reduce_backlog(sch, -(nb - 1), -(len - prev_len));
>
> A DODGY GSO skb from packet_snd() has no GSO size cap, and gso_size can
> be as small as TCP_MIN_GSO_SIZE (8). With maximum IPv4/TCP headers
> (134 bytes), about 60 KB of payload then yields more than 1 MiB of
> segment bytes. A stab can map that skb to a length just above TBF
> max_size, while filler packets map to 1 MiB.
>
> Take the commit's own RED reproducer, with the packet-limited pfifo
> placed under a TBF, and RED's backlog near QDISC_MAX_BACKLOG. Can both
> RED's and TBF's 32-bit backlogs still wrap?
>
> If so, red_enqueue() would feed the wrapped child->qstats.backlog into
> red_calc_qavg(), which is what this patch is meant to prevent for RED.
> cake and dualpi2 re-check after segmentation, but RED with a segmenting
> child does not.
>
> [Severity: Low]
> This new drop path only increments qstats.overlimits and qstats.drops.
> Should it also update one of RED's own counters? red_dump_stats()
> exports pdrop from q->stats.pdrop, and the uAPI defines it as:
>
>         __u32           pdrop;          /* Drops due to queue limits */
>
> Every other drop path in red_enqueue() updates prob_drop, forced_drop or
> pdrop, so a drop at the byte ceiling never shows up in RED's xstats.
> Qevent TC_ACT_SHOT drops are already missing from xstats, so the
> breakdown did not add up to qstats.drops before this patch either.
>
> [ ... ]
>
> > @@ -135,7 +142,6 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >               break;
> >       }
> >
> > -     len = qdisc_pkt_len(skb);
> >       ret = qdisc_enqueue(skb, child, to_free);
> >       if (likely(ret == NET_XMIT_SUCCESS)) {
> >               qstats_backlog_add(sch, len);
>
> [Severity: High]
> Since this read moved to the top of red_enqueue(), len is now taken
> before the qe_mark tcf_qevent_handle() calls in the RED_PROB_MARK and
> RED_HARD_MARK cases. Can a qevent action change qdisc_pkt_len(skb) in
> between?
>
> With RED in ecn mode and an act_ct action in the qe_mark block, act_ct
> does this after a successful defragmentation:
>
> net/sched/act_ct.c:tcf_ct_act() {
>     ...
> out_clear:
>         if (defrag)
>                 qdisc_skb_cb(skb)->pkt_len = skb->len;
>         return retval;
>     ...
> }
>
> tcf_qevent_handle() returns the reassembled skb for any verdict other
> than shot or stolen. RED then adds the stale len here, while
> red_dequeue() subtracts the new length:
>
>         qdisc_qstats_backlog_dec(sch, skb);
>
> If the reassembled packet is larger, doesn't RED's u32 backlog underflow
> to nearly 2^32, which is above QDISC_MAX_BACKLOG?
>
> After that, every red_enqueue() would fail the new ceiling check even
> with an empty child. RED would drop all traffic until the qdisc is
> reset. If the stab-inflated fragment length is the larger one, the
> backlog grows instead and eventually hits the ceiling too.
>
> Before this patch, len was read after the qevents, so the two values
> matched. Could the ceiling check use qdisc_pkt_len(skb) directly, and the
> original read stay just before qdisc_enqueue()?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v2.20260926123300%40mojatatu.com