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

On Fri, Sep 4, 2026 at 5:42 AM [off-list ref] 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.

quoted

    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?

quoted

      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?

quoted

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