* Re: [PATCH net v3 3/9] net/sched: sfq: clamp quantum in change path
2026-09-04 9:42 ` [PATCH net v3 3/9] net/sched: sfq: clamp quantum in change path netdev-bot+sashiko
@ 2026-09-04 18:53 ` Jamal Hadi Salim
0 siblings, 0 replies; 2+ messages in thread
From: Jamal Hadi Salim @ 2026-09-04 18:53 UTC (permalink / raw)
To: netdev-bot+sashiko
Cc: netdev, jiri, davem, edumazet, kuba, pabeni, horms, victor, vega,
shuah, linux-kselftest, toke
On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> 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 */`…
>
>
Ok, so 4 issues:
1. [High] "256 floor does not bound the deficit-refill walk"
This is valid but NOT introduced by this patch.
This is a pre-existing residual of the same family (the guard belongs
at the refill site in sfq_dequeue(), exactly like the fq_codel case);
Again, it is NOT introduced by this patch and does not invalidate the clamp.
Filed as a separate follow-up patch; will reference whatever this
commit is when it gets merged on that followup.
2. [Medium] "reproducer cannot reach sfq_change()"
Valid. sfq_change() is only wired to sfq_init()/add-replace, not
change. Changelog wording issue only. Only if i have to repost.
3. [Medium] "justification contradicts the tree"
QDISC_PKT_LEN_MAX caps pkt_len at 1 MiB on the tree. So i wont do
anything with this.
4. [Low] "comment says MUST BE >= MTU"
noted; the 256 floor supersedes the comment's assumption; comment
refresh belongs with any future sfq patch.
Summary: i dont see anything here either that requires a resend.
cheers,
jamal
>
> 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
^ permalink raw reply [flat|nested] 2+ messages in thread