Re: [PATCH net v3 5/9] net/sched: dualpi2: clamp psched_mtu at all call sites

On Fri, Sep 4, 2026 at 5:42 AM [off-list ref] wrote:

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Changelog/reproducer mismatch in "net/sched: dualpi2: clamp
  psched_mtu at all call sites": the commit message blames "a crafted…
- [Low] netdev coding-style violation and triplicated open-coded clamp.

My conclusion:

1. [Medium] "changelog/reproducer mismatch — stab does not reach
   psched_mtu()"
Valid point.
The real trigger is a large device MTU  (dummy accepts up to ~2^31-1
since dev->max_mtu = 0), not a size table; the changelog's stab-based
recipe exercises none of the three
 The correct reproducer is: "ip link set dev dummy0 mtu 2147483647"
Changelog issue only; the clamp is the right fix for the MTU path.

2. [Low] "declaration ordering + triplicated clamp"
Also valid:  Will fix the ordering (and consider a small helper) if
the series is ever resent.

On the deferred scope note (dequeue_packet() credit multiply,
deferred_drops_len accumulation): We verified separately the generic 1
MiB pkt_len cap bounds the multiply, but the s32 credit ACCUMULATION
can still wrap when the C queue is empty; filed as a follow-up
("under dualpi2 c_protection_credit s32 accumulation wrap
(C-queue-empty case)") .

cheers,
jamal

--

net/sched: dualpi2: clamp psched_mtu at all call sites

The patch clamps psched_mtu(qdisc_dev(sch)) into [1, 1 << 20] at the three
places sch_dualpi2.c uses it: dualpi2_calculate_c_protection(), must_drop()
and get_memory_limit().  The stated goal is to avoid overflow of the signed
multiply for c_protection_init and the wrap of 2 * psched_mtu().

A couple of questions about the changelog and the style of the new code.

[Severity: Medium]

quoted

With a crafted size table qdisc_pkt_len reaches ~2 GiB, causing a soft
lockup / denial of service.

  tc qdisc add dev dummy0 root dualpi2
  tc qdisc change dev dummy0 root dualpi2 stab data 32768 size_log 15 cell_log 0

Do these two commands actually exercise any of the three changed lines?

The patch only bounds psched_mtu(), which is derived from the netdevice:

include/net/pkt_sched.h:
        static inline unsigned int psched_mtu(const struct net_device *dev)
        {
                return READ_ONCE(dev->mtu) + dev->hard_header_len;
        }

A TCA_STAB only rewrites qdisc_skb_cb(skb)->pkt_len in
__qdisc_calculate_pkt_len(), so a size table cannot influence
psched_mtu().  With the quoted commands dummy0 keeps its default MTU, so
dualpi2_calculate_c_protection(), must_drop() and get_memory_limit()
compute the same values before and after this patch.

Is the "~2 GiB" qdisc_pkt_len reachable on this tree at all?
__qdisc_calculate_pkt_len() already caps it, and that clamp is present in
the baseline too:

net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
        ...
        pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
        qdisc_skb_cb(skb)->pkt_len = pkt_len;
}

with include/net/pkt_sched.h:

        #define QDISC_PKT_LEN_MAX       (1 << 20)       /* 1 MiB */

Would it be more accurate to describe the trigger as a large device MTU?
dummy leaves the upper bound unset:

drivers/net/dummy.c:dummy_setup() {
        ...
        dev->min_mtu = 0;
        dev->max_mtu = 0;
}

and dev_validate_mtu() only enforces an upper limit when dev->max_mtu > 0,
so "ip link set dev dummy0 mtu 2147483647" is what makes
(s32)psched_mtu() * ((int)wc - (int)wl) overflow.  Note that
TCA_DUALPI2_C_PROTECTION is range-limited to [0, MAX_WC] with MAX_WC 100,
so the signed multiply only overflows above roughly 21.5 MiB of MTU, and
2 * psched_mtu() only wraps above 2 GiB.

One more thing on scope: the size-table path the message mentions is left
untouched in this file.  dequeue_packet() still does

        *credit_change *= qdisc_pkt_len(skb);

and drop_and_retry() still accumulates

        q->deferred_drops_len += qdisc_pkt_len(skb);

Those are fine here because of the generic 1 MiB pkt_len cap, but the
Fixes: tag points at trees that may not carry that cap.  Should the
changelog say so?

quoted

diff --git a/net/sched/sch_dualpi2.c b/net/sched/sch_dualpi2.c
index 4f678d4ff10ec..4947def7c49ef 100644
--- a/net/sched/sch_dualpi2.c
+++ b/net/sched/sch_dualpi2.c

[ ... ]

quoted

@@ -285,8 +287,9 @@ static bool must_drop(struct Qdisc *sch, struct dualpi2_sched_data *q,
      u64 local_l_prob;
      bool overload;
      u32 prob;
+     u32 mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);

[Severity: Low]
This isn't a bug, but the new declaration in must_drop() is the longest
line of the block and is placed last, which breaks the netdev declaration
ordering rule in Documentation/process/maintainer-netdev.rst ("Order the
variable declaration lines longest to shortest").  Moving the
initialization out of line, or reordering the block, would match the rule.

quoted

-     if (sch->qstats.backlog < 2 * psched_mtu(qdisc_dev(sch)))
+     if (sch->qstats.backlog < 2 * mtu)
              return false;

      prob = READ_ONCE(q->pi2_prob);

[ ... ]

quoted

@@ -712,7 +715,8 @@ static u32 get_memory_limit(struct Qdisc *sch, u32 limit)
      /* Apply rule of thumb, i.e., doubling the packet length,
       * to further include per packet overhead in memory_limit.
       */
-     u64 memlim = mul_u32_u32(limit, 2 * psched_mtu(qdisc_dev(sch)));
+     u64 memlim = mul_u32_u32(limit, 2 * clamp_t(u32, psched_mtu(qdisc_dev(sch)),
+                                                  1, 1 << 20));

Would a small helper, e.g. dualpi2_psched_mtu(sch), be preferable here?
The same clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20) expression is
now open-coded three times, including this line-wrapped copy inside
mul_u32_u32(), so the "at all call sites" property has to be re-checked by
hand for any future psched_mtu() user or bound change.

quoted

      if (upper_32_bits(memlim))
              return U32_MAX;

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