Re: [PATCH net-next v10 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit fixes three real bugs in cake_enqueue()
  (net/sched/sch_cake.c) but carries no Fixes: tag, so a stable…
- [Low] The "Conditions to recreate the bug" section does not produce any
  visible failure on the unpatched kernel.

--

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

This splits cake_hash() into a side-effect-free cake_hash_keys() and a
cake_hash_resolve() that runs only after admission. The split-GSO path
moves ahead of resolution, with a ceiling check on the segment sum and
per-segment drop reason tagging. flow_mode is read once with READ_ONCE()
in cake_enqueue().

Link: https://lore.kernel.org/netdev/179231933068.3135.80340792633077986@kernel.org/
Link: https://lore.kernel.org/netdev/179269577270.3135.16072065138845192450@kernel.org/

> net/sched: cake: hash before segmenting and reject an overlong split
>
> 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

[ ... ]

> 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(), and to
> the later flowchain calls (cake_get_flow_quantum(),
> cake_inc_srchost_bulk_flow_count(), cake_inc_dsthost_bulk_flow_count()).
> cake_mq_change() updates the shared cake_config outside the qdisc lock, so
> a reconfiguration could extract and resolve keys in different modes.

[Severity: Low]
The message calls these changes fixes, but there is no Fixes: tag. Could
Fixes: tags be added, so stable backporters can tell which trees need
which part?

The three problems fixed in cake_enqueue() seem to come from different
commits.

First, the set-associative state (q->tags[], host indices, way counters)
was committed before a segmentation failure dropped the packet. That
ordering goes back to the split-GSO path:

  0c850344d388 ("sch_cake: Conditionally split GSO segments")

Second, the segmentation-failure path called plain qdisc_drop():

	segs = skb_gso_segment(skb, features & ~NETIF_F_GSO_MASK);
	if (IS_ERR_OR_NULL(segs))
		return qdisc_drop(skb, sch, to_free);

By then __skb_gso_segment() had overwritten tc_skb_cb.drop_reason. That
only became a misreport once __dev_xmit_skb() started setting
QDISC_DROP_GENERIC and __tcf_kfree_skb_list() started reading the field:

  ff2998f29f39 ("net: sched: introduce qdisc-specific drop reason tracing")

Third, flow_mode could change between key extraction and resolution.
That depends on the shared cake_mq config, where cake_mq_change() calls
cake_config_change() without the child qdisc lock:

  bc0ce2bad36c ("net/sched: sch_cake: Factor out config variables into separate struct")
  ebc65a873eff ("net/sched: sch_cake: Add cake_mq qdisc for using cake on mq devices")
  87826c01837c ("net/sched: sch_cake: Share config across cake_mq sub-qdiscs")

The drop reason fix only matters on kernels that have ff2998f29f39. The
flow_mode fix only matters on kernels that have the shared cake_mq
config.

> 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
>   # hold the tun fd open without reading so the backlog persists; a stab
>   # overhead of 1048468 accounts each 92-byte packet at 1048560 bytes, so
>   # 4095 queued packets leave 65519 bytes below QDISC_MAX_BACKLOG.  A
>   # 65000-byte UDP_SEGMENT payload with a 1400-byte segment size then has
>   # a segment sum past the remaining headroom, and cake rejects the whole
>   # split.  Setup needs CAP_NET_ADMIN; the payload itself does not.
>   tc qdisc add dev tun0 root handle 1: stab overhead 1048468 \
>       cake split-gso memlimit 512Mb

[Severity: Low]
Does this reproducer show any failure on the unpatched kernel? It seems
to only describe a case that the patched cake_enqueue() now rejects.

Working through the numbers:

  QDISC_MAX_BACKLOG = U32_MAX - (1 << 20) = 4293918719
  4095 * 1048560 = 4293853200, leaving 65519 bytes of headroom

The GSO skb's stab-adjusted length is 66316 + 1048468.
__qdisc_calculate_pkt_len() clamps it:

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
    ...
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
    ...
}

So qdisc_backlog_overflow() takes the first branch and admits the
packet:

include/net/sch_generic.h:qdisc_backlog_overflow() {
    ...
	if (likely(len <= QDISC_PKT_LEN_MAX))
		return backlog > QDISC_MAX_BACKLOG;
	return len > U32_MAX - backlog;
}

The 65000-byte payload with a 1400-byte segment size splits into 47
segments, so slen = 65000 + 47 * 28 = 66316.

On the parent commit the backlog then reaches 4293919516. That is 797
bytes over QDISC_MAX_BACKLOG but about 1 MiB below U32_MAX, so nothing
wraps. The next enqueue is rejected by qdisc_backlog_overflow() anyway.

The core helper itself lets the backlog go past QDISC_MAX_BACKLOG when a
packet of up to 1 MiB arrives while the backlog is still at or below the
ceiling. So going past the ceiling is not a failure on its own.

A real wrap in the split path seems to need slen above roughly 1 MiB
while the clamped pkt_len still passes the helper's check, for example a
trusted small-MSS BIG TCP aggregate. Could the message describe that
case, so maintainers can judge the severity and stable relevance?

The subject line also says "hash before segmenting", but the parent code
already called cake_hash(b, skb, ...) before skb_gso_segment(). The real
change is that cake_hash_resolve() now waits until after admission, and
segmentation and the ceiling check now run before it. Could the subject
say that instead?

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