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

[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] Two new comments in net/sched/sch_cake.c say the TC action has
  consumed the packet when the helper returns true.

--

net/sched: cake: return the classifier result to the caller

This commit splits the external classifier out of cake_classify() into a
new helper, cake_tcf_classify(). The helper now returns the filter's
flow and host overrides, and cake_enqueue() does the tin selection and
flow hashing itself. It is described as preparation for later
accounting changes, with no change to classification behaviour.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..6432d10c992e5 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).
> + */

[Severity: Low]
This isn't a bug, but is "consumed the packet" accurate here?

For TC_ACT_SHOT the action does not take ownership of the skb.
tcf_gact_act() in net/sched/act_gact.c only updates its stats and
returns the verdict:

	if (action == TC_ACT_SHOT)
		tcf_action_inc_drop_qstats(&gact->common);

	tcf_lastuse_update(&gact->tcf_tm);

	return action;

Even so, cake_tcf_classify() returns true for it. The
STOLEN/QUEUED/TRAP cases work the same way, because the qdisc still
drops its own reference.

[ ... ]

> @@ -1758,20 +1762,25 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,

[ ... ]

> -	/* 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;
>  	}

[Severity: Low]
This comment has the same wording. The very next lines still have to
free the skb with __qdisc_drop(skb, to_free), so the action has not
consumed it.

A true return really means "do not enqueue, the caller must drop the
skb". Could both comments say that instead?

If a later caller trusted the current wording, it could skip the free
and leak the skb. Both comments are unchanged at the end of the
series.

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