On Fri, Sep 4, 2026 at 5:42 AM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> - [Medium] The commit message's "Conditions to recreate the bug" section
> does not reproduce the described bug.
>
Valid
The clamp fix correctly hardens the mtu=0x80000000 case; the
changelog's reproducer and premise need the `ip link set mtu ...` step
instead.
Documentation-only fix.
Note: same wrong-reproducer issue as patch 5 (dualpi2); both
changelogs blame a stab path that psched_mtu() never reads.
cheers,
jamal
>
> net/sched: pie: clamp psched_mtu in pie_drop_early
>
> The patch clamps the value returned by psched_mtu() in pie_drop_early() to
> the range [1, 1<<20], with the stated aim of preventing a very large device
> MTU from zeroing the bytemode drop probability. The change itself is a
> one-line clamp_t() in net/sched/sch_pie.c.
>
> > pie_drop_early() calls psched_mtu() with no clamp. With mtu=0x80000000
> > the bytemode divide silently zeroes the drop probability, disabling AQM.
> > Clamp to [1, 1<<20].
> >
> > Conditions to recreate the bug:
> > CONFIG_NET_SCH_PIE=y. Requires CAP_NET_ADMIN (namespace-local via
> > unshare -Urn suffices).
> >
> > tc qdisc add dev dummy0 root pie
> > tc qdisc change dev dummy0 root pie stab data 32768 size_log 15
> > cell_log 0
>
> [Severity: Medium]
> Do these two commands actually reproduce the described condition?
>
> The clamped quantity comes only from the netdevice:
>
> include/net/pkt_sched.h:psched_mtu() {
> return READ_ONCE(dev->mtu) + dev->hard_header_len;
> }
>
> Neither "tc qdisc add ... root pie" nor "tc qdisc change ... stab data 32768
> size_log 15 cell_log 0" changes dev->mtu or dev->hard_header_len, so dummy0
> keeps its ether_setup() defaults of 1500 + 14 = 1514. That is already inside
> [1, 1<<20], so clamp_t() returns the same value and the reproducer sees no
> difference before or after the patch. Shouldn't the steps include something
> like "ip link set dev dummy0 mtu 2147483634" (which dummy allows, since
> dummy_setup() leaves dev->min_mtu = 0 and dev->max_mtu = 0)?
>
> Also, does the size table have any bearing here? A TCA_STAB table only
> affects qdisc_skb_cb(skb)->pkt_len via __qdisc_calculate_pkt_len(), which is
> already capped:
>
> net/sched/sch_api.c:__qdisc_calculate_pkt_len() {
> ...
> pkt_len = clamp_t(int, pkt_len, 1, QDISC_PKT_LEN_MAX);
> ...
> }
>
> and pie_qdisc_enqueue() does not use it as packet_size:
>
> net/sched/sch_pie.c:pie_qdisc_enqueue() {
> ...
> if (!pie_drop_early(sch, &q->params, &q->vars, sch->qstats.backlog,
> skb->len)) {
> ...
> }
>
> Finally, the divide the message blames is gated on bytemode:
>
> net/sched/sch_pie.c:pie_drop_early() {
> ...
> if (params->bytemode && packet_size <= mtu)
> local_prob = (u64)packet_size * div_u64(local_prob, mtu);
> ...
> }
>
> and bytemode is off by default:
>
> include/net/pie.h:pie_params_init() {
> ...
> params->bytemode = false;
> ...
> }
>
> Since the listed commands never pass bytemode, is that branch ever reached in
> the reproducer? Could the "Conditions to recreate the bug" section be updated
> to raise the MTU and enable bytemode, so that the Tested-by tag corresponds to
> a sequence that exercises the clamped value and the divide?
>
> > {
> > u64 rnd;
> > u64 local_prob = vars->prob;
> > - u32 mtu = psched_mtu(qdisc_dev(sch));
> > + u32 mtu = clamp_t(u32, psched_mtu(qdisc_dev(sch)), 1, 1 << 20);
>
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-0CFC.v3.20260901204856%40mojatatu.com
