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
