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

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

[netdev-bot+sashiko]

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

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

[Jamal Hadi Salim]

On Fri, Sep 4, 2026 at 5:42 AM <netdev-bot+sashiko@kernel.org> 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]
>
> > 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