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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the new overlimit rejection branch of cake_enqueue()
  (net/sched/sch_cake.c), only one skb gets the QDISC_DROP_OVERLIMIT…
- [Low] Before this patch, cake_enqueue() read q->config->flow_mode once
  and passed that one value to cake_hash(), so key extraction and set-…
- [Low] The commit message calls itself a bug fix ('The Fix:') for two
  defects that go back to when split-GSO support was added to cake.

--

net/sched: cake: hash before segmenting and reject an overlong split

This commit splits cake_hash() into two parts. cake_hash_keys() has no
side effects and runs on the unsegmented skb. cake_hash_resolve() commits
the set-associative state, and only runs once the packet is admitted.

It also moves split-GSO segmentation ahead of the resolution. The summed
segment length is checked against QDISC_MAX_BACKLOG, and the whole list
is rejected before any state is committed.

> The Fix:
> with CAKE_FLAG_SPLIT_GSO, cake computes the flow/host hash keys, then
> splits a GSO skb into segments it accounts individually.

[ ... ]

> Reviewed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

[Severity: Low]
This isn't a bug, but should this have a Fixes: tag?

The commit message calls this a fix. Both problems it addresses go back
to when split-GSO support was added to cake:

With CAKE_FLAG_SPLIT_GSO, slen was added to the 32-bit
sch->qstats.backlog without any ceiling check. slen can be larger than
the headroom that qdisc_backlog_at_max() reserves.

A GSO skb whose skb_gso_segment() failed was dropped after cake_hash()
and the shaper state update had already committed state.

The new check also depends on QDISC_MAX_BACKLOG, which comes from the
preceding patch in the series ("net/sched: cap the accounted backlog
before it can wrap"). Any backport would need both patches.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index adfe11737c66d..c67db1c61a90b 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c

[ ... ]

> @@ -1787,12 +1814,55 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  					 QDISC_DROP_OVERLIMIT);
>  	}
>  
> -	/* choose flow to insert into */
> +	/* Tin selection and key extraction run on the unsegmented skb:
> +	 * skb_gso_segment() rewrites an encapsulated GSO packet's headers to
> +	 * the inner ones, changing the keys. Neither commits state.
> +	 */
>  	b = cake_select_tin(sch, skb);
> -	idx = cake_hash(b, skb, q->config->flow_mode, flow_override,
> -			host_override) + 1;
> +	cake_hash_keys(skb, q->config->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 the top check does not
> +		 * bound it. Sum the list and reject it whole.
> +		 */
> +		skb_list_walk_safe(segs, seg, nskb)
> +			slen += seg->len;
> +
> +		if (unlikely((u64)sch->qstats.backlog + slen > QDISC_MAX_BACKLOG)) {
> +			qdisc_qstats_overlimit(sch);

[ ... ]

> +			if (unlikely(segs == skb)) {
> +				tcf_set_qdisc_drop_reason(skb,
> +							  QDISC_DROP_OVERLIMIT);
> +				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);
> +		}
> +	}

[Severity: Medium]
Do the segments placed on to_free here get a valid drop reason?

When segs != skb, qdisc_drop_reason() tags only the original skb. When
segs == skb, tcf_set_qdisc_drop_reason() tags only the head. Every other
segment keeps whatever value is in tc_skb_cb(seg)->drop_reason.

That field is at cb offset 32, right after the 32-byte struct
qdisc_skb_cb. SKB_GSO_CB_OFFSET is also 32, and __skb_gso_segment()
writes to that location:

net/core/gso.c:__skb_gso_segment() {
    ...
	SKB_GSO_CB(skb)->mac_offset = skb_headroom(skb);
    ...
}

This overwrites the QDISC_DROP_GENERIC value set by __dev_xmit_skb().
__copy_skb_header() then copies the cb into every segment.

__tcf_kfree_skb_list() reads the reason separately for each skb:

net/sched/sch_generic.c:__tcf_kfree_skb_list() {
    ...
		u32 reason = tc_skb_cb(skb)->drop_reason;
    ...
		if ((reason & SKB_DROP_REASON_SUBSYS_MASK) == __QDISC_DROP_REASON) {
			trace_qdisc_drop(q, txq, dev, skb, (enum qdisc_drop_reason)reason);
			skb_reason = SKB_DROP_REASON_QDISC_DROP;
		} else {
			skb_reason = (enum skb_drop_reason)reason;
		}
		kfree_skb_reason(skb, skb_reason);
    ...
}

A headroom-sized value has no qdisc subsystem bits. For those segments,
trace_qdisc_drop would be skipped, and kfree_skb_reason() would report an
unrelated core drop reason. A value of 1 would show up as SKB_CONSUMED,
and a value of 0 would hit the DEBUG_NET_WARN_ON_ONCE in
__sk_skb_reason_drop().

The qdisc_backlog_at_max() rejection at the top of cake_enqueue() tags
its drop correctly.

Could the existing slen-summing skb_list_walk_safe() loop call
tcf_set_qdisc_drop_reason(seg, QDISC_DROP_OVERLIMIT) on each segment?

> +
> +	/* Admitted: resolve the keys and commit flow/shaper state. */
> +	idx = cake_hash_resolve(b, &keys, q->config->flow_mode);

[Severity: Low]
Can the two reads of q->config->flow_mode see different values?

Before this patch, cake_hash() got a single flow_mode value, so key
extraction and resolution always used the same mode. Now
cake_hash_keys() and cake_hash_resolve() each do their own plain load.
On the split-GSO path, skb_gso_segment() and the slen walk run between
the two loads.

cake_change() calls cake_config_change(), which does
WRITE_ONCE(q->flow_mode, flow_mode) before sch_tree_lock() is taken. That
write is not serialised against cake_enqueue().

For example, keys built in a flows-only mode have srchost_hash and
dsthost_hash set to 0. If cake_hash_resolve() then runs in a dual-srchost
or dual-dsthost mode, allocate_src or allocate_dst is set and the flow is
tied to the host slot for hash 0. Later packets take the tags-match fast
path, so this assignment stays until the flow drains.

Would it be better to read the mode once with
READ_ONCE(q->config->flow_mode) and pass that value to both helpers?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-BA27.v6.r0.20261014054826%40mojatatu.com