Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

Re: [PATCH net v3 1/9] net/sched: fq: clamp quantum and initial_quantum in change path

[netdev-bot+sashiko]

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

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

- [Medium] Commit message misdescribes the pre-patch behaviour and the
  severity of the bug being fixed.
- [Medium] fq_change() no longer rejects an out-of-range TCA_FQ_QUANTUM.
- [Medium] The `Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds
  to quantum and initial quantum")` tag does not identify the commit…
- [Medium] Narrowing `iq_range.max` from INT_MAX to 1<<20 makes any
  configuration that sets initial_quantum above 1 MiB fail with -ERANGE…
- [Low] The tc-testing case 10f7 "Create FQ with invalid initial_quantum
  setting" uses `initial_quantum 0x80000000` (2147483648), which was…

--

net/sched: fq: clamp quantum and initial_quantum in change path

This narrows the netlink policy range for TCA_FQ_INITIAL_QUANTUM to 1<<20
and replaces the range check on TCA_FQ_QUANTUM in fq_change() with an
unconditional clamp to [256, 1<<20].  fq_init() now also clamps its
default quantum to the same range.

A few questions on the changelog and on the behaviour changes.

> The fq change path accepts TCA_FQ_QUANTUM in [1, INT_MAX] and
> TCA_FQ_INITIAL_QUANTUM up to INT_MAX, while fq_init() already clamps to
> [1, 1<<20]. A user can override the init clamp via tc qdisc change,
> restoring the small-quantum deficit spin that the init clamp prevents.

[Severity: Medium]
Is this description of the pre-patch behaviour accurate?  The code this
patch removes from fq_change() is:

net/sched/sch_fq.c:fq_change() {
	if (tb[TCA_FQ_QUANTUM]) {
		u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);

		if (quantum > 0 && quantum <= (1 << 20)) {
			WRITE_ONCE(q->quantum, quantum);
		} else {
			NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
			err = -EINVAL;
		}
	}

so the accepted range on the change path was [1, 1<<20], not
[1, INT_MAX].

Also, 709f34f7c28d only touched fq_init(), where the clamp prevents
quantum == 0 arising from 2 * psched_mtu() wrapping.  quantum == 0 was
already refused by fq_change() with -EINVAL, so what exactly could be
overridden through tc qdisc change?

On the "spin" itself, __qdisc_calculate_pkt_len() in net/sched/sch_api.c
already bounds the stab-inflated length:

	pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);

With QDISC_PKT_LEN_MAX == (1 << 20), the fq_dequeue() refill loop

	if (f->credit <= 0) {
		f->credit += q->quantum;
		...
		goto begin;
	}

terminates after at most ~2^20 passes for any quantum >= 1.

Would it be more accurate to state that the residual gap closed on the
change path is quantum in [1, 255]?

> Fixes: 709f34f7c28d ("net/sched: fq: add overflow bounds to quantum and initial quantum")

[Severity: Medium]
Does this Fixes tag point at the right commit?  709f34f7c28d changes only
fq_init().  Blame at the parent of this commit attributes the fq_change()
quantum handling to 3725a269815ba6 (2015) with the 1<<20 upper bound from
d9e15a2733067c (2020), and iq_range.max = INT_MAX to 7041101ff6c307
(2023).

The reproducer in the changelog

	tc qdisc change dev dummy0 root fq quantum 1 stab ...

goes only through fq_change(), so it would behave the same on kernels
predating 709f34f7c28d.  Since stable selection keys off the Fixes tag,
should the tag name one of the older commits instead?

> diff --git a/net/sched/sch_fq.c b/net/sched/sch_fq.c
> index 6144b5686f13f..35f940b2205df 100644
> --- a/net/sched/sch_fq.c
> +++ b/net/sched/sch_fq.c
> @@ -980,7 +980,7 @@ static int fq_resize(struct Qdisc *sch, u32 log)
>  }
>  
>  static const struct netlink_range_validation iq_range = {
> -	.max = INT_MAX,
> +	.max = 1 << 20,
>  };

[Severity: Medium]
Is this an intentional uAPI restriction?  With
NLA_POLICY_FULL_RANGE(NLA_U32, &iq_range), validate_nla() now returns
-ERANGE during nla_parse_nested_deprecated(), so

	tc qdisc add dev dummy0 root fq initial_quantum 2000000

fails outright and fq_change() never runs.  Values above 1 MiB have been
accepted since 7041101ff6c307, and unvalidated before that.

What bug does a large initial_quantum cause?  In sch_fq.c the only uses of
q->initial_quantum are the two assignments in fq_classify()

	f->credit = q->initial_quantum;

plus the dump, while the refill loop in fq_dequeue() adds q->quantum.  So
initial_quantum looks like a per-new-flow burst knob that cannot drive the
refill loop.

Should the changelog mention the compatibility break for existing tc
scripts?

>  
>  static const struct nla_policy fq_policy[TCA_FQ_MAX + 1] = {
> @@ -1106,14 +1106,10 @@ static int fq_change(struct Qdisc *sch, struct nlattr *opt,
>  			   nla_get_u32(tb[TCA_FQ_FLOW_PLIMIT]));
>  
>  	if (tb[TCA_FQ_QUANTUM]) {
> -		u32 quantum = nla_get_u32(tb[TCA_FQ_QUANTUM]);
> +		u32 quantum = clamp_t(u32, nla_get_u32(tb[TCA_FQ_QUANTUM]),
> +				     256, 1 << 20);
>  
> -		if (quantum > 0 && quantum <= (1 << 20)) {
> -			WRITE_ONCE(q->quantum, quantum);
> -		} else {
> -			NL_SET_ERR_MSG_MOD(extack, "invalid quantum");
> -			err = -EINVAL;
> -		}
> +		WRITE_ONCE(q->quantum, quantum);
>  	}

[Severity: Medium]
Is the loss of the -EINVAL here intended?  After this change,

	tc qdisc change dev dummy0 root fq quantum 0

returns success and installs 256, where it previously returned -EINVAL
with the "invalid quantum" extack (that rejection dates back to
3725a269815ba6, with the upper bound from d9e15a2733067c).  Values in
[1, 255] that were previously honoured are now silently rewritten to 256.

There is a second, less obvious effect: this branch can no longer set err,
so the later blocks in fq_change()

	if (!err && tb[TCA_FQ_PRIOMAP])
		err = fq_load_priomap(...);

	if (!err && tb[TCA_FQ_WEIGHTS])
		err = fq_load_weights(...);

	if (!err) {
		sch_tree_unlock(sch);
		err = fq_resize(sch, fq_log);
		sch_tree_lock(sch);
	}

now all run for a request that carries an invalid quantum together with
priomap, weights or a new buckets_log.  Previously such a request was
refused as a whole.  Was that change in behaviour considered?

The same file still rejects out-of-range band quanta in fq_load_weights()
with

	NL_SET_ERR_MSG_MOD(extack, "invalid weights");

and fq_codel_change() keeps the split convention:

net/sched/sch_fq_codel.c:fq_codel_change() {
		quantum = max(256U, nla_get_u32(tb[TCA_FQ_CODEL_QUANTUM]));
		if (quantum > FQ_CODEL_QUANTUM_MAX) {
			NL_SET_ERR_MSG(extack, "Invalid quantum");
			return -EINVAL;
		}
}

Within this patch, TCA_FQ_INITIAL_QUANTUM is hard-rejected at parse time
while its neighbour TCA_FQ_QUANTUM is silently clamped.  Could the two
adjacent attributes signal out-of-range input the same way, i.e. keep the
-EINVAL for quantum above 1<<20 and only clamp the low end?

>  
>  	if (tb[TCA_FQ_INITIAL_QUANTUM])

[ ... ]

[Severity: Low]
This isn't a bug, but is there test coverage for the new bounds?  The
tdc case 10f7 "Create FQ with invalid initial_quantum setting" in
tools/testing/selftests/tc-testing/tc-tests/qdiscs/fq.json uses

	initial_quantum 0x80000000

which is 2147483648 and was already refused by the old INT_MAX bound, so
it passes identically before and after this patch and would keep passing
if the 1<<20 bound were reverted.  Case 845b uses initial_quantum 900000,
below the new bound, and the only quantum case 2390 uses quantum 9000,
inside the new clamp.

Nothing in fq.json exercises 0x100000 accepted versus 0x100001 rejected,
and nothing exercises quantum 0, quantum 1 (the reproducer named in the
changelog) or quantum above 1<<20.  The series does update tc-testing for
the ETS clamp but adds nothing for fq.  Could fq.json be extended along
with this change?

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