Re: [PATCH net v3 8/9] net/sched: ets: clamp quantum in parse and fallback paths

[netdev-bot+sashiko@kernel.org]

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

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

- [High] ets_quantum_parse() (net/sched/sch_ets.c:82-88) no longer
  validates user input at all: the explicit rejection of a zero quantum…
- [Medium] The clamping added to ets_quantum_parse() contradicts in-tree
  tdc test cases that are NOT updated anywhere in this series.
- [Medium] The commit message's "Conditions to recreate the bug" command,
  `tc qdisc add dev dummy0 root ets bands 3 strict 2 quanta 1 1`, cannot…
- [Medium] The changelog's causal premise — "With a crafted size table
  qdisc_pkt_len reaches ~2 GiB" — is stale for this tree.
- [Low] The comment introduced in ets_qdisc_change() states the fallback
  bands get a "quantum of max(MTU, 256)", but the code immediately below…

--

net/sched: ets: clamp quantum in parse and fallback paths

This moves the quantum floor from the psched_mtu() fallback in
ets_qdisc_change() into ets_quantum_parse(), so that both explicitly
configured quanta and the MTU-derived fallback are clamped to the range
[256, 1<<20].

A few questions about the changelog and about the parse path below.

> net/sched: ets: clamp quantum in parse and fallback paths
>
> ets_qdisc_change() falls back to psched_mtu() with no floor for bands
> without an explicit quantum. With a crafted size table qdisc_pkt_len
> reaches ~2 GiB, so a zero psched_mtu on a headerless device makes the
> deficit-refill loop spin under the qdisc lock.

[Severity: Medium]
Is the "~2 GiB" premise still accurate for this tree?  The baseline already
carries a 1 MiB cap on qdisc_pkt_len:

include/net/pkt_sched.h:
    #define QDISC_PKT_LEN_MAX	(1 << 20)	/* 1 MiB */

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

with the comment there saying the value is capped so that deficit
schedulers such as DRR/ETS terminate their refill loops.

If qdisc_pkt_len cannot exceed 1 MiB, then the only non-terminating case
left in ets_qdisc_dequeue() is quantum == 0, where

	cl->deficit += READ_ONCE(cl->quantum);

never grows and the while (1) loop spins under the qdisc lock.  That case
comes purely from the unfloored psched_mtu() fallback on a device whose
MTU + hard_header_len is 0.  With the 1 MiB packet-length cap plus the new
floor of 256 the loop is bounded to 4096 refill passes, so the fix looks
adequate; could the changelog describe the actual current failure mode
instead?

> Conditions to recreate the bug:
>   CONFIG_NET_SCH_ETS=y. Requires CAP_NET_ADMIN (namespace-local via
>   unshare -Urn suffices).
>
>   tc qdisc add dev dummy0 root ets bands 3 strict 2 quanta 1 1

[Severity: Medium]
Does this command actually install a qdisc?  ets_qdisc_quanta_parse()
starts assignment at the first non-strict band:

net/sched/sch_ets.c:ets_qdisc_quanta_parse() {
	int band = nstrict;
    ...
		case TCA_ETS_QUANTA_BAND:
			if (band >= nbands) {
				NL_SET_ERR_MSG_MOD(extack, "ETS quanta has more values than bands");
				return -EINVAL;
			}
    ...
}

With bands 3 and strict 2, the first quantum lands in quanta[2] and band
becomes 3, so the second quantum hits band >= nbands and the whole add
fails with -EINVAL.  The tdc case fe3c (bands 4 strict 2 quanta 1000 2000
3000, expExitCode 1) encodes that same kernel-side rejection.

The quoted command also has neither of the two ingredients the message
says are needed: there is no size table (stab ...) to inflate
qdisc_pkt_len, and dummy0 has psched_mtu() of 1500 + 14 rather than 0.
Could a reproducer be given that reaches the psched_mtu() == 0 fallback,
so the Fixes: tag can be evaluated by stable backporters?

> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index 25fcf4079fece..6cc902a038387 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c
> @@ -83,11 +83,7 @@ static int ets_quantum_parse(struct Qdisc *sch, const struct nlattr *attr,
>  			     unsigned int *quantum,
>  			     struct netlink_ext_ack *extack)
>  {
> -	*quantum = nla_get_u32(attr);
> -	if (!*quantum) {
> -		NL_SET_ERR_MSG(extack, "ETS quantum cannot be zero");
> -		return -EINVAL;
> -	}
> +	*quantum = clamp_t(u32, nla_get_u32(attr), 256, 1 << 20);
>  	return 0;
>  }

[Severity: High]
Does this change user visible behaviour in two ways that the changelog
does not mention?  The changelog only says the floor is moved into
ets_quantum_parse().

First, the -EINVAL for a zero quantum disappears.  ets_quantum_parse() is
the sole validation point for TCA_ETS_QUANTA_BAND, reached from both
ets_qdisc_quanta_parse() and ets_class_change(), and it can no longer
fail:

	tc qdisc add ... ets quanta 1000 0 800 700
	tc class change ... ets quantum 0

Both used to return -EINVAL with an extack; now they succeed with quantum
silently set to 256.  The extack argument and the int return of
ets_quantum_parse() are both dead now, and the if (err) branch in
ets_class_change() is unreachable.

Second, a new 1 MiB ceiling is applied to explicitly configured quanta, so
quanta 4194304 1048576 (a 4:1 ratio) becomes 1:1, and quanta 1 1000000
becomes 256:1000000.  The installed band ratios and the weights handed to
hardware in ets_offload_change() then differ from what was asked for.

The mutated value is stored in cl->quantum and echoed back by
ets_qdisc_dump():

	if (nla_put_u32(skb, TCA_ETS_QUANTA_BAND,
			READ_ONCE(q->classes[band].quantum)))

so user space can only discover the change by re-reading and diffing its
own configuration.  The declared policies still advertise the full u32
range:

	static const struct nla_policy ets_quanta_policy[TCA_ETS_MAX + 1] = {
		[TCA_ETS_QUANTA_BAND] = { .type = NLA_U32 },
	};

Would it be better to follow the sibling patch in this series,
"net/sched: drr: clamp quantum in change class", which keeps the explicit
rejection and clamps afterwards?

net/sched/sch_drr.c:drr_change_class() {
    ...
		if (quantum == 0) {
			NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
			return -EINVAL;
		}
		quantum = clamp_t(u32, quantum, 256, 1 << 20);
    ...
}

Only the internal psched_mtu()-derived fallback needs the floor for
ets_qdisc_dequeue()'s refill loop to terminate.

[Severity: Medium]
Do two in-tree tdc cases still assert the removed rejection?  In
tools/testing/selftests/tc-testing/tc-tests/qdiscs/ets.json, case e51a
("Zero element in quanta"):

    "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 1000 0 800 700",
    "expExitCode": "1",
    "matchPattern": "qdisc ets",
    "matchCount": "0",

and case e7f2 ("Sole zero element in quanta"):

    "cmdUnderTest": "$TC qdisc add dev $DUMMY handle 1: root ets quanta 0",
    "expExitCode": "1",
    "matchPattern": "qdisc ets",
    "matchCount": "0",

After the clamp, 0 becomes 256, the add succeeds, tc exits 0 and the qdisc
is present, so both cases fail on expExitCode and on matchCount.  Exit
code 1 in this file is the kernel-rejection code (cf. f7c8 and fe3c),
versus 2 for tc-side rejection in 65b6 and 9877.

Diffing ets.json between this commit and the end of the series
(2ebfd9fc19fbc4d4615473751bf847fc57e820fa) shows only case 41f5 changed by
"selftests: tc-testing: update ETS test 41f5 for clamped quanta", so e51a
and e7f2 are still broken at the tip of the series.  Could they be updated
in the same series, or the zero rejection kept so they keep passing?

For completeness, the related 41f5 concern (the dump no longer echoing
quanta 4294967294 1 1) is resolved later in the series by that same
commit, which rewrites the pattern to "quanta 1048576 256 256".

>  
> @@ -632,11 +628,13 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
>  			return err;
>  	}
>  	/* If there are more bands than strict + quanta provided, the remaining
> -	 * ones are ETS with quantum of MTU. Initialize the missing values here.
> +	 * ones are ETS with quantum of max(MTU, 256). Initialize the missing
> +	 * values here.
>  	 */
>  	for (i = nstrict; i < nbands; i++) {
>  		if (!quanta[i])
> -			quanta[i] = psched_mtu(qdisc_dev(sch));
> +			quanta[i] = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)),
> +					    256, 1 << 20);
>  	}

[Severity: Low]
This isn't a bug, but the comment says max(MTU, 256) while the code below
it computes min(max(MTU, 256), 1 MiB).  The upper bound is missing from
the comment, so on a device where psched_mtu() exceeds 1 MiB the described
behaviour and the actual behaviour differ.

>  
>  	/* Before commit, make sure we can allocate all new qdiscs */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com