Re: [PATCH] net/sched: sch_netem: prevent packet length underflow with negative overhead

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In net/sched/sch_netem.c, packet_time_ns() line 376 now does
  `s32 cell_len = (s32)q->cell_size + q->cell_overhead;`, and lines…
- [Low] In net/sched/sch_netem.c, packet_time_ns() now computes the cell-
  adjusted length as `len = (u64)cells * cell_len;` (line 382) instead…
- [Low] The commit message says the new clamping is 'matching the
  behavior in sch_cake and sch_tbf'.

--

net/sched: sch_netem: prevent packet length underflow with negative overhead

This patch changes packet_time_ns() in sch_netem. It now returns 0 when
the packet length plus a negative packet_overhead is non-positive, and
also when cell_size + cell_overhead is non-positive. The cell-adjusted
length is now computed as a u64 product instead of a u32 one.

> Clamp the effective packet length to 0 if the overhead adjustment
> would underflow, and return 0 delay if effective cell length is
> non-positive, matching the behavior in sch_cake and sch_tbf.

[Severity: Low]
Is the comparison to sch_cake and sch_tbf accurate?

sch_tbf uses psched_l2t_ns() with struct psched_ratecfg, where overhead is
a u16. Its overhead cannot be negative, so there is no negative-overhead
behavior to match there.

sch_cake's cake_calc_overhead() clamps to the MPU rather than to zero:

	len = max((s32)len + q->rate_overhead, (s32)q->rate_mpu);

Its ATM/PTM framing is fixed, so it has no configurable cell size and no
rule like "return 0 delay when cell_size + cell_overhead <= 0".

The commit message also doesn't mention that the cell product is now
computed as (u64)cells * cell_len instead of in wrapping u32 arithmetic.
That changes the computed delay for large configurations.

Since this is tagged for stable, could the message describe this as a new
policy (a non-positive effective length means no rate shaping for that
packet) instead of an existing convention?

> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index 8a1b89a289d8f..a3dcd33bec499 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -366,14 +366,20 @@ static s64 tabledist(s64 mu, s32 sigma,
>  
>  static u64 packet_time_ns(u64 len, const struct netem_sched_data *q)
>  {
> +	if ((s64)len + q->packet_overhead <= 0)
> +		return 0;
> +
>  	len += q->packet_overhead;
>  
>  	if (q->cell_size) {
>  		u32 cells = reciprocal_divide(len, q->cell_size_reciprocal);
> +		s32 cell_len = (s32)q->cell_size + q->cell_overhead;
>  
>  		if (len > cells * q->cell_size)	/* extra cell needed for remainder */
>  			cells++;
> -		len = cells * (q->cell_size + q->cell_overhead);
> +		if (cell_len <= 0)
> +			return 0;

[Severity: Medium]
Can cell_len come out negative here for configurations the kernel already
accepts?

q->cell_size comes from the __u32 cell_size field of struct tc_netem_rate.
get_rate() stores it without any range check:

net/sched/sch_netem.c:get_rate() {
    ...
	q->cell_size = r->cell_size;
	q->cell_overhead = r->cell_overhead;
    ...
}

In two cases cell_len goes negative even though the real cell length is
large and positive:

  - any cell_size >= 0x80000000 turns negative after the (s32) cast
  - cell_size = 0x7fffffff with cell_overhead = 1 wraps the s32 addition
    past INT_MAX

In both cases packet_time_ns() returns 0, so this line in netem_enqueue()
adds no serialization delay:

	delay += packet_time_ns(qdisc_pkt_len(skb), q);

The configured rate shaping is then silently skipped. Before this patch,
cell_size = 0x80000000 with cell_overhead = 0 still produced a large
positive delay, and netem_dump() reports the value back unchanged.

Would computing the sum in a wider type avoid this? For example:

	s64 cell_len = (s64)q->cell_size + q->cell_overhead;

Another option is to reject these values in get_rate()/netem_change()
with an extack error.

> +		len = (u64)cells * cell_len;
>  	}
>  
>  	return div64_u64(len * NSEC_PER_SEC, q->rate);

[Severity: Low]
len is now a u64 product, so it is no longer bounded by 2^32 - 1. Can
len * NSEC_PER_SEC overflow here?

Take cell_size = 1, cell_overhead = 0x7ffffffe (so cell_len = 0x7fffffff)
and a 1500 byte packet. reciprocal_value(1) gives cells = 1500, so len is
about 3.2e12. len * NSEC_PER_SEC is then about 3.2e21, which wraps u64.

With a small q->rate (for example 1 byte/s) the quotient can also exceed
S64_MAX. The s64 delay in netem_enqueue() would then turn negative:

	delay += packet_time_ns(qdisc_pkt_len(skb), q);
	...
	cb->time_to_send = now + delay;

and time_to_send would land in the past. The old u32 product capped this
at about 4.3e18, which is below S64_MAX.

The same configuration already gave a meaningless delay before this patch,
because the u32 product wrapped. The patch moves the wrap into the 64-bit
multiply and adds the negative-delay case.

Would mul_u64_u64_div_u64() or an explicit clamp make this well-defined?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008035656.330667-1-dungvn2345%40gmail.com