Re: [PATCH net v2 1/3] net: cap tx_queue_len at S16_MAX to prevent oversized ring allocations

[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

- [Medium] Unconditional uAPI narrowing of tx_queue_len for every
  netdevice, justified in the commit message by a claim that is…
- [Low] RTM_NEWLINK device-creation bypass of the new S16_MAX cap.
- [Low] The new 32767 upper bound is not recorded in the sysfs ABI
  documentation.

--

net: cap tx_queue_len at S16_MAX to prevent oversized ring allocations

This replaces the unsigned-long truncation test in
netif_change_tx_queue_len() with a hard upper bound of S16_MAX, so that
values above 32767 are rejected with -ERANGE at set time instead of later
driving skb_array/ptr_ring allocations in pfifo_fast, tun and tap.

Closes: https://lore.kernel.org/netdev/20260828121902.66837-1-jhs@mojatatu.com/

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 290e0f099e6b..ecfbd72d5d1a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9982,7 +9982,7 @@ int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
>  	unsigned int orig_len = dev->tx_queue_len;
>  	int res;
>  
> -	if (new_len != (unsigned int)new_len)
> +	if (new_len > S16_MAX)
>  		return -ERANGE;

[Severity: Medium]
The commit message justifies the constant with:

  "32767 is chosen as the largest value NLA_POLICY_FULL_RANGE can express
   for the u32 IFLA_TXQLEN policy in patch 2/3"

Is that accurate?  NLA_POLICY_FULL_RANGE() takes a pointer to:

include/net/netlink.h:
	struct netlink_range_validation {
		u64 min, max;
	};

so a u32 bound such as U16_MAX or U32_MAX is expressible there.  The s16
limitation applies to NLA_POLICY_RANGE()/MIN()/MAX(), which embed the
bounds in the policy entry, not to the FULL_RANGE variant.

Since the new check in netif_change_tx_queue_len() applies to all callers
(sysfs tx_queue_len_store(), SIOCSIFTXQLEN, RTM_SETLINK via do_setlink()),
previously accepted configurations such as

	ip link set dev X txqueuelen 65535
	echo 100000 > /sys/class/net/X/tx_queue_len

now return -ERANGE, including for consumers that allocate nothing at set
time and only use the value as a packet-count limit:

net/sched/sch_fifo.c:fifo_init() {
	...
	limit = qdisc_dev(sch)->tx_queue_len;
	...
}

along with gred/plug/sfb limits, htb direct_qlen, qfq max_classes and
teql.  Could the reasoning for narrowing the shared knob be restated
without the FULL_RANGE constraint, or could the bound instead be applied
in the allocating consumers (pfifo_fast_init(), tun/tap ring sizing)?

The same paragraph also says:

  "a negative ifr_qlen from the ioctl lands far above the cap after
   conversion, so both old failure modes are covered by the one
   comparison."

The ioctl path already rejects negatives before the helper is reached:

net/core/dev_ioctl.c:dev_ifsioc() {
	...
	case SIOCSIFTXQLEN:
		if (ifr->ifr_qlen < 0)
			return -EINVAL;
		return dev_change_tx_queue_len(dev, ifr->ifr_qlen);
	...
}

so is the claim about that failure mode being covered here still needed?

[Severity: Low]
The commit message says netif_change_tx_queue_len() "is the single entry
point for IFLA_TXQLEN, sysfs, and the SIOCSIFTXQLEN ioctl", and that on the
fixed kernel RTM_NEWLINK create is also rejected.  With only this patch
applied, does the device creation path still bypass the cap?

net/core/rtnetlink.c:rtnl_create_link() {
	...
	if (tb[IFLA_TXQLEN])
		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
	...
}

and at this commit the policy imposes no range:

net/core/rtnetlink.c:
	[IFLA_TXQLEN]		= { .type = NLA_U32 },

so ip link add dev X txqueuelen 500000 type dummy stores the oversized
value directly, which pfifo_fast_init() and tap_init()/tun queue attach
then use for ring sizing.

This is closed later in the same series: the follow-up commit "net: reject
oversized tx_queue_len at netlink parse time" changes the policy entry to
NLA_POLICY_FULL_RANGE(NLA_U32, &txqlen_range) with .max = S16_MAX, so all
netlink parse paths reject the attribute before rtnl_create_link() runs.
Mentioning that dependency here would make a partial backport of patch 1
alone less likely to leave the create path uncapped.

[Severity: Low]
Should the sysfs ABI documentation be updated for the new maximum?

Documentation/ABI/testing/sysfs-class-net still describes the attribute
without any upper bound:

	What:		/sys/class/net/<iface>/tx_queue_len
	...
			Indicates the interface transmit queue len in number
			of packets, as an integer value.

while writes above 32767 through tx_queue_len_store() ->
dev_change_tx_queue_len() -> netif_change_tx_queue_len() now fail with
-ERANGE.  The kernel-doc for the exported wrapper dev_change_tx_queue_len()
in net/core/dev_api.c likewise only states "Return: 0 on success, -errno on
failure" and does not mention the 32767 limit.

The netlink half of this is handled inside the series: the follow-up commit
"net: reject oversized tx_queue_len at netlink parse time" adds
checks: max: 32767 to Documentation/netlink/specs/rt-link.yaml, so only the
sysfs ABI text and the wrapper kernel-doc remain unamended.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-2899.v2.20260901233641%40mojatatu.com