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

[netdev-bot+sashiko@kernel.org]

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.

--

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]

> 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?

> 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

[ ... ]

> @@ -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.

>  
> -	if (sch->qstats.backlog < 2 * psched_mtu(qdisc_dev(sch)))
> +	if (sch->qstats.backlog < 2 * mtu)
>  		return false;
>  
>  	prob = READ_ONCE(q->pi2_prob);

[ ... ]

> @@ -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.

>  
>  	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