Re: [PATCH net v3 6/9] net/sched: pie: clamp psched_mtu in pie_drop_early

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.

--

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