On Fri, Sep 4, 2026 at 5:42 AM <[email protected]> wrote: > > Thank you for your contribution! Sashiko AI review found 2 potential > issue(s) to consider: > > Critical: 0 · High: 1 · Medium: 1 · Low: 0 > > - [High] net/sched/sch_drr.c:85 and :87 (drr_change_class) silently > rewrite an administrator-supplied TCA_DRR_QUANTUM instead of honouring…
Valid as a "description". My view is the patch is fine. The rewrite-vs-reject choice is the series' deliberate uAPI - the whole point is that a too-small quantum is crazy as illustrated by the series so it is never installed... I am not sure if sashiko is looking at the net tree which has commit 8f735d64382d but qdisc_pkt_len cap which is clamp-style already, and Eric accepted v3 on this basis. The asymmetry is discoverable by re-read (dump echoes the stored value). Summary: If the series is resent, we will state the rewrite behaviour in the changelog. > - [Medium] Commit message premise and reproduction recipe do not match > the tree the patch applies to, and an undeclared stable-backport… > Very valid, if i understood correctly. I think you mean: backporting on trees matching Fixes: 13d2a1d2b032 there is no QDISC_PKT_LEN_MAX cap, so things will go crazy there... I would say It's worth mentioning 8f735d64382d as a backport prerequisite not sure if that's in the commit, although i am not familiar enough with the backport rules to make that call. +Cc [email protected] cheers, jamal > net/sched: drr: clamp quantum in change class > > This adds a clamp_t(u32, quantum, 256, 1 << 20) in drr_change_class(), > applied both to an explicitly supplied TCA_DRR_QUANTUM and to the > psched_mtu() fallback, so that the deficit-refill loop in drr_dequeue() > cannot iterate on a very small quantum. The existing rejection of an > explicit quantum of zero is kept. > > Two questions below, one about the changelog and one about the new > clamping behaviour. > > > drr_change_class() rejects explicit quantum==0 but falls back to > > psched_mtu() with no floor. With a crafted size table qdisc_pkt_len > > reaches ~2 GiB, so quantum=1 (or a zero psched_mtu on a headerless > > device) makes the deficit-refill loop spin under the qdisc lock. > > [Severity: Medium] > > Is the "~2 GiB" figure still accurate for this tree? The baseline > already ends __qdisc_calculate_pkt_len() in net/sched/sch_api.c 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); > qdisc_skb_cb(skb)->pkt_len = pkt_len; > > with QDISC_PKT_LEN_MAX defined as (1 << 20) in include/net/pkt_sched.h. > > Since drr_dequeue() advances the deficit by one quantum per round: > > WRITE_ONCE(cl->deficit, cl->deficit + READ_ONCE(cl->quantum)); > > wouldn't the worst case with quantum=1 be around 1M rounds rather than > ~2^31? The loop bound seems tied to that packet-length cap, not to a > 2 GiB length. > > Related to that, the recipe given: > > > tc qdisc add dev dummy0 root drr > > tc class add dev dummy0 parent 1: classid 1:1 drr quantum 1 > > installs no size table and sends no traffic, so it does not appear to > exercise the refill loop at all. Could the reproducer be extended to > include the size table and a transmit step? > > There is also a backport angle. With the new floor at 256 the effective > iteration bound (~4096 rounds) comes from QDISC_PKT_LEN_MAX rather than > from this change. Trees matching "Fixes: 13d2a1d2b032" have no such cap, > so this hunk on its own still allows roughly 8.4M refill iterations per > dequeue there. Should the QDISC_PKT_LEN_MAX commit be named as a > backport prerequisite? > > > diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c > > index 91b1ef824afaf..8621d057edd9f 100644 > > --- a/net/sched/sch_drr.c > > +++ b/net/sched/sch_drr.c > > @@ -82,8 +82,9 @@ static int drr_change_class(struct Qdisc *sch, u32 > > classid, u32 parentid, > > NL_SET_ERR_MSG(extack, "Specified DRR quantum cannot > > be zero"); > > return -EINVAL; > > } > > + quantum = clamp_t(u32, quantum, 256, 1 << 20); > > [Severity: High] > > Should an out-of-range quantum be rewritten rather than rejected here? > Only the exact value 0 returns -EINVAL; every other value now falls > through this clamp and is committed, either via the existing-class path > in drr_change_class(): > > if (tb[TCA_DRR_QUANTUM]) > WRITE_ONCE(cl->quantum, quantum); > > or via cl->quantum = quantum on the create path. Netlink then returns > success for a configuration that was not applied. > > Does this create a set/get asymmetry? drr_dump_class() reports the > stored value: > > if (nla_put_u32(skb, TCA_DRR_QUANTUM, READ_ONCE(cl->quantum))) > > so "tc class add ... drr quantum 1" succeeds and reads back 256. > > Does this also change service ratios silently? drr_dequeue() refills by > cl->quantum each round, so relative shares are the ratio of class > quanta. A 10:100 weighting would become 256:256, and 4 MiB:1 MiB would > become 1 MiB:1 MiB, on any later "tc class change ... quantum <n>" > against a class that worked before. > > The declared policy still advertises the whole u32 range: > > [TCA_DRR_QUANTUM] = { .type = NLA_U32 }, > > The sibling implementation of the same bounded-quantum idea rejects > instead of mutating, in net/sched/sch_qfq.c: > > static const struct netlink_range_validation lmax_range = { > .min = QFQ_MIN_LMAX, > .max = QFQ_MAX_LMAX, > }; > > static const struct nla_policy qfq_policy[TCA_QFQ_MAX + 1] = { > [TCA_QFQ_WEIGHT] = NLA_POLICY_RANGE(NLA_U32, 1, > QFQ_MAX_WEIGHT), > [TCA_QFQ_LMAX] = NLA_POLICY_FULL_RANGE(NLA_U32, &lmax_range), > }; > > and the sfq patch in this same series rejects the identical upper bound > in sfq_change(): > > if (ctl->quantum > 1 << 20) { > NL_SET_ERR_MSG_MOD(extack, "quantum too large"); > return -EINVAL; > } > > Would a declared NLA_POLICY_FULL_RANGE (or an explicit -EINVAL plus > extack) be preferable, which would also subsume the zero check? > > One more on the upper bound: a quantum larger than qdisc_pkt_len() cannot > add refill rounds, so the 1 << 20 maximum does not seem related to the > loop described in the changelog. Is the narrowing of the accepted range > intentional, and if so should it be spelled out in the commit message? > The message currently reads: > > > Add clamp_t(u32, quantum, 256, 1<<20) after the zero reject and on the > > fallback path. The explicit-zero reject is preserved. > > which reads as a hardening change with no behaviour change, and does not > mention that previously accepted values are now altered. That seems > worth stating given the Fixes: tag implies a stable backport. > > > } else > > - quantum = psched_mtu(qdisc_dev(sch)); > > + quantum = clamp_t(u32, (u32)psched_mtu(qdisc_dev(sch)), 256, > > 1 << 20); > > > > if (cl != NULL) { > > if (tca[TCA_RATE]) { > > -- > Sashiko AI review · > https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com
