[PATCH net-next v7 0/4] net/sched: cap the accounted backlog before it can wrap

[PATCH net-next v7 0/4] net/sched: cap the accounted backlog before it can wrap

[Jamal Hadi Salim]

This series bounds the accounted backlog of the AQM qdiscs so that it
cannot wrap mod 2^32.

v1 widened the counters to u64 and was rejected; from v2 on the fix drops
at enqueue instead. The v5 posting was a single patch; at the reviewer's
request it is split into four:

  1/4  classifier refactor: cake_tcf_classify() and fq_codel_classify()
       return their result to the caller.
  2/4  cake: use a dedicated split-GSO list iterator.
  3/4  the fix: drop at enqueue once the stored backlog cannot
       represent the packet's accounted length -- the standing
       QDISC_MAX_BACKLOG ceiling for ordinary packets, a per-packet
       representability check for the larger wire estimates. Covers
       fq_codel, cake, codel, pie, fq_pie, dualpi2, RED, pfifo, plug
       and gred.
  4/4  cake: split cake_hash() into side-effect-free key extraction (on
       the unsegmented skb) and set-associative resolution (after
       admission), move the split-GSO path ahead of the resolution, and
       tag every deferred split segment.

The commit logs and the in-code comments are written for readability.

The guard half is a net fix, but the CAKE hash split is new-code net-next
material, so the series targets net-next with no Fixes: (the aggregate
wrap predates every touched qdisc).

Changes in v7:
  - 1/4: complete the fq_codel_classify() comment (Sashiko nipa v6
    main-2-1, Low). The comment listed only the terminal-action and
    no-class-ID cases; the function also returns true when no filter
    matched or when the selected class minor is above q->flows_cnt,
    and those packets are BYPASS drops, not a fallback to hashing as
    in cake. Comment-only, no code change; folded into this resend
    since the series is revised anyway.
  - 3/4: do not clamp the shared qdisc_pkt_len() (Sashiko nipa v6
    main-2-6, Medium). The clamp silently undercharged BPF
    __sk_buff.wire_len, act_police and the HTB/TBF shapers for a valid
    large GSO estimate. The initializer now stays truthful for every
    input and the overflow protection is enqueue admission: each
    affected qdisc drops a packet once the stored backlog cannot
    represent its accounted length -- the standing QDISC_MAX_BACKLOG
    ceiling covers ordinary packets, and a per-packet representability
    check covers the larger wire estimates, which a supported
    small-MSS BIG TCP aggregate can legitimately carry above 1 MiB
    while its raw length stays within GSO_MAX_SIZE. The consumers read
    the real length, and sch_handle_ingress() keeps complete pkt_segs
    metadata.
  - 4/4: tag every deferred split-GSO segment with QDISC_DROP_OVERLIMIT
    (Sashiko nipa v6 gpt-6-astra-2-5, Medium). __skb_gso_segment() stores
    SKB_GSO_CB at cb offset 32, aliasing tc_skb_cb.drop_reason, and the cb
    is copied into every segment, so tagging only the head left the rest
    with GSO scratch as their drop reason.
  - 4/4: snapshot q->config->flow_mode once with READ_ONCE() and pass it
    to both hash helpers (Sashiko nipa v6 gpt-6-astra-5-9, Low). The split
    introduced two loads with the segmentation work between them.
  - Not folded: the 3/4 intermediate-commit split-GSO limitation (4/4
    closes it; the pre-series path was already unbounded) and the
    Fixes:/stable suggestions (intentional net-next target). The gemini
    v6 TC_ACT_CONSUMED High is the v5 finding already rebutted as
    unreachable from a qdisc classify switch.

v6  rewrite as a four-patch series per Toke Høiland-Jørgensen's review of
    v5 (split the classifier refactor, the split-GSO iterator, the
    overflow guard and the cake hash split; drop the machine-written
    prose). Considered the Sashiko gemini v5 TC_ACT_CONSUMED report and
    did not fold it (unreachable; see below). Reworded the cake block
    comment that had an embedded "/*" (Simon Horman's W=1 warning) and
    reworded a comment that overstated the classifier's return cases. The
    TC_ACT_CONSUMED switch case was dropped from the final 0001 because it
    still reached __qdisc_drop(); no in-tree producer can deliver
    TC_ACT_CONSUMED to a qdisc classify switch (act_ct returns it only on
    a clsact/ingress qdisc, act_mirred only for a non-cloning ingress
    redirect, and cls_bpf/act_bpf remap an unknown verdict to UNSPEC).

v5/v4/v3/v2 history: see the v6 posting. In short, v3 adopted the stored
    backlog compare qdisc_backlog_at_max() (no double-count of the 1 MiB
    headroom, no per-enqueue 64-bit add), moved the classifier and the
    ceiling check ahead of the hash/shaper state, and added pfifo; v4
    split the hash keys from the set-associative resolution and handled
    the fraglist-GSO self-link; v2 replaced the rejected u64 widening with
    the enqueue drop; v1 widened the counters.

Jamal Hadi Salim (4):
  net/sched: cake, fq_codel: return the classifier result to the caller
  net/sched: cake: use a dedicated split-GSO list iterator
  net/sched: cap the accounted backlog before it can wrap
  net/sched: cake: hash before segmenting and reject an overlong split

 include/net/pkt_sched.h   |   1 -
 include/net/sch_generic.h |  20 +++++
 net/sched/sch_cake.c      | 184 +++++++++++++++++++++++++++-----------
 net/sched/sch_codel.c     |  16 ++--
 net/sched/sch_dualpi2.c   |   1 +
 net/sched/sch_fifo.c      |   6 ++
 net/sched/sch_fq_codel.c  |  55 +++++++++---
 net/sched/sch_fq_pie.c    |   3 +-
 net/sched/sch_gred.c      |   9 ++
 net/sched/sch_pie.c       |   3 +-
 net/sched/sch_plug.c      |   9 +-
 net/sched/sch_red.c       |  23 +++--
 12 files changed, 251 insertions(+), 79 deletions(-)

-- 
2.43.0

[PATCH net-next v7 1/4] net/sched: cake, fq_codel: return the classifier result to the caller

[Jamal Hadi Salim]

The Fix:
cake and fq_codel run an optional external classifier from their enqueue
path and then compute the flow hash inside the helper. Have the helper
return the filter's overrides (cake) or a 1-based flow index or a request
to hash (fq_codel) instead, so the caller owns the flow selection.

This is preparation for the accounting changes that follow. Packet
classification and flow selection are unchanged.

Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_cake.c     | 35 +++++++++++++++++++-----------
 net/sched/sch_fq_codel.c | 47 +++++++++++++++++++++++++++++-----------
 2 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..6432d10c992e 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1712,18 +1712,24 @@ static struct cake_tin_data *cake_select_tin(struct Qdisc *sch,
 	return &qd->tins[tin];
 }
 
-static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t,
-			 struct sk_buff *skb, int flow_mode, int *qerr)
+/* Run the qdisc's optional external classifier. Returns true when a
+ * terminal TC action consumed the packet, with the reason in *qerr;
+ * otherwise *flow and *host hold the filter's overrides (0 if none).
+ */
+static bool cake_tcf_classify(struct Qdisc *sch, struct sk_buff *skb,
+			      u16 *flow, u16 *host, int *qerr)
 {
 	struct cake_sched_data *q = qdisc_priv(sch);
 	struct tcf_proto *filter;
 	struct tcf_result res;
-	u16 flow = 0, host = 0;
 	int result;
 
+	*flow = 0;
+	*host = 0;
+
 	filter = rcu_dereference_bh(q->filter_list);
 	if (!filter)
-		goto hash;
+		return false;
 
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	result = tcf_classify_qdisc(skb, filter, &res, false);
@@ -1737,17 +1743,15 @@ static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t,
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 			fallthrough;
 		case TC_ACT_SHOT:
-			return 0;
+			return true;
 		}
 #endif
 		if (TC_H_MIN(res.classid) <= CAKE_QUEUES)
-			flow = TC_H_MIN(res.classid);
+			*flow = TC_H_MIN(res.classid);
 		if (TC_H_MAJ(res.classid) <= (CAKE_QUEUES << 16))
-			host = TC_H_MAJ(res.classid) >> 16;
+			*host = TC_H_MAJ(res.classid) >> 16;
 	}
-hash:
-	*t = cake_select_tin(sch, skb);
-	return cake_hash(*t, skb, flow_mode, flow, host) + 1;
+	return false;
 }
 
 static void cake_reconfigure(struct Qdisc *sch);
@@ -1758,20 +1762,25 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	u32 idx, tin, prev_qlen, prev_backlog, drop_id;
 	struct cake_sched_data *q = qdisc_priv(sch);
 	int len = qdisc_pkt_len(skb), ret;
+	u16 flow_override, host_override;
 	struct sk_buff *ack = NULL;
 	ktime_t now = ktime_get();
 	struct cake_tin_data *b;
 	struct cake_flow *flow;
 	bool same_flow = false;
 
-	/* choose flow to insert into */
-	idx = cake_classify(sch, &b, skb, q->config->flow_mode, &ret);
-	if (idx == 0) {
+	/* Classify first: a terminal TC action consumes the packet. */
+	if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &ret)) {
 		if (ret & __NET_XMIT_BYPASS)
 			qdisc_qstats_drop(sch);
 		__qdisc_drop(skb, to_free);
 		return ret;
 	}
+
+	/* choose flow to insert into */
+	b = cake_select_tin(sch, skb);
+	idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
+			host_override) + 1;
 	tin = (u32)(b - q->tins);
 	idx--;
 	flow = &b->flows[idx];
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index e6c87a32950f..011b8fc54d84 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -73,22 +73,38 @@ static unsigned int fq_codel_hash(const struct fq_codel_sched_data *q,
 	return reciprocal_scale(skb_get_hash(skb), q->flows_cnt);
 }
 
-static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
-				      int *qerr)
+/* Run the qdisc's optional external classifier. Returns true when the
+ * caller must drop the packet, with the reason in *qerr: a terminal TC
+ * action consumed it, or no valid class in 1..q->flows_cnt was
+ * selected (no filter matched, the result carried no class ID, or its
+ * minor was above q->flows_cnt). Unlike cake, a packet with no valid
+ * class is dropped here (a BYPASS drop) rather than falling back to
+ * hashing. Otherwise *idx is the 1-based flow index, or *do_hash asks
+ * the caller to use fq_codel_hash().
+ */
+static bool fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
+			      unsigned int *idx, bool *do_hash, int *qerr)
 {
 	struct fq_codel_sched_data *q = qdisc_priv(sch);
 	struct tcf_proto *filter;
 	struct tcf_result res;
 	int result;
 
+	*idx = 0;
+	*do_hash = false;
+
 	if (TC_H_MAJ(skb->priority) == sch->handle &&
 	    TC_H_MIN(skb->priority) > 0 &&
-	    TC_H_MIN(skb->priority) <= q->flows_cnt)
-		return TC_H_MIN(skb->priority);
+	    TC_H_MIN(skb->priority) <= q->flows_cnt) {
+		*idx = TC_H_MIN(skb->priority);
+		return false;
+	}
 
 	filter = rcu_dereference_bh(q->filter_list);
-	if (!filter)
-		return fq_codel_hash(q, skb) + 1;
+	if (!filter) {
+		*do_hash = true;
+		return false;
+	}
 
 	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	result = tcf_classify_qdisc(skb, filter, &res, false);
@@ -101,13 +117,16 @@ static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 			fallthrough;
 		case TC_ACT_SHOT:
-			return 0;
+			return true;
 		}
 #endif
-		if (TC_H_MIN(res.classid) <= q->flows_cnt)
-			return TC_H_MIN(res.classid);
+		if (TC_H_MIN(res.classid) > 0 &&
+		    TC_H_MIN(res.classid) <= q->flows_cnt) {
+			*idx = TC_H_MIN(res.classid);
+			return false;
+		}
 	}
-	return 0;
+	return true;
 }
 
 /* helper functions : might be changed when/if skb use a standard list_head */
@@ -188,17 +207,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct fq_codel_sched_data *q = qdisc_priv(sch);
 	unsigned int idx, prev_backlog, prev_qlen;
 	struct fq_codel_flow *flow;
-	int ret;
+	bool do_hash = false;
 	unsigned int pkt_len;
 	bool memory_limited;
+	int ret;
 
-	idx = fq_codel_classify(skb, sch, &ret);
-	if (idx == 0) {
+	if (fq_codel_classify(skb, sch, &idx, &do_hash, &ret)) {
 		if (ret & __NET_XMIT_BYPASS)
 			qdisc_qstats_drop(sch);
 		__qdisc_drop(skb, to_free);
 		return ret;
 	}
+	if (do_hash)
+		idx = fq_codel_hash(q, skb) + 1;
 	idx--;
 
 	codel_set_enqueue_time(skb);
-- 
2.43.0

Re: [PATCH net-next v7 1/4] net/sched: cake, fq_codel: return the classifier result to the caller

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> The Fix:
> cake and fq_codel run an optional external classifier from their enqueue
> path and then compute the flow hash inside the helper. Have the helper
> return the filter's overrides (cake) or a 1-based flow index or a request
> to hash (fq_codel) instead, so the caller owns the flow selection.
>
> This is preparation for the accounting changes that follow. Packet
> classification and flow selection are unchanged.

It's not clear to me why this change is needed at all for fq_codel?
fq_codel_hash just does a reciprocal_scale() of the skb hash, it doesn't
mutate any qdisc state. So why can't fq_codel_classify not just stay as
it is?

> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>  net/sched/sch_cake.c     | 35 +++++++++++++++++++-----------
>  net/sched/sch_fq_codel.c | 47 +++++++++++++++++++++++++++++-----------
>  2 files changed, 56 insertions(+), 26 deletions(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e7..6432d10c992e 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1712,18 +1712,24 @@ static struct cake_tin_data *cake_select_tin(struct Qdisc *sch,
>  	return &qd->tins[tin];
>  }
>  
> -static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t,
> -			 struct sk_buff *skb, int flow_mode, int *qerr)
> +/* Run the qdisc's optional external classifier. Returns true when a
> + * terminal TC action consumed the packet, with the reason in *qerr;
> + * otherwise *flow and *host hold the filter's overrides (0 if none).
> + */

This comment is unnecessary, and just repeating what the code does.
Please drop.

> +static bool cake_tcf_classify(struct Qdisc *sch, struct sk_buff *skb,
> +			      u16 *flow, u16 *host, int *qerr)
>  {
>  	struct cake_sched_data *q = qdisc_priv(sch);
>  	struct tcf_proto *filter;
>  	struct tcf_result res;
> -	u16 flow = 0, host = 0;
>  	int result;
>  
> +	*flow = 0;
> +	*host = 0;
> +
>  	filter = rcu_dereference_bh(q->filter_list);
>  	if (!filter)
> -		goto hash;
> +		return false;
>  
>  	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>  	result = tcf_classify_qdisc(skb, filter, &res, false);
> @@ -1737,17 +1743,15 @@ static u32 cake_classify(struct Qdisc *sch, struct cake_tin_data **t,
>  			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
>  			fallthrough;
>  		case TC_ACT_SHOT:
> -			return 0;
> +			return true;
>  		}
>  #endif
>  		if (TC_H_MIN(res.classid) <= CAKE_QUEUES)
> -			flow = TC_H_MIN(res.classid);
> +			*flow = TC_H_MIN(res.classid);
>  		if (TC_H_MAJ(res.classid) <= (CAKE_QUEUES << 16))
> -			host = TC_H_MAJ(res.classid) >> 16;
> +			*host = TC_H_MAJ(res.classid) >> 16;
>  	}
> -hash:
> -	*t = cake_select_tin(sch, skb);
> -	return cake_hash(*t, skb, flow_mode, flow, host) + 1;
> +	return false;
>  }
>  
>  static void cake_reconfigure(struct Qdisc *sch);
> @@ -1758,20 +1762,25 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	u32 idx, tin, prev_qlen, prev_backlog, drop_id;
>  	struct cake_sched_data *q = qdisc_priv(sch);
>  	int len = qdisc_pkt_len(skb), ret;
> +	u16 flow_override, host_override;
>  	struct sk_buff *ack = NULL;
>  	ktime_t now = ktime_get();
>  	struct cake_tin_data *b;
>  	struct cake_flow *flow;
>  	bool same_flow = false;
>  
> -	/* choose flow to insert into */
> -	idx = cake_classify(sch, &b, skb, q->config->flow_mode, &ret);
> -	if (idx == 0) {
> +	/* Classify first: a terminal TC action consumes the packet. */
> +	if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &ret)) {
>  		if (ret & __NET_XMIT_BYPASS)
>  			qdisc_qstats_drop(sch);
>  		__qdisc_drop(skb, to_free);
>  		return ret;
>  	}
> +
> +	/* choose flow to insert into */
> +	b = cake_select_tin(sch, skb);
> +	idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
> +			host_override) + 1;
>  	tin = (u32)(b - q->tins);
>  	idx--;
>  	flow = &b->flows[idx];
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index e6c87a32950f..011b8fc54d84 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -73,22 +73,38 @@ static unsigned int fq_codel_hash(const struct fq_codel_sched_data *q,
>  	return reciprocal_scale(skb_get_hash(skb), q->flows_cnt);
>  }
>  
> -static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
> -				      int *qerr)
> +/* Run the qdisc's optional external classifier. Returns true when the
> + * caller must drop the packet, with the reason in *qerr: a terminal TC
> + * action consumed it, or no valid class in 1..q->flows_cnt was
> + * selected (no filter matched, the result carried no class ID, or its
> + * minor was above q->flows_cnt). Unlike cake, a packet with no valid
> + * class is dropped here (a BYPASS drop) rather than falling back to
> + * hashing. Otherwise *idx is the 1-based flow index, or *do_hash asks
> + * the caller to use fq_codel_hash().
> + */
> +static bool fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
> +			      unsigned int *idx, bool *do_hash, int *qerr)
>  {
>  	struct fq_codel_sched_data *q = qdisc_priv(sch);
>  	struct tcf_proto *filter;
>  	struct tcf_result res;
>  	int result;
>  
> +	*idx = 0;
> +	*do_hash = false;
> +
>  	if (TC_H_MAJ(skb->priority) == sch->handle &&
>  	    TC_H_MIN(skb->priority) > 0 &&
> -	    TC_H_MIN(skb->priority) <= q->flows_cnt)
> -		return TC_H_MIN(skb->priority);
> +	    TC_H_MIN(skb->priority) <= q->flows_cnt) {
> +		*idx = TC_H_MIN(skb->priority);
> +		return false;
> +	}
>  
>  	filter = rcu_dereference_bh(q->filter_list);
> -	if (!filter)
> -		return fq_codel_hash(q, skb) + 1;
> +	if (!filter) {
> +		*do_hash = true;
> +		return false;
> +	}
>  
>  	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
>  	result = tcf_classify_qdisc(skb, filter, &res, false);
> @@ -101,13 +117,16 @@ static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
>  			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
>  			fallthrough;
>  		case TC_ACT_SHOT:
> -			return 0;
> +			return true;
>  		}
>  #endif
> -		if (TC_H_MIN(res.classid) <= q->flows_cnt)
> -			return TC_H_MIN(res.classid);
> +		if (TC_H_MIN(res.classid) > 0 &&
> +		    TC_H_MIN(res.classid) <= q->flows_cnt) {
> +			*idx = TC_H_MIN(res.classid);
> +			return false;
> +		}
>  	}
> -	return 0;
> +	return true;
>  }
>  
>  /* helper functions : might be changed when/if skb use a standard list_head */
> @@ -188,17 +207,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	struct fq_codel_sched_data *q = qdisc_priv(sch);
>  	unsigned int idx, prev_backlog, prev_qlen;
>  	struct fq_codel_flow *flow;
> -	int ret;
> +	bool do_hash = false;
>  	unsigned int pkt_len;
>  	bool memory_limited;
> +	int ret;
>  
> -	idx = fq_codel_classify(skb, sch, &ret);
> -	if (idx == 0) {
> +	if (fq_codel_classify(skb, sch, &idx, &do_hash, &ret)) {
>  		if (ret & __NET_XMIT_BYPASS)
>  			qdisc_qstats_drop(sch);
>  		__qdisc_drop(skb, to_free);
>  		return ret;
>  	}
> +	if (do_hash)
> +		idx = fq_codel_hash(q, skb) + 1;
>  	idx--;
>  
>  	codel_set_enqueue_time(skb);
> -- 
> 2.43.0

Re: [PATCH net-next v7 1/4] net/sched: cake, fq_codel: return the classifier result to the caller

[Jamal Hadi Salim]

On Mon, Oct 19, 2026 at 2:11 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > The Fix:
> > cake and fq_codel run an optional external classifier from their enqueue
> > path and then compute the flow hash inside the helper. Have the helper
> > return the filter's overrides (cake) or a 1-based flow index or a request
> > to hash (fq_codel) instead, so the caller owns the flow selection.
> >
> > This is preparation for the accounting changes that follow. Packet
> > classification and flow selection are unchanged.
>
> It's not clear to me why this change is needed at all for fq_codel?
> fq_codel_hash just does a reciprocal_scale() of the skb hash, it doesn't
> mutate any qdisc state. So why can't fq_codel_classify not just stay as
> it is?

It is not functionaly needed for fq_codel, it is more because these
two are cousins and simplifies subsequent patches.
Note in patch 4 we force the same semantics on cake as in fq_codel
(classifier decides the drop, the caller owns flow selection). Keeping
fq_codel's classify the same makes patch 3 hunks in the two files
similar. There is another reason: the old helper encoded "drop" as a
magic return and carried the 1-based +1 arithmetic on every return
path.
In any case, unless you feel strongly i would rather keep it.

cheers,
jamal

Re: [PATCH net-next v7 1/4] net/sched: cake, fq_codel: return the classifier result to the caller

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> On Mon, Oct 19, 2026 at 2:11 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>>
>> > The Fix:
>> > cake and fq_codel run an optional external classifier from their enqueue
>> > path and then compute the flow hash inside the helper. Have the helper
>> > return the filter's overrides (cake) or a 1-based flow index or a request
>> > to hash (fq_codel) instead, so the caller owns the flow selection.
>> >
>> > This is preparation for the accounting changes that follow. Packet
>> > classification and flow selection are unchanged.
>>
>> It's not clear to me why this change is needed at all for fq_codel?
>> fq_codel_hash just does a reciprocal_scale() of the skb hash, it doesn't
>> mutate any qdisc state. So why can't fq_codel_classify not just stay as
>> it is?
>
> It is not functionaly needed for fq_codel, it is more because these
> two are cousins and simplifies subsequent patches.
> Note in patch 4 we force the same semantics on cake as in fq_codel
> (classifier decides the drop, the caller owns flow selection). Keeping
> fq_codel's classify the same makes patch 3 hunks in the two files
> similar. There is another reason: the old helper encoded "drop" as a
> magic return and carried the 1-based +1 arithmetic on every return
> path.

But you're not making the same change to fq_pie, although its classify
function is line-for-line the same as fq_codel. I think it's better to
keep those two the same; you can add the new check to fq_codel between
the 'idx--' and 'codel_set_enqueue_time()' lines just fine.

> In any case, unless you feel strongly i would rather keep it.

Well, see above :)

-Toke

Re: [PATCH net-next v7 1/4] net/sched: cake, fq_codel: return the classifier result to the caller

[Jamal Hadi Salim]

On Tue, Oct 20, 2026 at 4:58 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > On Mon, Oct 19, 2026 at 2:11 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> >>
> >> Jamal Hadi Salim <jhs@mojatatu.com> writes:
> >>
> >> > The Fix:
> >> > cake and fq_codel run an optional external classifier from their enqueue
> >> > path and then compute the flow hash inside the helper. Have the helper
> >> > return the filter's overrides (cake) or a 1-based flow index or a request
> >> > to hash (fq_codel) instead, so the caller owns the flow selection.
> >> >
> >> > This is preparation for the accounting changes that follow. Packet
> >> > classification and flow selection are unchanged.
> >>
> >> It's not clear to me why this change is needed at all for fq_codel?
> >> fq_codel_hash just does a reciprocal_scale() of the skb hash, it doesn't
> >> mutate any qdisc state. So why can't fq_codel_classify not just stay as
> >> it is?
> >
> > It is not functionaly needed for fq_codel, it is more because these
> > two are cousins and simplifies subsequent patches.
> > Note in patch 4 we force the same semantics on cake as in fq_codel
> > (classifier decides the drop, the caller owns flow selection). Keeping
> > fq_codel's classify the same makes patch 3 hunks in the two files
> > similar. There is another reason: the old helper encoded "drop" as a
> > magic return and carried the 1-based +1 arithmetic on every return
> > path.
>
> But you're not making the same change to fq_pie, although its classify
> function is line-for-line the same as fq_codel. I think it's better to
> keep those two the same; you can add the new check to fq_codel between
> the 'idx--' and 'codel_set_enqueue_time()' lines just fine.
>
> > In any case, unless you feel strongly i would rather keep it.
>
> Well, see above :)

Ok, i will drop it ;->

cheers,
jamal
> -Toke

[PATCH net-next v7 2/4] net/sched: cake: use a dedicated split-GSO list iterator

[Jamal Hadi Salim]

skb_list_walk_safe(segs, segs, nskb) reuses the list head as the walk
cursor, so segs no longer names the segment list once the walk finishes.
Use a separate seg cursor to keep the head and the walker distinct. No
behaviour change.

Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_cake.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 6432d10c992e..0d13c4b0bfe3 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1810,7 +1810,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;
 
@@ -1818,19 +1818,19 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		if (IS_ERR_OR_NULL(segs))
 			return qdisc_drop(skb, sch, to_free);
 
-		skb_list_walk_safe(segs, segs, nskb) {
-			skb_mark_not_on_list(segs);
-			qdisc_skb_cb(segs)->pkt_len = segs->len;
-			qdisc_skb_cb(segs)->pkt_segs = 1;
-			cobalt_set_enqueue_time(segs, now);
-			get_cobalt_cb(segs)->adjusted_len = cake_overhead(q,
-									  segs);
-			flow_queue_add(flow, segs);
+		skb_list_walk_safe(segs, seg, nskb) {
+			skb_mark_not_on_list(seg);
+			qdisc_skb_cb(seg)->pkt_len = seg->len;
+			qdisc_skb_cb(seg)->pkt_segs = 1;
+			cobalt_set_enqueue_time(seg, now);
+			get_cobalt_cb(seg)->adjusted_len = cake_overhead(q,
+									 seg);
+			flow_queue_add(flow, seg);
 
 			qdisc_qlen_inc(sch);
 			numsegs++;
-			slen += segs->len;
-			q->buffer_used += segs->truesize;
+			q->buffer_used += seg->truesize;
+			slen += seg->len;
 			WRITE_ONCE(b->packets, b->packets + 1);
 		}
 
-- 
2.43.0

Re: [PATCH net-next v7 2/4] net/sched: cake: use a dedicated split-GSO list iterator

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> skb_list_walk_safe(segs, segs, nskb) reuses the list head as the walk
> cursor, so segs no longer names the segment list once the walk finishes.
> Use a separate seg cursor to keep the head and the walker distinct. No
> behaviour change.
>
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

[PATCH net-next v7 3/4] net/sched: cap the accounted backlog before it can wrap

[Jamal Hadi Salim]

This is a followup of commit 709f34f7c28d ("net/sched: fq: add overflow
bounds to quantum and initial quantum") as reported by Sashiko.

The issue: fq_codel, cake, codel, pie, fq_pie, dualpi2, RED, pfifo, plug
and (non-WRED) gred account every enqueued packet's stab-adjusted length
into the 32-bit sch->qstats.backlog, which the AQM logic reads back (for
non-WRED gred it is the reported stats and the dequeue-side subtractions
that go wrong instead). A crafted tc size table inflates qdisc_pkt_len()
up to QDISC_PKT_LEN_MAX (1 MiB), and a dodgy GSO skb can account even more
through qdisc_pkt_len_segs_init(), so a few thousand packets wrap the
counter mod 2^32. The AQM then reads a small backlog and makes the wrong
drop decision, and the dequeue-side subtractions leave it corrupt. In
fq_codel a wrapped per-flow backlog can also make fq_codel_drop()'s scan
pick an empty flow, dereferencing a NULL flow->head.

The Fix: drop at enqueue once the stored backlog cannot represent the
incoming packet's accounted length. Ordinary packets (qdisc_pkt_len() at
most QDISC_PKT_LEN_MAX) are covered by a standing ceiling: drop once the
backlog is within QDISC_MAX_BACKLOG (U32_MAX - QDISC_PKT_LEN_MAX) of the
wrap point, the largest backlog one more 1 MiB packet cannot wrap, so
there is no per-enqueue 64-bit add. A larger wire estimate is checked
against the space that actually remains (len > U32_MAX - backlog):
qdisc_pkt_len_segs_init() adds the headers replicated on every output
segment, so a supported small-MSS BIG TCP aggregate can carry a wire
estimate above 1 MiB while its raw length stays within GSO_MAX_SIZE, and
such traffic must pass on an empty queue. qdisc_pkt_len() itself stays
truthful for every input: it is not clamped and the initializer keeps
succeeding, so BPF __sk_buff.wire_len, act_police and the HTB/TBF shapers
read the real length, and sch_handle_ingress() -- which ignores the
initializer's return value and reads pkt_segs for packet statistics --
keeps complete metadata. Cover sch_plug and non-WRED sch_gred too: plug
admits on skb->len while qdisc_enqueue_tail() adds qdisc_pkt_len(), and
gred bounds each virtual queue but adds every VQ into the one aggregate.
pfifo is included because RED reads a grafted child's backlog into
red_calc_qavg(); it can be reached standalone or as the default child of
SFB. In RED, account the packet to the aggregate before handing it to the
child, because a duplicating netem child re-enqueues its clone at the root
while the stored backlog is still un-updated -- one guard check must not
authorize two additions.

The guard bounds each qdisc's own enqueue path: a child that segments a
GSO skb (tbf, netem) can still grow a parent's aggregate through
qdisc_tree_reduce_backlog()'s unclamped byte delta, which is left to a
separate change.

Conditions to recreate the bug:
  ip tuntap add tun0 mode tun
  ip link set tun0 txqueuelen 32 up
  ip addr add 10.99.0.1/24 dev tun0
  tc qdisc add dev tun0 root handle 1: stab overhead 2000000000 \
      fq_codel flows 1 limit 20000 ecn
  # hold the tun fd open without reading so the backlog persists, then
  # send 6000 packets: the counter wraps and the unfixed kernel reports
  # a wrapped backlog, while the fixed one drops at the ceiling.

Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
Link: https://lore.kernel.org/netdev/CANn89iLfMJV7ancKH1Gjzzm7ZUG-gKcrczkJjJEWN5sCuWd-ug@mail.gmail.com/
Link: https://lore.kernel.org/netdev/CANn89i+GOFH_8g+vSV0jUj0aBqLYVWuFOrHuDggZu9kPC_m6tQ@mail.gmail.com/
Link: https://lore.kernel.org/netdev/87a4oottcj.fsf@toke.dk/
Link: https://lore.kernel.org/netdev/87pkxks68f.fsf@toke.dk/
Link: https://lore.kernel.org/netdev/179231932946.3135.7497478990186385688@kernel.org/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v6 -> v7: qdisc_pkt_len_segs_init() no longer clamps the shared
qdisc_pkt_len(); it stays truthful for every input, including the wire
estimates above QDISC_PKT_LEN_MAX that a small-MSS BIG TCP aggregate
legitimately carries. The overflow protection is enqueue admission
instead: each affected qdisc drops a packet once the stored backlog
cannot represent its accounted length -- the standing QDISC_MAX_BACKLOG
ceiling covers ordinary packets, and a per-packet representability check
covers the larger wire estimates -- so BPF __sk_buff.wire_len,
act_police and the HTB/TBF shapers read the real length and the ingress
path keeps complete pkt_segs metadata. (Sashiko nipa v6 review, main-2-6)
---
 include/net/pkt_sched.h   |  1 -
 include/net/sch_generic.h | 20 ++++++++++++++++++++
 net/sched/sch_cake.c      |  7 +++++++
 net/sched/sch_codel.c     | 16 +++++++++-------
 net/sched/sch_dualpi2.c   |  1 +
 net/sched/sch_fifo.c      |  6 ++++++
 net/sched/sch_fq_codel.c  |  8 ++++++++
 net/sched/sch_fq_pie.c    |  3 ++-
 net/sched/sch_gred.c      |  9 +++++++++
 net/sched/sch_pie.c       |  3 ++-
 net/sched/sch_plug.c      |  9 ++++++++-
 net/sched/sch_red.c       | 23 ++++++++++++++++++-----
 12 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 90d3e7943b19..18a419cd9d94 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -12,7 +12,6 @@
 
 #define DEFAULT_TX_QUEUE_LEN	1000
 #define STAB_SIZE_LOG_MAX	30
-#define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */
 
 struct qdisc_walker {
 	int	stop;
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 1acaadb3e2cf..a5a6bdf55e0a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -915,6 +915,26 @@ static inline unsigned int qdisc_pkt_len(const struct sk_buff *skb)
 	return qdisc_skb_cb(skb)->pkt_len;
 }
 
+#define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */
+
+/* Largest backlog that still has room for one more 1 MiB packet. */
+#define QDISC_MAX_BACKLOG	(U32_MAX - QDISC_PKT_LEN_MAX)
+
+/* Would this packet's accounted length wrap sch->qstats.backlog?
+ * Packets up to QDISC_PKT_LEN_MAX fit below QDISC_MAX_BACKLOG; GSO
+ * can make the length larger, so compare against the real headroom.
+ */
+static inline bool qdisc_backlog_overflow(const struct Qdisc *sch,
+					  const struct sk_buff *skb)
+{
+	u32 backlog = READ_ONCE(sch->qstats.backlog);
+	u32 len = qdisc_pkt_len(skb);
+
+	if (likely(len <= QDISC_PKT_LEN_MAX))
+		return backlog > QDISC_MAX_BACKLOG;
+	return len > U32_MAX - backlog;
+}
+
 static inline unsigned int qdisc_pkt_segs(const struct sk_buff *skb)
 {
 	u32 pkt_segs = qdisc_skb_cb(skb)->pkt_segs;
diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 0d13c4b0bfe3..91c33295b30b 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1777,6 +1777,13 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		return ret;
 	}
 
+	/* Drop before touching the hash or shaper state. */
+	if (unlikely(qdisc_backlog_overflow(sch, skb))) {
+		qdisc_qstats_overlimit(sch);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	/* choose flow to insert into */
 	b = cake_select_tin(sch, skb);
 	idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
diff --git a/net/sched/sch_codel.c b/net/sched/sch_codel.c
index a1ba2e355f2d..78c5d72d6406 100644
--- a/net/sched/sch_codel.c
+++ b/net/sched/sch_codel.c
@@ -116,15 +116,17 @@ static struct sk_buff *codel_peek(struct Qdisc *sch)
 static int codel_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			       struct sk_buff **to_free)
 {
-	struct codel_sched_data *q;
+	struct codel_sched_data *q = qdisc_priv(sch);
 
-	if (likely(qdisc_qlen(sch) < sch->limit)) {
-		codel_set_enqueue_time(skb);
-		return qdisc_enqueue_tail(skb, sch);
+	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely(qdisc_backlog_overflow(sch, skb))) {
+		WRITE_ONCE(q->drop_overlimit, q->drop_overlimit + 1);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
 	}
-	q = qdisc_priv(sch);
-	WRITE_ONCE(q->drop_overlimit, q->drop_overlimit + 1);
-	return qdisc_drop_reason(skb, sch, to_free, QDISC_DROP_OVERLIMIT);
+
+	codel_set_enqueue_time(skb);
+	return qdisc_enqueue_tail(skb, sch);
 }
 
 static const struct nla_policy codel_policy[TCA_CODEL_MAX + 1] = {
diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
index 4947def7c49e..7331f0ec1c8f 100644
--- a/net/sched/sch_dualpi2.c
+++ b/net/sched/sch_dualpi2.c
@@ -392,6 +392,7 @@ static int dualpi2_enqueue_skb(struct sk_buff *skb, struct Qdisc *sch,
 	struct dualpi2_skb_cb *cb;
 
 	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely(qdisc_backlog_overflow(sch, skb)) ||
 	    unlikely((u64)q->memory_used + skb->truesize > q->memory_limit)) {
 		qdisc_qstats_overlimit(sch);
 		if (skb_in_l_queue(skb))
diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index 1b6388d50967..16c8d4f74e7e 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c
@@ -29,6 +29,9 @@ static int bfifo_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 static int pfifo_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			 struct sk_buff **to_free)
 {
+	if (unlikely(qdisc_backlog_overflow(sch, skb)))
+		return qdisc_drop(skb, sch, to_free);
+
 	if (likely(sch->q.qlen < READ_ONCE(sch->limit)))
 		return qdisc_enqueue_tail(skb, sch);
 
@@ -43,6 +46,9 @@ static int pfifo_tail_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	if (unlikely(READ_ONCE(sch->limit) == 0))
 		return qdisc_drop(skb, sch, to_free);
 
+	if (unlikely(qdisc_backlog_overflow(sch, skb)))
+		return qdisc_drop(skb, sch, to_free);
+
 	if (likely(sch->q.qlen < READ_ONCE(sch->limit)))
 		return qdisc_enqueue_tail(skb, sch);
 
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 011b8fc54d84..f708ae56b7e3 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -218,6 +218,14 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 		__qdisc_drop(skb, to_free);
 		return ret;
 	}
+
+	/* Drop before hashing so a dropped packet cannot create a flow. */
+	if (unlikely(qdisc_backlog_overflow(sch, skb))) {
+		q->drop_overlimit++;
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	if (do_hash)
 		idx = fq_codel_hash(q, skb) + 1;
 	idx--;
diff --git a/net/sched/sch_fq_pie.c b/net/sched/sch_fq_pie.c
index 5982847df8f8..9f927a08b7bc 100644
--- a/net/sched/sch_fq_pie.c
+++ b/net/sched/sch_fq_pie.c
@@ -155,7 +155,8 @@ static int fq_pie_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	memory_limited = q->memory_usage > q->memory_limit + skb->truesize;
 
 	/* Checks if the qdisc is full */
-	if (unlikely(qdisc_qlen(sch) >= sch->limit)) {
+	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely(qdisc_backlog_overflow(sch, skb))) {
 		q->stats.overlimit++;
 		goto out;
 	} else if (unlikely(memory_limited)) {
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index f04f425c6c44..bedb945d3b67 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -170,6 +170,15 @@ static int gred_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	unsigned long qavg = 0;
 	u16 dp = tc_index_to_dp(skb);
 
+	/* Per-VQ limits don't bound the total: every VQ adds into the
+	 * same u32 backlog, so check it before picking a VQ.
+	 */
+	if (unlikely(qdisc_backlog_overflow(sch, skb))) {
+		qdisc_qstats_overlimit(sch);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	if (dp >= t->DPs || (q = t->tab[dp]) == NULL) {
 		dp = t->def;
 
diff --git a/net/sched/sch_pie.c b/net/sched/sch_pie.c
index 3b7863ffd284..2a5fc493a7e5 100644
--- a/net/sched/sch_pie.c
+++ b/net/sched/sch_pie.c
@@ -89,7 +89,8 @@ static int pie_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	struct pie_sched_data *q = qdisc_priv(sch);
 	bool enqueue = false;
 
-	if (unlikely(qdisc_qlen(sch) >= sch->limit)) {
+	if (unlikely(qdisc_qlen(sch) >= sch->limit) ||
+	    unlikely(qdisc_backlog_overflow(sch, skb))) {
 		WRITE_ONCE(q->stats.overlimit, q->stats.overlimit + 1);
 		goto out;
 	}
diff --git a/net/sched/sch_plug.c b/net/sched/sch_plug.c
index b60ddfee6a68..c90b5ca60bc0 100644
--- a/net/sched/sch_plug.c
+++ b/net/sched/sch_plug.c
@@ -89,7 +89,14 @@ static int plug_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 {
 	struct plug_sched_data *q = qdisc_priv(sch);
 
-	if (likely((u64)sch->qstats.backlog + skb->len <= q->limit)) {
+	if (unlikely(qdisc_backlog_overflow(sch, skb))) {
+		qdisc_qstats_overlimit(sch);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
+	if (likely((u64)sch->qstats.backlog + qdisc_pkt_len(skb) <=
+		   q->limit)) {
 		if (!q->unplug_indefinite)
 			q->pkts_current_epoch++;
 		return qdisc_enqueue_tail(skb, sch);
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index d7598214270b..72990d0db4fd 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -76,6 +76,12 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	unsigned int len;
 	int ret;
 
+	if (unlikely(qdisc_backlog_overflow(sch, skb))) {
+		qdisc_qstats_overlimit(sch);
+		return qdisc_drop_reason(skb, sch, to_free,
+					 QDISC_DROP_OVERLIMIT);
+	}
+
 	q->vars.qavg = red_calc_qavg(&q->parms,
 				     &q->vars,
 				     child->qstats.backlog);
@@ -136,14 +142,21 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 
 	len = qdisc_pkt_len(skb);
+	/* Reserve the length before the child sees the packet: netem
+	 * can re-enqueue a clone here before the backlog is updated.
+	 * Undo it if the child drops the packet.
+	 */
+	qstats_backlog_add(sch, len);
 	ret = qdisc_enqueue(skb, child, to_free);
 	if (likely(ret == NET_XMIT_SUCCESS)) {
-		qstats_backlog_add(sch, len);
 		qdisc_qlen_inc(sch);
-	} else if (net_xmit_drop_count(ret)) {
-		WRITE_ONCE(q->stats.pdrop,
-			   q->stats.pdrop + 1);
-		qdisc_qstats_drop(sch);
+	} else {
+		qstats_backlog_sub(sch, len);
+		if (net_xmit_drop_count(ret)) {
+			WRITE_ONCE(q->stats.pdrop,
+				   q->stats.pdrop + 1);
+			qdisc_qstats_drop(sch);
+		}
 	}
 	return ret;
 
-- 
2.43.0

Re: [PATCH net-next v7 3/4] net/sched: cap the accounted backlog before it can wrap

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> This is a followup of commit 709f34f7c28d ("net/sched: fq: add overflow
> bounds to quantum and initial quantum") as reported by Sashiko.

The rest of this commit message is somewhat obtuse - too much run-on
information. I think it can be pretty easily fixed by a couple of
rearrangements and paragraph breaks, like this:

> The issue: fq_codel, cake, codel, pie, fq_pie, dualpi2, RED, pfifo,
> plug and (non-WRED) gred account every enqueued packet's stab-adjusted
> length into the 32-bit sch->qstats.backlog. A crafted tc size table
> inflates qdisc_pkt_len() up to QDISC_PKT_LEN_MAX (1 MiB), and a dodgy
> GSO skb can account even more through qdisc_pkt_len_segs_init(), so a
> few thousand packets wrap the counter mod 2^32.
>
> The AQM then reads a small backlog and makes the wrong drop decision,
> and the dequeue-side subtractions leave it corrupt (for non-WRED gred
> it is the reported stats and the dequeue-side subtractions that go
> wrong instead). In fq_codel a wrapped per-flow backlog can also make
> fq_codel_drop()'s scan pick an empty flow, dereferencing a NULL
> flow->head.
>
> The Fix: drop at enqueue once the stored backlog cannot represent the
> incoming packet's accounted length.
>
> Ordinary packets (qdisc_pkt_len() at most QDISC_PKT_LEN_MAX) are
> covered by a standing ceiling: drop once the backlog is within
> QDISC_MAX_BACKLOG (U32_MAX - QDISC_PKT_LEN_MAX) of the wrap point, the
> largest backlog one more 1 MiB packet cannot wrap, so there is no
> per-enqueue 64-bit add.
>
> A larger wire estimate is checked against the space that actually
> remains (len > U32_MAX - backlog): qdisc_pkt_len_segs_init() adds the
> headers replicated on every output segment, so a supported small-MSS
> BIG TCP aggregate can carry a wire estimate above 1 MiB while its raw
> length stays within GSO_MAX_SIZE, and such traffic must pass on an
> empty queue.
>
> qdisc_pkt_len() itself stays truthful for every input: it is not
> clamped and the initializer keeps succeeding, so BPF
> __sk_buff.wire_len, act_police and the HTB/TBF shapers read the real
> length, and sch_handle_ingress() -- which ignores the initializer's
> return value and reads pkt_segs for packet statistics -- keeps
> complete metadata.
>
> Cover sch_plug and non-WRED sch_gred too: plug admits on skb->len
> while qdisc_enqueue_tail() adds qdisc_pkt_len(), and gred bounds each
> virtual queue but adds every VQ into the one aggregate. pfifo is
> included because RED reads a grafted child's backlog into
> red_calc_qavg(); it can be reached standalone or as the default child
> of SFB. In RED, account the packet to the aggregate before handing it
> to the child, because a duplicating netem child re-enqueues its clone
> at the root while the stored backlog is still un-updated -- one guard
> check must not authorize two additions.
>
> The guard bounds each qdisc's own enqueue path: a child that segments a
> GSO skb (tbf, netem) can still grow a parent's aggregate through
> qdisc_tree_reduce_backlog()'s unclamped byte delta, which is left to a
> separate change.
>
> Conditions to recreate the bug:
>   ip tuntap add tun0 mode tun
>   ip link set tun0 txqueuelen 32 up
>   ip addr add 10.99.0.1/24 dev tun0
>   tc qdisc add dev tun0 root handle 1: stab overhead 2000000000 \
>       fq_codel flows 1 limit 20000 ecn
>   # hold the tun fd open without reading so the backlog persists, then
>   # send 6000 packets: the counter wraps and the unfixed kernel reports
>   # a wrapped backlog, while the fixed one drops at the ceiling.
>
> Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
> Link: https://lore.kernel.org/netdev/CANn89iLfMJV7ancKH1Gjzzm7ZUG-gKcrczkJjJEWN5sCuWd-ug@mail.gmail.com/
> Link: https://lore.kernel.org/netdev/CANn89i+GOFH_8g+vSV0jUj0aBqLYVWuFOrHuDggZu9kPC_m6tQ@mail.gmail.com/
> Link: https://lore.kernel.org/netdev/87a4oottcj.fsf@toke.dk/
> Link: https://lore.kernel.org/netdev/87pkxks68f.fsf@toke.dk/
> Link: https://lore.kernel.org/netdev/179231932946.3135.7497478990186385688@kernel.org/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

With the above rewording:

Reviewed-by: Toke Høiland-Jørgensen <toke@toke.dk>

Re: [PATCH net-next v7 3/4] net/sched: cap the accounted backlog before it can wrap

[Jamal Hadi Salim]

On Mon, Oct 19, 2026 at 2:20 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > This is a followup of commit 709f34f7c28d ("net/sched: fq: add overflow
> > bounds to quantum and initial quantum") as reported by Sashiko.
>
> The rest of this commit message is somewhat obtuse - too much run-on
> information. I think it can be pretty easily fixed by a couple of
> rearrangements and paragraph breaks, like this:
>

Sigh - i thought i took care of this (iirc, Stephen preferred the
shorter form as well)

Thanks for the reviewed by.
I guess i have to send a v8.

cheers,
jamal
> > The issue: fq_codel, cake, codel, pie, fq_pie, dualpi2, RED, pfifo,
> > plug and (non-WRED) gred account every enqueued packet's stab-adjusted
> > length into the 32-bit sch->qstats.backlog. A crafted tc size table
> > inflates qdisc_pkt_len() up to QDISC_PKT_LEN_MAX (1 MiB), and a dodgy
> > GSO skb can account even more through qdisc_pkt_len_segs_init(), so a
> > few thousand packets wrap the counter mod 2^32.
> >
> > The AQM then reads a small backlog and makes the wrong drop decision,
> > and the dequeue-side subtractions leave it corrupt (for non-WRED gred
> > it is the reported stats and the dequeue-side subtractions that go
> > wrong instead). In fq_codel a wrapped per-flow backlog can also make
> > fq_codel_drop()'s scan pick an empty flow, dereferencing a NULL
> > flow->head.
> >
> > The Fix: drop at enqueue once the stored backlog cannot represent the
> > incoming packet's accounted length.
> >
> > Ordinary packets (qdisc_pkt_len() at most QDISC_PKT_LEN_MAX) are
> > covered by a standing ceiling: drop once the backlog is within
> > QDISC_MAX_BACKLOG (U32_MAX - QDISC_PKT_LEN_MAX) of the wrap point, the
> > largest backlog one more 1 MiB packet cannot wrap, so there is no
> > per-enqueue 64-bit add.
> >
> > A larger wire estimate is checked against the space that actually
> > remains (len > U32_MAX - backlog): qdisc_pkt_len_segs_init() adds the
> > headers replicated on every output segment, so a supported small-MSS
> > BIG TCP aggregate can carry a wire estimate above 1 MiB while its raw
> > length stays within GSO_MAX_SIZE, and such traffic must pass on an
> > empty queue.
> >
> > qdisc_pkt_len() itself stays truthful for every input: it is not
> > clamped and the initializer keeps succeeding, so BPF
> > __sk_buff.wire_len, act_police and the HTB/TBF shapers read the real
> > length, and sch_handle_ingress() -- which ignores the initializer's
> > return value and reads pkt_segs for packet statistics -- keeps
> > complete metadata.
> >
> > Cover sch_plug and non-WRED sch_gred too: plug admits on skb->len
> > while qdisc_enqueue_tail() adds qdisc_pkt_len(), and gred bounds each
> > virtual queue but adds every VQ into the one aggregate. pfifo is
> > included because RED reads a grafted child's backlog into
> > red_calc_qavg(); it can be reached standalone or as the default child
> > of SFB. In RED, account the packet to the aggregate before handing it
> > to the child, because a duplicating netem child re-enqueues its clone
> > at the root while the stored backlog is still un-updated -- one guard
> > check must not authorize two additions.
> >
> > The guard bounds each qdisc's own enqueue path: a child that segments a
> > GSO skb (tbf, netem) can still grow a parent's aggregate through
> > qdisc_tree_reduce_backlog()'s unclamped byte delta, which is left to a
> > separate change.
> >
> > Conditions to recreate the bug:
> >   ip tuntap add tun0 mode tun
> >   ip link set tun0 txqueuelen 32 up
> >   ip addr add 10.99.0.1/24 dev tun0
> >   tc qdisc add dev tun0 root handle 1: stab overhead 2000000000 \
> >       fq_codel flows 1 limit 20000 ecn
> >   # hold the tun fd open without reading so the backlog persists, then
> >   # send 6000 packets: the counter wraps and the unfixed kernel reports
> >   # a wrapped backlog, while the fixed one drops at the ceiling.
> >
> > Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/netdev/20260818101130.16203-1-jhs@mojatatu.com/
> > Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260818101130.16203-1-jhs@mojatatu.com
> > Link: https://lore.kernel.org/netdev/CANn89iLfMJV7ancKH1Gjzzm7ZUG-gKcrczkJjJEWN5sCuWd-ug@mail.gmail.com/
> > Link: https://lore.kernel.org/netdev/CANn89i+GOFH_8g+vSV0jUj0aBqLYVWuFOrHuDggZu9kPC_m6tQ@mail.gmail.com/
> > Link: https://lore.kernel.org/netdev/87a4oottcj.fsf@toke.dk/
> > Link: https://lore.kernel.org/netdev/87pkxks68f.fsf@toke.dk/
> > Link: https://lore.kernel.org/netdev/179231932946.3135.7497478990186385688@kernel.org/
> > Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> With the above rewording:
>
> Reviewed-by: Toke Høiland-Jørgensen <toke@toke.dk>

[PATCH net-next v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split

[Jamal Hadi Salim]

The Fix:
with CAKE_FLAG_SPLIT_GSO, cake computes the flow/host hash keys, then
splits a GSO skb into segments it accounts individually. Key extraction
must run on the unsegmented skb -- skb_gso_segment() can rewrite an
encapsulated packet's headers to the inner ones -- and the resolution
must wait until the packet is admitted, since it commits the
set-associative tags, host indices and way counters that a later
rejection would leave mutated (a GSO skb whose segmentation fails was
dropped with that state already committed).

Split cake_hash() into a side-effect-free cake_hash_keys(), run on the
unsegmented skb, and cake_hash_resolve(), run only once the packet is
admitted. Move the split-GSO path ahead of the resolution: sum the
segment list, test the total against the ceiling before linking any
segment, and drop the whole list on rejection so no persistent state is
committed. The list is deferred to to_free so the tail segment's
sock_wfree() never runs under the qdisc lock; a fraglist GSO, where
skb_segment_list() returns the original skb as the head, drops the
caller's reference and queues the list once instead of self-linking it.

Tag every deferred segment with QDISC_DROP_OVERLIMIT: __skb_gso_segment()
keeps SKB_GSO_CB at cb offset 32, aliasing tc_skb_cb.drop_reason, and
__copy_skb_header() copies it into every segment, so tagging only the
head left the rest with GSO scratch (a headroom-sized value) as their
drop reason, which __tcf_kfree_skb_list() would report as an unrelated
core reason.

Read q->config->flow_mode once with READ_ONCE() in cake_enqueue() and
pass that value to both cake_hash_keys() and cake_hash_resolve().
cake_config_change() writes the mode before cake_change() takes
sch_tree_lock(), and the split introduced two separate loads with the
segmentation work between them; a concurrent reconfiguration could
extract keys in one mode and resolve them in another.

Link: https://lore.kernel.org/netdev/179231933068.3135.80340792633077986@kernel.org/
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v6 -> v7: the split-GSO reject path now sets QDISC_DROP_OVERLIMIT on
every deferred segment, not only on the head/original skb; the GSO cb
aliases tc_skb_cb.drop_reason and is copied into every segment, so
untagged segments carried GSO scratch as their drop reason. The
flow_mode is likewise snapshotted once with READ_ONCE() and passed to
both cake_hash_keys() and cake_hash_resolve(). (Sashiko nipa v6
review, gpt-6-astra-2-5, gpt-6-astra-5-9)
---
 net/sched/sch_cake.c | 130 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 99 insertions(+), 31 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 91c33295b30b..730c90af14bd 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -706,19 +706,28 @@ static u16 cake_get_flow_quantum(struct cake_tin_data *q,
 		get_random_u16()) >> 16;
 }
 
-static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
-		     int flow_mode, u16 flow_override, u16 host_override)
+struct cake_hash_keys {
+	u32 srchost_hash;
+	u32 dsthost_hash;
+	u32 flow_hash;
+};
+
+static void cake_hash_keys(const struct sk_buff *skb, int flow_mode,
+			   u16 flow_override, u16 host_override,
+			   struct cake_hash_keys *out)
 {
 	bool hash_flows = (!flow_override && !!(flow_mode & CAKE_FLOW_FLOWS));
 	bool hash_hosts = (!host_override && !!(flow_mode & CAKE_FLOW_HOSTS));
 	bool nat_enabled = !!(flow_mode & CAKE_FLOW_NAT_FLAG);
-	u32 flow_hash = 0, srchost_hash = 0, dsthost_hash = 0;
-	u16 reduced_hash, srchost_idx, dsthost_idx;
 	struct flow_keys keys, host_keys;
 	bool use_skbhash = skb->l4_hash;
 
+	out->flow_hash = 0;
+	out->srchost_hash = 0;
+	out->dsthost_hash = 0;
+
 	if (unlikely(flow_mode == CAKE_FLOW_NONE))
-		return 0;
+		return;
 
 	/* If both overrides are set, or we can use the SKB hash and nat mode is
 	 * disabled, we can skip packet dissection entirely. If nat mode is
@@ -753,50 +762,64 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
 	switch (host_keys.control.addr_type) {
 	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
 		host_keys.addrs.v4addrs.src = 0;
-		dsthost_hash = flow_hash_from_keys(&host_keys);
+		out->dsthost_hash = flow_hash_from_keys(&host_keys);
 		host_keys.addrs.v4addrs.src = keys.addrs.v4addrs.src;
 		host_keys.addrs.v4addrs.dst = 0;
-		srchost_hash = flow_hash_from_keys(&host_keys);
+		out->srchost_hash = flow_hash_from_keys(&host_keys);
 		break;
 
 	case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
 		memset(&host_keys.addrs.v6addrs.src, 0,
 		       sizeof(host_keys.addrs.v6addrs.src));
-		dsthost_hash = flow_hash_from_keys(&host_keys);
+		out->dsthost_hash = flow_hash_from_keys(&host_keys);
 		host_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
 		memset(&host_keys.addrs.v6addrs.dst, 0,
 		       sizeof(host_keys.addrs.v6addrs.dst));
-		srchost_hash = flow_hash_from_keys(&host_keys);
+		out->srchost_hash = flow_hash_from_keys(&host_keys);
 		break;
 
 	default:
-		dsthost_hash = 0;
-		srchost_hash = 0;
+		out->dsthost_hash = 0;
+		out->srchost_hash = 0;
 	}
 
 	/* This *must* be after the above switch, since as a
 	 * side-effect it sorts the src and dst addresses.
 	 */
 	if (hash_flows && !use_skbhash)
-		flow_hash = flow_hash_from_keys(&keys);
+		out->flow_hash = flow_hash_from_keys(&keys);
 
 skip_hash:
 	if (flow_override)
-		flow_hash = flow_override - 1;
+		out->flow_hash = flow_override - 1;
 	else if (use_skbhash && (flow_mode & CAKE_FLOW_FLOWS))
-		flow_hash = skb->hash;
+		out->flow_hash = skb->hash;
 	if (host_override) {
-		dsthost_hash = host_override - 1;
-		srchost_hash = host_override - 1;
+		out->dsthost_hash = host_override - 1;
+		out->srchost_hash = host_override - 1;
 	}
 
 	if (!(flow_mode & CAKE_FLOW_FLOWS)) {
 		if (flow_mode & CAKE_FLOW_SRC_IP)
-			flow_hash ^= srchost_hash;
+			out->flow_hash ^= out->srchost_hash;
 
 		if (flow_mode & CAKE_FLOW_DST_IP)
-			flow_hash ^= dsthost_hash;
+			out->flow_hash ^= out->dsthost_hash;
 	}
+}
+
+/* Commit the flow into the set-associative table (q->tags[], host
+ * indices, bulk counters). Only after the packet is admitted.
+ */
+static u32 cake_hash_resolve(struct cake_tin_data *q,
+			     const struct cake_hash_keys *keys, int flow_mode)
+{
+	u32 flow_hash = keys->flow_hash, srchost_hash = keys->srchost_hash,
+	    dsthost_hash = keys->dsthost_hash;
+	u16 reduced_hash, srchost_idx, dsthost_idx;
+
+	if (unlikely(flow_mode == CAKE_FLOW_NONE))
+		return 0;
 
 	reduced_hash = flow_hash % CAKE_QUEUES;
 
@@ -1761,13 +1784,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 {
 	u32 idx, tin, prev_qlen, prev_backlog, drop_id;
 	struct cake_sched_data *q = qdisc_priv(sch);
+	int flow_mode = READ_ONCE(q->config->flow_mode);
 	int len = qdisc_pkt_len(skb), ret;
 	u16 flow_override, host_override;
+	struct sk_buff *segs = NULL;
+	struct cake_hash_keys keys;
 	struct sk_buff *ack = NULL;
 	ktime_t now = ktime_get();
 	struct cake_tin_data *b;
 	struct cake_flow *flow;
 	bool same_flow = false;
+	u64 slen = 0;
 
 	/* Classify first: a terminal TC action consumes the packet. */
 	if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &ret)) {
@@ -1784,12 +1811,59 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 					 QDISC_DROP_OVERLIMIT);
 	}
 
-	/* choose flow to insert into */
+	/* Select the tin and extract the keys before segmenting:
+	 * skb_gso_segment() rewrites encapsulated headers, which
+	 * changes the keys.
+	 */
 	b = cake_select_tin(sch, skb);
-	idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
-			host_override) + 1;
+	cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys);
+
+	if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
+		struct sk_buff *nskb, *seg;
+		netdev_features_t features = netif_skb_features(skb);
+
+		segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
+		if (IS_ERR_OR_NULL(segs))
+			return qdisc_drop(skb, sch, to_free);
+
+		/* The split path accounts the segment sum, not the
+		 * stab-adjusted qdisc_pkt_len(), so check it here.
+		 */
+		skb_list_walk_safe(segs, seg, nskb)
+			slen += seg->len;
+
+		if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
+			qdisc_qstats_overlimit(sch);
+			/* __skb_gso_segment() puts SKB_GSO_CB at cb
+			 * offset 32, over tc_skb_cb.drop_reason; every
+			 * segment inherits it. Tag each one with the
+			 * qdisc drop reason.
+			 */
+			skb_list_walk_safe(segs, seg, nskb)
+				tcf_set_qdisc_drop_reason(seg,
+							  QDISC_DROP_OVERLIMIT);
+			/* skb_gso_segment() moved sock_wfree to the tail
+			 * segment, so defer the list to to_free rather
+			 * than free it under the qdisc lock.
+			 *
+			 * Fraglist GSO returns the original skb as head
+			 * (segs == skb), so drop our ref first.
+			 */
+			if (unlikely(segs == skb)) {
+				consume_skb(skb);
+				return qdisc_drop_all(segs, sch, to_free);
+			}
+			__qdisc_drop_all(segs, to_free);
+			return qdisc_drop_reason(skb, sch, to_free,
+						 QDISC_DROP_OVERLIMIT);
+		}
+	}
+
+	/* Admitted: resolve the flow. The mode is read once above, so
+	 * extraction and resolution agree if cake_change() intervenes.
+	 */
+	idx = cake_hash_resolve(b, &keys, flow_mode);
 	tin = (u32)(b - q->tins);
-	idx--;
 	flow = &b->flows[idx];
 
 	/* ensure shaper state isn't stale */
@@ -1816,14 +1890,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	if (unlikely(len > b->max_skblen))
 		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, *seg;
-		netdev_features_t features = netif_skb_features(skb);
-		unsigned int slen = 0, numsegs = 0;
-
-		segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
-		if (IS_ERR_OR_NULL(segs))
-			return qdisc_drop(skb, sch, to_free);
+	if (segs) {
+		struct sk_buff *nskb, *seg;
+		unsigned int numsegs = 0;
 
 		skb_list_walk_safe(segs, seg, nskb) {
 			skb_mark_not_on_list(seg);
@@ -1837,7 +1906,6 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 			qdisc_qlen_inc(sch);
 			numsegs++;
 			q->buffer_used += seg->truesize;
-			slen += seg->len;
 			WRITE_ONCE(b->packets, b->packets + 1);
 		}
 
-- 
2.43.0

Re: [PATCH net-next v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split

[Toke Høiland-Jørgensen]

Jamal Hadi Salim <jhs@mojatatu.com> writes:

> The Fix:
> with CAKE_FLAG_SPLIT_GSO, cake computes the flow/host hash keys, then
> splits a GSO skb into segments it accounts individually. Key extraction
> must run on the unsegmented skb -- skb_gso_segment() can rewrite an
> encapsulated packet's headers to the inner ones -- and the resolution
> must wait until the packet is admitted, since it commits the
> set-associative tags, host indices and way counters that a later
> rejection would leave mutated (a GSO skb whose segmentation fails was
> dropped with that state already committed).
>
> Split cake_hash() into a side-effect-free cake_hash_keys(), run on the
> unsegmented skb, and cake_hash_resolve(), run only once the packet is
> admitted. Move the split-GSO path ahead of the resolution: sum the
> segment list, test the total against the ceiling before linking any
> segment, and drop the whole list on rejection so no persistent state is
> committed. The list is deferred to to_free so the tail segment's
> sock_wfree() never runs under the qdisc lock; a fraglist GSO, where
> skb_segment_list() returns the original skb as the head, drops the
> caller's reference and queues the list once instead of self-linking it.
>
> Tag every deferred segment with QDISC_DROP_OVERLIMIT: __skb_gso_segment()
> keeps SKB_GSO_CB at cb offset 32, aliasing tc_skb_cb.drop_reason, and
> __copy_skb_header() copies it into every segment, so tagging only the
> head left the rest with GSO scratch (a headroom-sized value) as their
> drop reason, which __tcf_kfree_skb_list() would report as an unrelated
> core reason.
>
> Read q->config->flow_mode once with READ_ONCE() in cake_enqueue() and
> pass that value to both cake_hash_keys() and cake_hash_resolve().
> cake_config_change() writes the mode before cake_change() takes
> sch_tree_lock(), and the split introduced two separate loads with the
> segmentation work between them; a concurrent reconfiguration could
> extract keys in one mode and resolve them in another.
>
> Link: https://lore.kernel.org/netdev/179231933068.3135.80340792633077986@kernel.org/
> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
> v6 -> v7: the split-GSO reject path now sets QDISC_DROP_OVERLIMIT on
> every deferred segment, not only on the head/original skb; the GSO cb
> aliases tc_skb_cb.drop_reason and is copied into every segment, so
> untagged segments carried GSO scratch as their drop reason. The
> flow_mode is likewise snapshotted once with READ_ONCE() and passed to
> both cake_hash_keys() and cake_hash_resolve(). (Sashiko nipa v6
> review, gpt-6-astra-2-5, gpt-6-astra-5-9)
> ---
>  net/sched/sch_cake.c | 130 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 99 insertions(+), 31 deletions(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index 91c33295b30b..730c90af14bd 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -706,19 +706,28 @@ static u16 cake_get_flow_quantum(struct cake_tin_data *q,
>  		get_random_u16()) >> 16;
>  }
>  
> -static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
> -		     int flow_mode, u16 flow_override, u16 host_override)
> +struct cake_hash_keys {
> +	u32 srchost_hash;
> +	u32 dsthost_hash;
> +	u32 flow_hash;
> +};
> +
> +static void cake_hash_keys(const struct sk_buff *skb, int flow_mode,
> +			   u16 flow_override, u16 host_override,
> +			   struct cake_hash_keys *out)
>  {
>  	bool hash_flows = (!flow_override && !!(flow_mode & CAKE_FLOW_FLOWS));
>  	bool hash_hosts = (!host_override && !!(flow_mode & CAKE_FLOW_HOSTS));
>  	bool nat_enabled = !!(flow_mode & CAKE_FLOW_NAT_FLAG);
> -	u32 flow_hash = 0, srchost_hash = 0, dsthost_hash = 0;
> -	u16 reduced_hash, srchost_idx, dsthost_idx;
>  	struct flow_keys keys, host_keys;
>  	bool use_skbhash = skb->l4_hash;
>  
> +	out->flow_hash = 0;
> +	out->srchost_hash = 0;
> +	out->dsthost_hash = 0;
> +
>  	if (unlikely(flow_mode == CAKE_FLOW_NONE))
> -		return 0;
> +		return;
>  
>  	/* If both overrides are set, or we can use the SKB hash and nat mode is
>  	 * disabled, we can skip packet dissection entirely. If nat mode is
> @@ -753,50 +762,64 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
>  	switch (host_keys.control.addr_type) {
>  	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
>  		host_keys.addrs.v4addrs.src = 0;
> -		dsthost_hash = flow_hash_from_keys(&host_keys);
> +		out->dsthost_hash = flow_hash_from_keys(&host_keys);
>  		host_keys.addrs.v4addrs.src = keys.addrs.v4addrs.src;
>  		host_keys.addrs.v4addrs.dst = 0;
> -		srchost_hash = flow_hash_from_keys(&host_keys);
> +		out->srchost_hash = flow_hash_from_keys(&host_keys);
>  		break;
>  
>  	case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
>  		memset(&host_keys.addrs.v6addrs.src, 0,
>  		       sizeof(host_keys.addrs.v6addrs.src));
> -		dsthost_hash = flow_hash_from_keys(&host_keys);
> +		out->dsthost_hash = flow_hash_from_keys(&host_keys);
>  		host_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
>  		memset(&host_keys.addrs.v6addrs.dst, 0,
>  		       sizeof(host_keys.addrs.v6addrs.dst));
> -		srchost_hash = flow_hash_from_keys(&host_keys);
> +		out->srchost_hash = flow_hash_from_keys(&host_keys);
>  		break;
>  
>  	default:
> -		dsthost_hash = 0;
> -		srchost_hash = 0;
> +		out->dsthost_hash = 0;
> +		out->srchost_hash = 0;
>  	}
>  
>  	/* This *must* be after the above switch, since as a
>  	 * side-effect it sorts the src and dst addresses.
>  	 */
>  	if (hash_flows && !use_skbhash)
> -		flow_hash = flow_hash_from_keys(&keys);
> +		out->flow_hash = flow_hash_from_keys(&keys);
>  
>  skip_hash:
>  	if (flow_override)
> -		flow_hash = flow_override - 1;
> +		out->flow_hash = flow_override - 1;
>  	else if (use_skbhash && (flow_mode & CAKE_FLOW_FLOWS))
> -		flow_hash = skb->hash;
> +		out->flow_hash = skb->hash;
>  	if (host_override) {
> -		dsthost_hash = host_override - 1;
> -		srchost_hash = host_override - 1;
> +		out->dsthost_hash = host_override - 1;
> +		out->srchost_hash = host_override - 1;
>  	}
>  
>  	if (!(flow_mode & CAKE_FLOW_FLOWS)) {
>  		if (flow_mode & CAKE_FLOW_SRC_IP)
> -			flow_hash ^= srchost_hash;
> +			out->flow_hash ^= out->srchost_hash;
>  
>  		if (flow_mode & CAKE_FLOW_DST_IP)
> -			flow_hash ^= dsthost_hash;
> +			out->flow_hash ^= out->dsthost_hash;
>  	}
> +}
> +
> +/* Commit the flow into the set-associative table (q->tags[], host
> + * indices, bulk counters). Only after the packet is admitted.
> + */
> +static u32 cake_hash_resolve(struct cake_tin_data *q,
> +			     const struct cake_hash_keys *keys, int flow_mode)
> +{
> +	u32 flow_hash = keys->flow_hash, srchost_hash = keys->srchost_hash,
> +	    dsthost_hash = keys->dsthost_hash;
> +	u16 reduced_hash, srchost_idx, dsthost_idx;
> +
> +	if (unlikely(flow_mode == CAKE_FLOW_NONE))
> +		return 0;
>  
>  	reduced_hash = flow_hash % CAKE_QUEUES;
>  
> @@ -1761,13 +1784,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  {
>  	u32 idx, tin, prev_qlen, prev_backlog, drop_id;
>  	struct cake_sched_data *q = qdisc_priv(sch);
> +	int flow_mode = READ_ONCE(q->config->flow_mode);

This breaks the reverse x-mas tree ordering. Move the u32 defs below,
and keep an empty line after the qdisc_priv() resolution?

>  	int len = qdisc_pkt_len(skb), ret;
>  	u16 flow_override, host_override;
> +	struct sk_buff *segs = NULL;
> +	struct cake_hash_keys keys;
>  	struct sk_buff *ack = NULL;
>  	ktime_t now = ktime_get();
>  	struct cake_tin_data *b;
>  	struct cake_flow *flow;
>  	bool same_flow = false;
> +	u64 slen = 0;
>  
>  	/* Classify first: a terminal TC action consumes the packet. */
>  	if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &ret)) {
> @@ -1784,12 +1811,59 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  					 QDISC_DROP_OVERLIMIT);
>  	}
>  
> -	/* choose flow to insert into */
> +	/* Select the tin and extract the keys before segmenting:
> +	 * skb_gso_segment() rewrites encapsulated headers, which
> +	 * changes the keys.
> +	 */
>  	b = cake_select_tin(sch, skb);
> -	idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
> -			host_override) + 1;
> +	cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys);
> +
> +	if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> +		struct sk_buff *nskb, *seg;
> +		netdev_features_t features = netif_skb_features(skb);

Reverse x-mas tree, please.

> +
> +		segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
> +		if (IS_ERR_OR_NULL(segs))
> +			return qdisc_drop(skb, sch, to_free);
> +
> +		/* The split path accounts the segment sum, not the
> +		 * stab-adjusted qdisc_pkt_len(), so check it here.
> +		 */
> +		skb_list_walk_safe(segs, seg, nskb)
> +			slen += seg->len;
> +
> +		if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
> +			qdisc_qstats_overlimit(sch);
> +			/* __skb_gso_segment() puts SKB_GSO_CB at cb
> +			 * offset 32, over tc_skb_cb.drop_reason; every
> +			 * segment inherits it. Tag each one with the
> +			 * qdisc drop reason.
> +			 */
> +			skb_list_walk_safe(segs, seg, nskb)
> +				tcf_set_qdisc_drop_reason(seg,
> +							  QDISC_DROP_OVERLIMIT);
> +			/* skb_gso_segment() moved sock_wfree to the tail
> +			 * segment, so defer the list to to_free rather
> +			 * than free it under the qdisc lock.

I don't think this part of the comment is needed - we do all the drops
using to_free, so the sock_wfree thing is irrelevant.

> +			 * Fraglist GSO returns the original skb as head
> +			 * (segs == skb), so drop our ref first.
> +			 */
> +			if (unlikely(segs == skb)) {
> +				consume_skb(skb);
> +				return qdisc_drop_all(segs, sch, to_free);
> +			}
> +			__qdisc_drop_all(segs, to_free);
> +			return qdisc_drop_reason(skb, sch, to_free,
> +						 QDISC_DROP_OVERLIMIT);

Since you already set the drop reason above, there's not really any
reason to use qdisc_drop_reason() anymore; so this logic can be
simplified to simply:

			if (unlikely(segs == skb))
				consume_skb(skb);
			return qdisc_drop_all(segs, sch, to_free);

> +		}
> +	}
> +
> +	/* Admitted: resolve the flow. The mode is read once above, so
> +	 * extraction and resolution agree if cake_change() intervenes.
> +	 */

The last sentence here is just distracting. Please drop.

> +	idx = cake_hash_resolve(b, &keys, flow_mode);
>  	tin = (u32)(b - q->tins);
> -	idx--;
>  	flow = &b->flows[idx];
>  
>  	/* ensure shaper state isn't stale */
> @@ -1816,14 +1890,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	if (unlikely(len > b->max_skblen))
>  		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, *seg;
> -		netdev_features_t features = netif_skb_features(skb);
> -		unsigned int slen = 0, numsegs = 0;
> -
> -		segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
> -		if (IS_ERR_OR_NULL(segs))
> -			return qdisc_drop(skb, sch, to_free);
> +	if (segs) {
> +		struct sk_buff *nskb, *seg;
> +		unsigned int numsegs = 0;
>  
>  		skb_list_walk_safe(segs, seg, nskb) {
>  			skb_mark_not_on_list(seg);
> @@ -1837,7 +1906,6 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  			qdisc_qlen_inc(sch);
>  			numsegs++;
>  			q->buffer_used += seg->truesize;
> -			slen += seg->len;
>  			WRITE_ONCE(b->packets, b->packets + 1);
>  		}
>  
> -- 
> 2.43.0

Re: [PATCH net-next v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split

[Jamal Hadi Salim]

On Mon, Oct 19, 2026 at 2:32 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > The Fix:
> > with CAKE_FLAG_SPLIT_GSO, cake computes the flow/host hash keys, then
> > splits a GSO skb into segments it accounts individually. Key extraction
> > must run on the unsegmented skb -- skb_gso_segment() can rewrite an
> > encapsulated packet's headers to the inner ones -- and the resolution
> > must wait until the packet is admitted, since it commits the
> > set-associative tags, host indices and way counters that a later
> > rejection would leave mutated (a GSO skb whose segmentation fails was
> > dropped with that state already committed).
> >
> > Split cake_hash() into a side-effect-free cake_hash_keys(), run on the
> > unsegmented skb, and cake_hash_resolve(), run only once the packet is
> > admitted. Move the split-GSO path ahead of the resolution: sum the
> > segment list, test the total against the ceiling before linking any
> > segment, and drop the whole list on rejection so no persistent state is
> > committed. The list is deferred to to_free so the tail segment's
> > sock_wfree() never runs under the qdisc lock; a fraglist GSO, where
> > skb_segment_list() returns the original skb as the head, drops the
> > caller's reference and queues the list once instead of self-linking it.
> >
> > Tag every deferred segment with QDISC_DROP_OVERLIMIT: __skb_gso_segment()
> > keeps SKB_GSO_CB at cb offset 32, aliasing tc_skb_cb.drop_reason, and
> > __copy_skb_header() copies it into every segment, so tagging only the
> > head left the rest with GSO scratch (a headroom-sized value) as their
> > drop reason, which __tcf_kfree_skb_list() would report as an unrelated
> > core reason.
> >
> > Read q->config->flow_mode once with READ_ONCE() in cake_enqueue() and
> > pass that value to both cake_hash_keys() and cake_hash_resolve().
> > cake_config_change() writes the mode before cake_change() takes
> > sch_tree_lock(), and the split introduced two separate loads with the
> > segmentation work between them; a concurrent reconfiguration could
> > extract keys in one mode and resolve them in another.
> >
> > Link: https://lore.kernel.org/netdev/179231933068.3135.80340792633077986@kernel.org/
> > Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > ---
> > v6 -> v7: the split-GSO reject path now sets QDISC_DROP_OVERLIMIT on
> > every deferred segment, not only on the head/original skb; the GSO cb
> > aliases tc_skb_cb.drop_reason and is copied into every segment, so
> > untagged segments carried GSO scratch as their drop reason. The
> > flow_mode is likewise snapshotted once with READ_ONCE() and passed to
> > both cake_hash_keys() and cake_hash_resolve(). (Sashiko nipa v6
> > review, gpt-6-astra-2-5, gpt-6-astra-5-9)
> > ---
> >  net/sched/sch_cake.c | 130 ++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 99 insertions(+), 31 deletions(-)
> >
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index 91c33295b30b..730c90af14bd 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
> > @@ -706,19 +706,28 @@ static u16 cake_get_flow_quantum(struct cake_tin_data *q,
> >               get_random_u16()) >> 16;
> >  }
> >
> > -static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
> > -                  int flow_mode, u16 flow_override, u16 host_override)
> > +struct cake_hash_keys {
> > +     u32 srchost_hash;
> > +     u32 dsthost_hash;
> > +     u32 flow_hash;
> > +};
> > +
> > +static void cake_hash_keys(const struct sk_buff *skb, int flow_mode,
> > +                        u16 flow_override, u16 host_override,
> > +                        struct cake_hash_keys *out)
> >  {
> >       bool hash_flows = (!flow_override && !!(flow_mode & CAKE_FLOW_FLOWS));
> >       bool hash_hosts = (!host_override && !!(flow_mode & CAKE_FLOW_HOSTS));
> >       bool nat_enabled = !!(flow_mode & CAKE_FLOW_NAT_FLAG);
> > -     u32 flow_hash = 0, srchost_hash = 0, dsthost_hash = 0;
> > -     u16 reduced_hash, srchost_idx, dsthost_idx;
> >       struct flow_keys keys, host_keys;
> >       bool use_skbhash = skb->l4_hash;
> >
> > +     out->flow_hash = 0;
> > +     out->srchost_hash = 0;
> > +     out->dsthost_hash = 0;
> > +
> >       if (unlikely(flow_mode == CAKE_FLOW_NONE))
> > -             return 0;
> > +             return;
> >
> >       /* If both overrides are set, or we can use the SKB hash and nat mode is
> >        * disabled, we can skip packet dissection entirely. If nat mode is
> > @@ -753,50 +762,64 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
> >       switch (host_keys.control.addr_type) {
> >       case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
> >               host_keys.addrs.v4addrs.src = 0;
> > -             dsthost_hash = flow_hash_from_keys(&host_keys);
> > +             out->dsthost_hash = flow_hash_from_keys(&host_keys);
> >               host_keys.addrs.v4addrs.src = keys.addrs.v4addrs.src;
> >               host_keys.addrs.v4addrs.dst = 0;
> > -             srchost_hash = flow_hash_from_keys(&host_keys);
> > +             out->srchost_hash = flow_hash_from_keys(&host_keys);
> >               break;
> >
> >       case FLOW_DISSECTOR_KEY_IPV6_ADDRS:
> >               memset(&host_keys.addrs.v6addrs.src, 0,
> >                      sizeof(host_keys.addrs.v6addrs.src));
> > -             dsthost_hash = flow_hash_from_keys(&host_keys);
> > +             out->dsthost_hash = flow_hash_from_keys(&host_keys);
> >               host_keys.addrs.v6addrs.src = keys.addrs.v6addrs.src;
> >               memset(&host_keys.addrs.v6addrs.dst, 0,
> >                      sizeof(host_keys.addrs.v6addrs.dst));
> > -             srchost_hash = flow_hash_from_keys(&host_keys);
> > +             out->srchost_hash = flow_hash_from_keys(&host_keys);
> >               break;
> >
> >       default:
> > -             dsthost_hash = 0;
> > -             srchost_hash = 0;
> > +             out->dsthost_hash = 0;
> > +             out->srchost_hash = 0;
> >       }
> >
> >       /* This *must* be after the above switch, since as a
> >        * side-effect it sorts the src and dst addresses.
> >        */
> >       if (hash_flows && !use_skbhash)
> > -             flow_hash = flow_hash_from_keys(&keys);
> > +             out->flow_hash = flow_hash_from_keys(&keys);
> >
> >  skip_hash:
> >       if (flow_override)
> > -             flow_hash = flow_override - 1;
> > +             out->flow_hash = flow_override - 1;
> >       else if (use_skbhash && (flow_mode & CAKE_FLOW_FLOWS))
> > -             flow_hash = skb->hash;
> > +             out->flow_hash = skb->hash;
> >       if (host_override) {
> > -             dsthost_hash = host_override - 1;
> > -             srchost_hash = host_override - 1;
> > +             out->dsthost_hash = host_override - 1;
> > +             out->srchost_hash = host_override - 1;
> >       }
> >
> >       if (!(flow_mode & CAKE_FLOW_FLOWS)) {
> >               if (flow_mode & CAKE_FLOW_SRC_IP)
> > -                     flow_hash ^= srchost_hash;
> > +                     out->flow_hash ^= out->srchost_hash;
> >
> >               if (flow_mode & CAKE_FLOW_DST_IP)
> > -                     flow_hash ^= dsthost_hash;
> > +                     out->flow_hash ^= out->dsthost_hash;
> >       }
> > +}
> > +
> > +/* Commit the flow into the set-associative table (q->tags[], host
> > + * indices, bulk counters). Only after the packet is admitted.
> > + */
> > +static u32 cake_hash_resolve(struct cake_tin_data *q,
> > +                          const struct cake_hash_keys *keys, int flow_mode)
> > +{
> > +     u32 flow_hash = keys->flow_hash, srchost_hash = keys->srchost_hash,
> > +         dsthost_hash = keys->dsthost_hash;
> > +     u16 reduced_hash, srchost_idx, dsthost_idx;
> > +
> > +     if (unlikely(flow_mode == CAKE_FLOW_NONE))
> > +             return 0;
> >
> >       reduced_hash = flow_hash % CAKE_QUEUES;
> >
> > @@ -1761,13 +1784,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >  {
> >       u32 idx, tin, prev_qlen, prev_backlog, drop_id;
> >       struct cake_sched_data *q = qdisc_priv(sch);
> > +     int flow_mode = READ_ONCE(q->config->flow_mode);
>
> This breaks the reverse x-mas tree ordering. Move the u32 defs below,
> and keep an empty line after the qdisc_priv() resolution?
>
> >       int len = qdisc_pkt_len(skb), ret;
> >       u16 flow_override, host_override;
> > +     struct sk_buff *segs = NULL;
> > +     struct cake_hash_keys keys;
> >       struct sk_buff *ack = NULL;
> >       ktime_t now = ktime_get();
> >       struct cake_tin_data *b;
> >       struct cake_flow *flow;
> >       bool same_flow = false;
> > +     u64 slen = 0;
> >
> >       /* Classify first: a terminal TC action consumes the packet. */
> >       if (cake_tcf_classify(sch, skb, &flow_override, &host_override, &ret)) {
> > @@ -1784,12 +1811,59 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >                                        QDISC_DROP_OVERLIMIT);
> >       }
> >
> > -     /* choose flow to insert into */
> > +     /* Select the tin and extract the keys before segmenting:
> > +      * skb_gso_segment() rewrites encapsulated headers, which
> > +      * changes the keys.
> > +      */
> >       b = cake_select_tin(sch, skb);
> > -     idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
> > -                     host_override) + 1;
> > +     cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys);
> > +
> > +     if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> > +             struct sk_buff *nskb, *seg;
> > +             netdev_features_t features = netif_skb_features(skb);
>
> Reverse x-mas tree, please.

Grrr...

> > +
> > +             segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
> > +             if (IS_ERR_OR_NULL(segs))
> > +                     return qdisc_drop(skb, sch, to_free);
> > +
> > +             /* The split path accounts the segment sum, not the
> > +              * stab-adjusted qdisc_pkt_len(), so check it here.
> > +              */
> > +             skb_list_walk_safe(segs, seg, nskb)
> > +                     slen += seg->len;
> > +
> > +             if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
> > +                     qdisc_qstats_overlimit(sch);
> > +                     /* __skb_gso_segment() puts SKB_GSO_CB at cb
> > +                      * offset 32, over tc_skb_cb.drop_reason; every
> > +                      * segment inherits it. Tag each one with the
> > +                      * qdisc drop reason.
> > +                      */
> > +                     skb_list_walk_safe(segs, seg, nskb)
> > +                             tcf_set_qdisc_drop_reason(seg,
> > +                                                       QDISC_DROP_OVERLIMIT);
> > +                     /* skb_gso_segment() moved sock_wfree to the tail
> > +                      * segment, so defer the list to to_free rather
> > +                      * than free it under the qdisc lock.
>
> I don't think this part of the comment is needed - we do all the drops
> using to_free, so the sock_wfree thing is irrelevant.

I was trying to rationalize "the segs == skb fraglist". How about:
/* Fraglist GSO returns the original skb as head  (segs == skb), so
drop our ref first */
I can drop it if it seems obvious

> > +                      * Fraglist GSO returns the original skb as head
> > +                      * (segs == skb), so drop our ref first.
> > +                      */
> > +                     if (unlikely(segs == skb)) {
> > +                             consume_skb(skb);
> > +                             return qdisc_drop_all(segs, sch, to_free);
> > +                     }
> > +                     __qdisc_drop_all(segs, to_free);
> > +                     return qdisc_drop_reason(skb, sch, to_free,
> > +                                              QDISC_DROP_OVERLIMIT);
>
> Since you already set the drop reason above, there's not really any
> reason to use qdisc_drop_reason() anymore; so this logic can be
> simplified to simply:
>
>                         if (unlikely(segs == skb))
>                                 consume_skb(skb);
>                         return qdisc_drop_all(segs, sch, to_free);
>

How does this look ?
tcf_set_qdisc_drop_reason(skb, QDISC_DROP_OVERLIMIT);
skb_list_walk_safe(segs, seg, nskb)
     tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT);

if (unlikely(segs == skb)) {
        consume_skb(skb);
        return qdisc_drop_all(segs, sch, to_free);
}
__qdisc_drop_all(segs, to_free);
return qdisc_drop(skb, sch, to_free);

> > +             }
> > +     }
> > +
> > +     /* Admitted: resolve the flow. The mode is read once above, so
> > +      * extraction and resolution agree if cake_change() intervenes.
> > +      */
>
> The last sentence here is just distracting. Please drop.
>

Ok.Will reword to: /* Admitted: resolve the flow. */

I wont wait for the 7 days for sashiko to respond if we can reach a
quick consensus.

cheers,
jamal

> > +     idx = cake_hash_resolve(b, &keys, flow_mode);
> >       tin = (u32)(b - q->tins);
> > -     idx--;
> >       flow = &b->flows[idx];
> >
> >       /* ensure shaper state isn't stale */
> > @@ -1816,14 +1890,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >       if (unlikely(len > b->max_skblen))
> >               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, *seg;
> > -             netdev_features_t features = netif_skb_features(skb);
> > -             unsigned int slen = 0, numsegs = 0;
> > -
> > -             segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
> > -             if (IS_ERR_OR_NULL(segs))
> > -                     return qdisc_drop(skb, sch, to_free);
> > +     if (segs) {
> > +             struct sk_buff *nskb, *seg;
> > +             unsigned int numsegs = 0;
> >
> >               skb_list_walk_safe(segs, seg, nskb) {
> >                       skb_mark_not_on_list(seg);
> > @@ -1837,7 +1906,6 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >                       qdisc_qlen_inc(sch);
> >                       numsegs++;
> >                       q->buffer_used += seg->truesize;
> > -                     slen += seg->len;
> >                       WRITE_ONCE(b->packets, b->packets + 1);
> >               }
> >
> > --
> > 2.43.0

Re: [PATCH net-next v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split

[Toke Høiland-Jørgensen]

>> > +
>> > +             segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
>> > +             if (IS_ERR_OR_NULL(segs))
>> > +                     return qdisc_drop(skb, sch, to_free);
>> > +
>> > +             /* The split path accounts the segment sum, not the
>> > +              * stab-adjusted qdisc_pkt_len(), so check it here.
>> > +              */
>> > +             skb_list_walk_safe(segs, seg, nskb)
>> > +                     slen += seg->len;
>> > +
>> > +             if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
>> > +                     qdisc_qstats_overlimit(sch);
>> > +                     /* __skb_gso_segment() puts SKB_GSO_CB at cb
>> > +                      * offset 32, over tc_skb_cb.drop_reason; every
>> > +                      * segment inherits it. Tag each one with the
>> > +                      * qdisc drop reason.
>> > +                      */
>> > +                     skb_list_walk_safe(segs, seg, nskb)
>> > +                             tcf_set_qdisc_drop_reason(seg,
>> > +                                                       QDISC_DROP_OVERLIMIT);
>> > +                     /* skb_gso_segment() moved sock_wfree to the tail
>> > +                      * segment, so defer the list to to_free rather
>> > +                      * than free it under the qdisc lock.
>>
>> I don't think this part of the comment is needed - we do all the drops
>> using to_free, so the sock_wfree thing is irrelevant.
>
> I was trying to rationalize "the segs == skb fraglist". How about:
> /* Fraglist GSO returns the original skb as head  (segs == skb), so
> drop our ref first */
> I can drop it if it seems obvious

Yeah, that bit is fine to keep :)

>> > +                      * Fraglist GSO returns the original skb as head
>> > +                      * (segs == skb), so drop our ref first.
>> > +                      */
>> > +                     if (unlikely(segs == skb)) {
>> > +                             consume_skb(skb);
>> > +                             return qdisc_drop_all(segs, sch, to_free);
>> > +                     }
>> > +                     __qdisc_drop_all(segs, to_free);
>> > +                     return qdisc_drop_reason(skb, sch, to_free,
>> > +                                              QDISC_DROP_OVERLIMIT);
>>
>> Since you already set the drop reason above, there's not really any
>> reason to use qdisc_drop_reason() anymore; so this logic can be
>> simplified to simply:
>>
>>                         if (unlikely(segs == skb))
>>                                 consume_skb(skb);
>>                         return qdisc_drop_all(segs, sch, to_free);
>>
>
> How does this look ?
> tcf_set_qdisc_drop_reason(skb, QDISC_DROP_OVERLIMIT);
> skb_list_walk_safe(segs, seg, nskb)
>      tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT);
>
> if (unlikely(segs == skb)) {
>         consume_skb(skb);
>         return qdisc_drop_all(segs, sch, to_free);
> }
> __qdisc_drop_all(segs, to_free);
> return qdisc_drop(skb, sch, to_free);

Ah, right, got the 'segs' and 'skb' variables a bit confused, sorry
about that.

But, thinking about it some more:

In both branches of your code above, we have a drop of the skb, and a
drop_all of the segs. And the existing code already has an unconditional
consume_skb() after segmentation, where none of the post-segmentation
uses the 'skb' pointer.

So wouldn't it be simpler to just add the consume_skb() unconditionally
immediately after the skb_gso_segment() call? Something like this, on
top of your patch:

diff --git i/net/sched/sch_cake.c w/net/sched/sch_cake.c
index 730c90af14bd..20173aaf973c 100644
--- i/net/sched/sch_cake.c
+++ w/net/sched/sch_cake.c
@@ -1819,13 +1819,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
        cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys);
 
        if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
-               struct sk_buff *nskb, *seg;
                netdev_features_t features = netif_skb_features(skb);
+               struct sk_buff *nskb, *seg;
 
                segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
                if (IS_ERR_OR_NULL(segs))
                        return qdisc_drop(skb, sch, to_free);
 
+               /* we never use the original skb after splitting */
+               consume_skb(skb);
+               skb = NULL;
+
                /* The split path accounts the segment sum, not the
                 * stab-adjusted qdisc_pkt_len(), so check it here.
                 */
@@ -1842,20 +1846,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
                        skb_list_walk_safe(segs, seg, nskb)
                                tcf_set_qdisc_drop_reason(seg,
                                                          QDISC_DROP_OVERLIMIT);
-                       /* skb_gso_segment() moved sock_wfree to the tail
-                        * segment, so defer the list to to_free rather
-                        * than free it under the qdisc lock.
-                        *
-                        * Fraglist GSO returns the original skb as head
-                        * (segs == skb), so drop our ref first.
-                        */
-                       if (unlikely(segs == skb)) {
-                               consume_skb(skb);
-                               return qdisc_drop_all(segs, sch, to_free);
-                       }
-                       __qdisc_drop_all(segs, to_free);
-                       return qdisc_drop_reason(skb, sch, to_free,
-                                                QDISC_DROP_OVERLIMIT);
+
+                       return qdisc_drop_all(segs, sch, to_free);
                }
        }
 
@@ -1917,9 +1909,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
                WRITE_ONCE(b->backlogs[idx], b->backlogs[idx] + slen);
 
                qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
-               consume_skb(skb);
        } else {
-               /* not splitting */
+               /* not splitting - skb pointer still valid */
                int ack_pkt_len = 0;
 
                cobalt_set_enqueue_time(skb, now);

>> > +             }
>> > +     }
>> > +
>> > +     /* Admitted: resolve the flow. The mode is read once above, so
>> > +      * extraction and resolution agree if cake_change() intervenes.
>> > +      */
>>
>> The last sentence here is just distracting. Please drop.
>>
>
> Ok.Will reword to: /* Admitted: resolve the flow. */
>
> I wont wait for the 7 days for sashiko to respond if we can reach a
> quick consensus.

Fine with me :)

-Toke

Re: [PATCH net-next v7 4/4] net/sched: cake: hash before segmenting and reject an overlong split

[Jamal Hadi Salim]

On Tue, Oct 20, 2026 at 4:47 AM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> >> > +
> >> > +             segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
> >> > +             if (IS_ERR_OR_NULL(segs))
> >> > +                     return qdisc_drop(skb, sch, to_free);
> >> > +
> >> > +             /* The split path accounts the segment sum, not the
> >> > +              * stab-adjusted qdisc_pkt_len(), so check it here.
> >> > +              */
> >> > +             skb_list_walk_safe(segs, seg, nskb)
> >> > +                     slen += seg->len;
> >> > +
> >> > +             if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
> >> > +                     qdisc_qstats_overlimit(sch);
> >> > +                     /* __skb_gso_segment() puts SKB_GSO_CB at cb
> >> > +                      * offset 32, over tc_skb_cb.drop_reason; every
> >> > +                      * segment inherits it. Tag each one with the
> >> > +                      * qdisc drop reason.
> >> > +                      */
> >> > +                     skb_list_walk_safe(segs, seg, nskb)
> >> > +                             tcf_set_qdisc_drop_reason(seg,
> >> > +                                                       QDISC_DROP_OVERLIMIT);
> >> > +                     /* skb_gso_segment() moved sock_wfree to the tail
> >> > +                      * segment, so defer the list to to_free rather
> >> > +                      * than free it under the qdisc lock.
> >>
> >> I don't think this part of the comment is needed - we do all the drops
> >> using to_free, so the sock_wfree thing is irrelevant.
> >
> > I was trying to rationalize "the segs == skb fraglist". How about:
> > /* Fraglist GSO returns the original skb as head  (segs == skb), so
> > drop our ref first */
> > I can drop it if it seems obvious
>
> Yeah, that bit is fine to keep :)
>
> >> > +                      * Fraglist GSO returns the original skb as head
> >> > +                      * (segs == skb), so drop our ref first.
> >> > +                      */
> >> > +                     if (unlikely(segs == skb)) {
> >> > +                             consume_skb(skb);
> >> > +                             return qdisc_drop_all(segs, sch, to_free);
> >> > +                     }
> >> > +                     __qdisc_drop_all(segs, to_free);
> >> > +                     return qdisc_drop_reason(skb, sch, to_free,
> >> > +                                              QDISC_DROP_OVERLIMIT);
> >>
> >> Since you already set the drop reason above, there's not really any
> >> reason to use qdisc_drop_reason() anymore; so this logic can be
> >> simplified to simply:
> >>
> >>                         if (unlikely(segs == skb))
> >>                                 consume_skb(skb);
> >>                         return qdisc_drop_all(segs, sch, to_free);
> >>
> >
> > How does this look ?
> > tcf_set_qdisc_drop_reason(skb, QDISC_DROP_OVERLIMIT);
> > skb_list_walk_safe(segs, seg, nskb)
> >      tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT);
> >
> > if (unlikely(segs == skb)) {
> >         consume_skb(skb);
> >         return qdisc_drop_all(segs, sch, to_free);
> > }
> > __qdisc_drop_all(segs, to_free);
> > return qdisc_drop(skb, sch, to_free);
>
> Ah, right, got the 'segs' and 'skb' variables a bit confused, sorry
> about that.
>
> But, thinking about it some more:
>
> In both branches of your code above, we have a drop of the skb, and a
> drop_all of the segs. And the existing code already has an unconditional
> consume_skb() after segmentation, where none of the post-segmentation
> uses the 'skb' pointer.
>
> So wouldn't it be simpler to just add the consume_skb() unconditionally
> immediately after the skb_gso_segment() call? Something like this, on
> top of your patch:
>
> diff --git i/net/sched/sch_cake.c w/net/sched/sch_cake.c
> index 730c90af14bd..20173aaf973c 100644
> --- i/net/sched/sch_cake.c
> +++ w/net/sched/sch_cake.c
> @@ -1819,13 +1819,17 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>         cake_hash_keys(skb, flow_mode, flow_override, host_override, &keys);
>
>         if (qdisc_pkt_segs(skb) > 1 && q->config->rate_flags & CAKE_FLAG_SPLIT_GSO) {
> -               struct sk_buff *nskb, *seg;
>                 netdev_features_t features = netif_skb_features(skb);
> +               struct sk_buff *nskb, *seg;
>
>                 segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
>                 if (IS_ERR_OR_NULL(segs))
>                         return qdisc_drop(skb, sch, to_free);
>
> +               /* we never use the original skb after splitting */
> +               consume_skb(skb);
> +               skb = NULL;
> +
>                 /* The split path accounts the segment sum, not the
>                  * stab-adjusted qdisc_pkt_len(), so check it here.
>                  */
> @@ -1842,20 +1846,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                         skb_list_walk_safe(segs, seg, nskb)
>                                 tcf_set_qdisc_drop_reason(seg,
>                                                           QDISC_DROP_OVERLIMIT);
> -                       /* skb_gso_segment() moved sock_wfree to the tail
> -                        * segment, so defer the list to to_free rather
> -                        * than free it under the qdisc lock.
> -                        *
> -                        * Fraglist GSO returns the original skb as head
> -                        * (segs == skb), so drop our ref first.
> -                        */
> -                       if (unlikely(segs == skb)) {
> -                               consume_skb(skb);
> -                               return qdisc_drop_all(segs, sch, to_free);
> -                       }
> -                       __qdisc_drop_all(segs, to_free);
> -                       return qdisc_drop_reason(skb, sch, to_free,
> -                                                QDISC_DROP_OVERLIMIT);
> +
> +                       return qdisc_drop_all(segs, sch, to_free);
>                 }
>         }
>
> @@ -1917,9 +1909,8 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>                 WRITE_ONCE(b->backlogs[idx], b->backlogs[idx] + slen);
>
>                 qdisc_tree_reduce_backlog(sch, 1-numsegs, len-slen);
> -               consume_skb(skb);
>         } else {
> -               /* not splitting */
> +               /* not splitting - skb pointer still valid */
>                 int ack_pkt_len = 0;
>
>                 cobalt_set_enqueue_time(skb, now);
>
> >> > +             }
> >> > +     }
> >> > +
> >> > +     /* Admitted: resolve the flow. The mode is read once above, so
> >> > +      * extraction and resolution agree if cake_change() intervenes.
> >> > +      */
> >>
> >> The last sentence here is just distracting. Please drop.
> >>
> >
> > Ok.Will reword to: /* Admitted: resolve the flow. */
> >
> > I wont wait for the 7 days for sashiko to respond if we can reach a
> > quick consensus.
>
> Fine with me :)

I will resend with your suggestions.

pw-bot: cr

cheers,
jamal
> -Toke