Re: [PATCH net v2 2/3] net: reject oversized tx_queue_len at netlink parse time

[netdev-bot+sashiko@kernel.org]

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 changelog of "net: reject oversized tx_queue_len at
  netlink parse time" misstates which paths and which sinks were…

--

net: reject oversized tx_queue_len at netlink parse time

This moves the tx_queue_len bound into ifla_policy so IFLA_TXQLEN is
validated against a netlink_range_validation of 0..S16_MAX at parse time,
covering the rtnl_create_link() path that assigns dev->tx_queue_len
directly. The rt-link.yaml spec gets a matching max check.

No Link: tags in the commit header.

One question about the changelog, quoting the parts in question:

> rtnl_create_link() assigns IFLA_TXQLEN directly to dev->tx_queue_len
> without going through netif_change_tx_queue_len(), so a device created
> with "ip link add ... txqueuelen 500000" bypasses the S16_MAX cap and
> still triggers the oversized ring allocations in pfifo_fast, tun and
> tap. The veth peer nest (rtnl_nla_parse_ifinfomsg()) and the
> RTM_NEWLINK-on-existing-device path reach the same sinks.

[Severity: Medium]

Is the tun sink actually reachable through the rtnl_create_link() bypass
described here?  drivers/net/tun.c refuses every rtnetlink creation
attempt:

drivers/net/tun.c:tun_validate() {
	NL_SET_ERR_MSG(extack,
		       "tun/tap creation via rtnetlink is not supported.");
	return -EOPNOTSUPP;
}

so rtnl_create_link() never runs for a tun device, and the remaining
writers of an existing tun's tx_queue_len (do_setlink(), sysfs,
SIOCSIFTXQLEN) all funnel through netif_change_tx_queue_len(), which
already returns before touching state:

net/core/dev.c:netif_change_tx_queue_len() {
	if (new_len > S16_MAX)
		return -ERANGE;

	if (new_len != orig_len) {
		WRITE_ONCE(dev->tx_queue_len, new_len);
	...
}

The tap half of the claim does hold, since macvtap/ipvtap are created via
rtnetlink and rtnl_create_link() can overwrite the TUN_READQ_SIZE default
that tap_open() later uses to size the ptr_ring.  Should the tun mention
be dropped?

Similarly, does the RTM_NEWLINK-on-existing-device path reach the same
sinks?  __rtnl_newlink() dispatches an existing device away from
rtnl_create_link():

net/core/rtnetlink.c:__rtnl_newlink() {
	if (dev)
		return rtnl_changelink(skb, nlh, ops, dev, tgt_net, tbs, data, extack);
	...
}

and rtnl_changelink() -> do_setlink() handles the attribute as:

net/core/rtnetlink.c:do_setlink() {
	if (tb[IFLA_TXQLEN]) {
		unsigned int value = nla_get_u32(tb[IFLA_TXQLEN]);

		err = netif_change_tx_queue_len(dev, value);
	...
}

which is the already-capped helper above.  That leaves the raw assignment
in rtnl_create_link():

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

as the only genuine bypass, reached from rtnl_newlink_create() and the
veth/vxcan/netkit peer nests.  Could the changelog be narrowed to that?

> at parse time and rejects the attribute with -ERANGE plus a proper
> "integer out of range" extack message before any device state is
> modified (the RTM_SETLINK half-application wart is gone with it).

Is the parenthetical too broad?  do_setlink() still applies attributes
one at a time and can leave earlier attributes committed when a later one
fails, so this only removes the half-application for an out-of-range
IFLA_TXQLEN rather than making RTM_SETLINK atomic.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 81c5a6104dea..be9d1625bac3 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2287,6 +2287,11 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
>  	return -EMSGSIZE;
>  }
>  
> +static const struct netlink_range_validation txqlen_range = {
> +	.min = 0,
> +	.max = S16_MAX,
> +};
> +

[ ... ]

> -	[IFLA_TXQLEN]		= { .type = NLA_U32 },
> +	[IFLA_TXQLEN]		= NLA_POLICY_FULL_RANGE(NLA_U32, &txqlen_range),

[ ... ]

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