Re: [PATCH net v3 3/9] net/sched: sfq: clamp quantum in change path

Re: [PATCH net v3 3/9] net/sched: sfq: clamp quantum in change path

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0 (1)

- [High] Incomplete fix: raising the quantum floor to 256 in sfq_change()
  does not bound the deficit-refill loop in sfq_dequeue()…
- [Medium] The reproducer in the commit message cannot reach
  sfq_change().
- [Medium] The commit message's justification contradicts the tree the
  patch is applied to.

Pre-existing issues:
- [Low] Code/comment mismatch: struct sfq_sched_data documents `unsigned
  int quantum; /* Allotment per round: MUST BE >= MTU */`…

--

net/sched: sfq: clamp quantum in change path

This adds two new checks to sfq_change(): quantum values above 1<<20 are
rejected with -EINVAL, and accepted quantum values are raised to a floor
of 256 bytes.  The stated goal is to stop the deficit-refill loop in
sfq_dequeue() from spinning for a very long time under the qdisc lock.

A few questions on the changelog and on how much of the loop is actually
bounded afterwards.

>     sfq_change() accepts any non-negative quantum (only rejects
>     (int)ctl->quantum < 0). With a crafted size table qdisc_pkt_len reaches
>     ~2 GiB, so quantum=1 makes the deficit-refill loop spin ~2^31 times
>     under the qdisc lock (a soft lockup / denial of service).

[Severity: Medium]
Is the "~2 GiB" figure still accurate for this tree?  The tail of
__qdisc_calculate_pkt_len() in net/sched/sch_api.c already ends 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);

with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h.
git blame puts that clamp at commit 8f735d64382dcf, which is already in
the baseline this patch applies to, so qdisc_pkt_len() appears to top out
at 1 MiB rather than ~2 GiB, making the "~2^31 times" number roughly
2048x too large.

Given the Fixes: tag will drive stable triage, would it be worth saying
that the generic pkt_len clamp already bounds the loop in current trees,
and that the 2^31 figure only applies to kernels predating that clamp?

>       tc qdisc add dev dummy0 root sfq
>       tc qdisc change dev dummy0 root sfq quantum 1 stab data 32768 size_log 15 cell_log 0

[Severity: Medium]
Can this second command actually reach sfq_change()?  sfq_qdisc_ops sets:

	.change		=	NULL,

and qdisc_change() in net/sched/sch_api.c rejects the request before
either the options or the size table are applied:

	if (tca[TCA_OPTIONS]) {
		if (!sch->ops->change) {
			NL_SET_ERR_MSG(extack, "Change operation not supported by specified qdisc");
			return -EINVAL;
		}

For "tc qdisc change dev dummy0 root sfq quantum 1 ..." iproute2 always
emits TCA_OPTIONS, and __tc_modify_qdisc() falls through to
qdisc_change() for an existing root qdisc of matching kind, so the
command should return -EINVAL.

The only live caller of sfq_change() looks to be sfq_init(), i.e. tc
qdisc add / replace.  Should the subject ("clamp quantum in change
path"), the narrative and the reproducer be reworded around
add/replace instead?

> diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
> index 187d3ed578f26..8bbcfc9e85d94 100644
> --- a/net/sched/sch_sfq.c
> +++ b/net/sched/sch_sfq.c
> @@ -660,6 +660,11 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
>  		return -EINVAL;
>  	}
>  
> +	if (ctl->quantum > 1 << 20) {
> +		NL_SET_ERR_MSG_MOD(extack, "quantum too large");
> +		return -EINVAL;
> +	}
> +
>  	if (ctl->perturb_period < 0 ||
>  	    ctl->perturb_period > INT_MAX / HZ) {
>  		NL_SET_ERR_MSG_MOD(extack, "invalid perturb period");
> @@ -688,7 +693,7 @@ static int sfq_change(struct Qdisc *sch, struct nlattr *opt,
>  
>  	/* update and validate configuration */
>  	if (ctl->quantum)
> -		quantum = ctl->quantum;
> +		quantum = max(256U, ctl->quantum);

[Severity: High]
Does a 256-byte quantum floor actually bound the refill loop, or only
divide its trip count?  The loop in sfq_dequeue() credits one quantum to
one slot per iteration and stops at the first visited slot with a
positive allot:

net/sched/sch_sfq.c:sfq_dequeue() {
	...
next_slot:
	a = q->tail->next;
	slot = &q->slots[a];
	if (slot->allot <= 0) {
		q->tail = slot;
		WRITE_ONCE(slot->allot, slot->allot + q->quantum);
		goto next_slot;
	}
	...
}

q->tail is not advanced past the slot that was just served; the served
slot is only debited:

	} else {
		WRITE_ONCE(slot->allot, slot->allot - qdisc_pkt_len(skb));
	}

So with a size table pushing qdisc_pkt_len() to QDISC_PKT_LEN_MAX, each
slot served in one round-robin sweep is left at about -1 MiB, and a slot
needs (1 << 20) / 256 = 4096 visits to become positive again.  New flows
start positive in sfq_enqueue():

		/* We could use a bigger initial quantum for new flows */
		WRITE_ONCE(slot->allot, q->quantum);

so N flows can each be driven deeply negative by one dequeue before the
ring wraps.  With flows 65408 (SFQ_MAX_FLOWS) that is on the order of
4096 * 65408 iterations inside a single sfq_dequeue() call, with the root
qdisc spinlock held and BH disabled.

The same arithmetic seems to hold without any small quantum at all: a
default quantum derived from a 1500-byte MTU still gives roughly
(1 << 20) / 1500 * 65408 iterations under the same setup.

Would bounding the deficit itself work better here, for example clamping
or resetting slot->allot when it goes below -q->quantum, or computing the
number of quanta arithmetically rather than looping one quantum at a
time?

[Severity: Low]
This is a pre-existing mismatch and not introduced by this patch, but
while this validation is being touched: struct sfq_sched_data documents

	unsigned int	quantum;	/* Allotment per round: MUST BE >= MTU */

and sfq_init() derives its default from the device MTU:

	q->quantum = clamp_t(u32, psched_mtu(qdisc_dev(sch)),
			     256, 1 << 20);

whereas sfq_change() now stores max(256U, ctl->quantum) without looking
at psched_mtu(), so on a 1500-MTU device a requested quantum of 300 is
stored verbatim.  Should the clamp use psched_mtu() as its lower bound,
or should the "MUST BE >= MTU" comment be updated?

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