Re: [PATCH net v3 7/9] net/sched: drr: clamp quantum in change class

[netdev-bot+sashiko@kernel.org]

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

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

- [High] net/sched/sch_drr.c:85 and :87 (drr_change_class) silently
  rewrite an administrator-supplied TCA_DRR_QUANTUM instead of honouring…
- [Medium] Commit message premise and reproduction recipe do not match
  the tree the patch applies to, and an undeclared stable-backport…

--

net/sched: drr: clamp quantum in change class

This adds a clamp_t(u32, quantum, 256, 1 << 20) in drr_change_class(),
applied both to an explicitly supplied TCA_DRR_QUANTUM and to the
psched_mtu() fallback, so that the deficit-refill loop in drr_dequeue()
cannot iterate on a very small quantum.  The existing rejection of an
explicit quantum of zero is kept.

Two questions below, one about the changelog and one about the new
clamping behaviour.

> drr_change_class() rejects explicit quantum==0 but falls back to
> psched_mtu() with no floor. With a crafted size table qdisc_pkt_len
> reaches ~2 GiB, so quantum=1 (or a zero psched_mtu on a headerless
> device) makes the deficit-refill loop spin under the qdisc lock.

[Severity: Medium]

Is the "~2 GiB" figure still accurate for this tree?  The baseline
already ends __qdisc_calculate_pkt_len() in net/sched/sch_api.c with:

	/* A size table can inflate qdisc_pkt_len() beyond any real packet
	 * (via overhead, the data table, or size_log); cap it so deficit
	 * schedulers such as DRR/ETS terminate their refill loops.
	 */
	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
	qdisc_skb_cb(skb)->pkt_len = pkt_len;

with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.

Since drr_dequeue() advances the deficit by one quantum per round:

	WRITE_ONCE(cl->deficit, cl->deficit + READ_ONCE(cl->quantum));

wouldn't the worst case with quantum=1 be around 1M rounds rather than
~2^31?  The loop bound seems tied to that packet-length cap, not to a
2 GiB length.

Related to that, the recipe given:

>   tc qdisc add dev dummy0 root drr
>   tc class add dev dummy0 parent 1: classid 1:1 drr quantum 1

installs no size table and sends no traffic, so it does not appear to
exercise the refill loop at all.  Could the reproducer be extended to
include the size table and a transmit step?

There is also a backport angle.  With the new floor at 256 the effective
iteration bound (~4096 rounds) comes from QDISC_PKT_LEN_MAX rather than
from this change.  Trees matching "Fixes: 13d2a1d2b032" have no such cap,
so this hunk on its own still allows roughly 8.4M refill iterations per
dequeue there.  Should the QDISC_PKT_LEN_MAX commit be named as a
backport prerequisite?

> diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
> index 91b1ef824afaf..8621d057edd9f 100644
> --- a/net/sched/sch_drr.c
> +++ b/net/sched/sch_drr.c
> @@ -82,8 +82,9 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  			NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot be zero");
>  			return -EINVAL;
>  		}
> +		quantum = clamp_t(u32, quantum, 256, 1 << 20);

[Severity: High]

Should an out-of-range quantum be rewritten rather than rejected here?
Only the exact value 0 returns -EINVAL; every other value now falls
through this clamp and is committed, either via the existing-class path
in drr_change_class():

		if (tb[TCA_DRR_QUANTUM])
			WRITE_ONCE(cl->quantum, quantum);

or via cl->quantum = quantum on the create path.  Netlink then returns
success for a configuration that was not applied.

Does this create a set/get asymmetry?  drr_dump_class() reports the
stored value:

	if (nla_put_u32(skb, TCA_DRR_QUANTUM, READ_ONCE(cl->quantum)))

so "tc class add ... drr quantum 1" succeeds and reads back 256.

Does this also change service ratios silently?  drr_dequeue() refills by
cl->quantum each round, so relative shares are the ratio of class
quanta.  A 10:100 weighting would become 256:256, and 4 MiB:1 MiB would
become 1 MiB:1 MiB, on any later "tc class change ... quantum <n>"
against a class that worked before.

The declared policy still advertises the whole u32 range:

	[TCA_DRR_QUANTUM]	= { .type = NLA_U32 },

The sibling implementation of the same bounded-quantum idea rejects
instead of mutating, in net/sched/sch_qfq.c:

	static const struct netlink_range_validation lmax_range = {
		.min = QFQ_MIN_LMAX,
		.max = QFQ_MAX_LMAX,
	};

	static const struct nla_policy qfq_policy[TCA_QFQ_MAX + 1] = {
		[TCA_QFQ_WEIGHT] = NLA_POLICY_RANGE(NLA_U32, 1, QFQ_MAX_WEIGHT),
		[TCA_QFQ_LMAX] = NLA_POLICY_FULL_RANGE(NLA_U32, &lmax_range),
	};

and the sfq patch in this same series rejects the identical upper bound
in sfq_change():

	if (ctl->quantum > 1 << 20) {
		NL_SET_ERR_MSG_MOD(extack, "quantum too large");
		return -EINVAL;
	}

Would a declared NLA_POLICY_FULL_RANGE (or an explicit -EINVAL plus
extack) be preferable, which would also subsume the zero check?

One more on the upper bound: a quantum larger than qdisc_pkt_len() cannot
add refill rounds, so the 1 << 20 maximum does not seem related to the
loop described in the changelog.  Is the narrowing of the accepted range
intentional, and if so should it be spelled out in the commit message?
The message currently reads:

> Add clamp_t(u32, quantum, 256, 1<<20) after the zero reject and on the
> fallback path. The explicit-zero reject is preserved.

which reads as a hardening change with no behaviour change, and does not
mention that previously accepted values are now altered.  That seems
worth stating given the Fixes: tag implies a stable backport.

>  	} else
> -		quantum = psched_mtu(qdisc_dev(sch));
> +		quantum = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)), 256, 1 << 20);
>  
>  	if (cl != NULL) {
>  		if (tca[TCA_RATE]) {

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