Re: [PATCH net-next v7 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] cake_enqueue() (net/sched/sch_cake.c:1825-1827): when
  skb_gso_segment() fails with IS_ERR_OR_NULL(segs), the original skb is…
- [Low] The commit presents itself as a bug fix ('The Fix:') with user-
  visible effects but has no Fixes: trailer.
- [Low] The commit message says cake_enqueue() reads q->config->flow_mode
  'once with READ_ONCE()'.

--

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

This commit splits cake_hash() into a side-effect-free cake_hash_keys(),
which runs on the unsegmented skb, and cake_hash_resolve(), which runs
only once the packet is admitted. The split-GSO path moves ahead of the
resolution: it sums the segment list, checks the total against
QDISC_MAX_BACKLOG and, on rejection, defers the whole list to to_free
with every segment tagged QDISC_DROP_OVERLIMIT. flow_mode is read once
with READ_ONCE() and passed to both hash helpers.

Link: https://lore.kernel.org/netdev/179231933068.3135.80340792633077986@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.

[ ... ]

> 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>

[Severity: Low]
This isn't a bug, but the commit is presented as a fix and has no Fixes:
tag.

It adds the missing check of the segment sum against QDISC_MAX_BACKLOG on
the CAKE_FLAG_SPLIT_GSO path. It also stops cake_hash() state from being
committed for packets that are then dropped. The split-GSO accounting
dates back to when CAKE_FLAG_SPLIT_GSO was added to sch_cake.

Could a Fixes: tag be added so backporters know which kernels need this?

> 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

[ ... ]

> @@ -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);

[Severity: Low]
The commit message says:

  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().

Is the snapshot complete? Further down in the same cake_enqueue() call,
the flowchain code still reloads the live config with plain loads:

net/sched/sch_cake.c:cake_enqueue() {
    ...
		WRITE_ONCE(flow->deficit, cake_get_flow_quantum(b, flow, q->config->flow_mode));
	} else if (flow->set == CAKE_SET_SPARSE_WAIT) {
    ...
		cake_inc_srchost_bulk_flow_count(b, flow, q->config->flow_mode);
		cake_inc_dsthost_bulk_flow_count(b, flow, q->config->flow_mode);
	}
    ...
}

cake_config_change() publishes the new mode with WRITE_ONCE() before
cake_change() takes sch_tree_lock(). A concurrent tc change can therefore
let cake_hash_resolve() run with a mode that lacks the dual src/dst bits,
so flow->srchost and flow->dsthost are not updated. The later reload can
then see the dual mode and charge hosts[] bulk counts at stale indices.

These plain loads also race with the WRITE_ONCE() store, which KCSAN can
report.

Would passing the local flow_mode to these three calls as well keep one
mode for the whole enqueue?

>  	int len = qdisc_pkt_len(skb), ret;
>  	u16 flow_override, host_override;
> +	struct sk_buff *segs = NULL;

[ ... ]

> +	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);

[Severity: Medium]
The comment a few lines below says SKB_GSO_CB sits over
tc_skb_cb.drop_reason. Doesn't the same aliasing affect this failure
return?

__skb_gso_segment() writes the headroom into that slot before calling
skb_mac_gso_segment(), and that call can still fail. Examples are
-EINVAL on malformed SKB_GSO_DODGY headers from tun/virtio, or -ENOMEM in
skb_segment():

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

This overwrites the QDISC_DROP_GENERIC that __dev_xmit_skb() set before
enqueue. qdisc_drop() does not write a reason back. __tcf_kfree_skb_list()
then reads the headroom value, finds no qdisc subsystem bits, and passes it
to kfree_skb_reason() as a core skb_drop_reason:

net/sched/sch_generic.c:__tcf_kfree_skb_list() {
    ...
		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);
    ...
}

As a result, the drop gets an unrelated core reason and trace_qdisc_drop
is not emitted.

This return is unchanged from the old code. Since the patch moves this
block and re-tags the reject path for this exact aliasing, could this path
use qdisc_drop_reason(skb, sch, to_free, QDISC_DROP_GENERIC) or a more
specific reason?

> +
> +		/* 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);

[ ... ]

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