* Re: [PATCH net] net: xps: re-validate the queue index after switching to the subordinate device
2026-10-21 15:55 [PATCH net] net: xps: re-validate the queue index after switching to the subordinate device Dudu Lu
@ 2026-10-23 9:49 ` Antoine Tenart
2026-10-25 17:47 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: Antoine Tenart @ 2026-10-23 9:49 UTC (permalink / raw)
To: Dudu Lu; +Cc: davem, edumazet, kuba, pabeni, netdev
On Wed, Oct 21, 2026 at 11:55:55PM +0800, Dudu Lu wrote:
> __netif_set_xps_queue() reassigns dev to the subordinate device
> (netdev_get_tx_queue(dev, index)->sb_dev) but keeps using index, which
> was only validated against the ORIGINAL device's queue count. A
> subordinate with fewer TX queues than the parent's index then overflows
> sb_dev->_tx[] / the XPS map:
>
> BUG: KASAN: slab-out-of-bounds in __netif_set_xps_queue+0x178c/0x1ee8
> Write of size 4 at addr ffff000005d3fae0 by task A-26-F1-S2/162
> __netif_set_xps_queue+0x178c/0x1ee8
> netif_set_xps_queue+0x38/0x58
> xps_cpus_store+0x1dc/0x2b8
>
> The store is netdev_queue_numa_node_write() on sub->_tx[index]. The
> top-of-function WARN_ON_ONCE(index >= dev->num_tx_queues) only checks the
> parent and is a debug assertion. Re-check index against the device that
> is actually indexed once dev has been rebound.
>
> Note this is a different defect from the "reject an out of range traffic
> class" guard (4da3b7b8b50f), which bounds tc, not index.
>
> Fixes: ffcfe25bb50f ("net: Add support for subordinate device traffic classes")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
> ---
> Tested base
> -----------
> Detected on Linux 7.3-rc4 (commit 93f51579e7df), arm64 QEMU guest.
> Re-checked against mainline f47d223627f8 (v7.3-341): the affected code
> is unchanged between the two, so the bug is still present there.
> Checked against linux-next next-20261021 (commit 9a482184d910): the
> affected code is unchanged there as well, so the bug is still present in
> linux-next too.
>
> Reproducer status
> -----------------
> A reproducer was written and executed on an arm64 QEMU guest (KASAN); it
> fires at the claimed sink, and still fires on a kernel built with 4da3b7b8b50f
> applied (identity established by disassembling both kernels: same store, same
> target offset #0x160). The guest has no SR-IOV NIC, so the reproducer stands
> in for that hardware with a small module that binds an sb_dev the same way
> netdev_bind_sb_channel_queue() does for i40e/ixgbe. The reproducer source is
> not posted here per Documentation/process/security-bugs.rst; it is available on
> request.
>
> Triggering conditions
> ---------------------
> CAP_NET_ADMIN to set the XPS mask. An sb_dev is bound in-tree by
> netdev_bind_sb_channel_queue(), which the i40e and ixgbe SR-IOV paths call
> when a VF's channel is bound to the PF. The QEMU guest used for the
> reproducer has no SR-IOV NIC, so the sb_dev binding was set up through a
> small module; the defect itself is reached in-tree on that hardware.
>
> Notes
> -----
> Built and checkpatch-clean: this patch was applied on top of 7.3-rc4
> (commit 93f51579e7df) and a full arm64 kernel (defconfig) was built with
> it -- no new warnings or errors. scripts/checkpatch.pl reports 0 errors,
> 0 warnings.
>
> net/core/dev.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360..d5ce25144806 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2900,6 +2900,14 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
> /* If queue belongs to subordinate dev use its map */
> dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;
>
> + /* dev may now be the subordinate device, whose queue
> + * count can be smaller than the parent's. index was only
> + * validated against the parent above, so re-check it before
The check is a WARN, not really validating against oob.
> + * it is used to index the (possibly different) dev->_tx[].
> + */
> + if (index >= dev->real_num_tx_queues)
> + return -EINVAL;
I don't know that area well but my understanding is while the mapping is
stored on the sb_dev, it points to queues on the dev; this would reject
valid configurations.
> +
> tc = netdev_txq_to_tc(dev, index);
> if (tc < 0 || tc >= num_tc)
> return -EINVAL;
Shouldn't netdev_queue_numa_node_write operate on the dev txqs instead
of the sb_dev ones?
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] net: xps: re-validate the queue index after switching to the subordinate device
2026-10-21 15:55 [PATCH net] net: xps: re-validate the queue index after switching to the subordinate device Dudu Lu
2026-10-23 9:49 ` Antoine Tenart
@ 2026-10-25 17:47 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 17:47 UTC (permalink / raw)
To: phx0fer; +Cc: davem, edumazet, kuba, pabeni, netdev
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 1 · 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).
- [High] The fix applies the wrong bound and leaves the faulty access in
place.
- [Medium] The new check also runs when the queue has no sb_dev and `dev`
is still the parent.
- [Low] The commit message describes the defect inaccurately.
- [Low] The submission notes ('Triggering conditions', below the ---
line) say the in-tree subordinate binding happens on 'the i40e and…
--
net: xps: re-validate the queue index after switching to the subordinate device
This patch adds a check to __netif_set_xps_queue(). After dev has been
switched to the subordinate device, index is compared against that
device's real_num_tx_queues and -EINVAL is returned if it is too large.
The goal is to stop the KASAN slab-out-of-bounds write reported from
xps_cpus_store().
> A subordinate with fewer TX queues than the parent's index then
> overflows sb_dev->_tx[] / the XPS map:
[Severity: Low]
Is the "/ the XPS map" part accurate? In __netif_set_xps_queue() the XPS
maps are indexed by tci, not by index:
tci = j * num_tc + tc;
map = copy ? xmap_dereference(dev_maps->attr_map[tci]) : NULL;
and index is only stored as a value:
map->queues[map->len++] = index;
The only out-of-bounds subscript seems to be netdev_get_tx_queue(dev,
index) in the NUMA node write at out_no_new_maps.
Also, netdev_set_sb_channel() rejects multiqueue devices, so every in-tree
subordinate has exactly one TX queue. That means "fewer TX queues than the
parent's index" is not a corner case. It covers every XPS write to any
offloaded queue.
Could the commit message describe this more precisely?
> CAP_NET_ADMIN to set the XPS mask. An sb_dev is bound in-tree by
> netdev_bind_sb_channel_queue(), which the i40e and ixgbe SR-IOV paths call
> when a VF's channel is bound to the PF.
[Severity: Low]
Is this description of the trigger correct? The only callers of
netdev_bind_sb_channel_queue() are ixgbe_fwd_ring_up() and
i40e_fwd_ring_up(). Both bind queues to a macvlan netdevice for L2
forwarding offload, via ndo_dfwd_add_station (ixgbe_fwd_add() and
i40e_fwd_add()):
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_fwd_ring_up() {
...
/* record configuration for macvlan interface in vdev */
for (i = 0; i < num_tc; i++)
netdev_bind_sb_channel_queue(adapter->netdev, vdev,
i, rss_i, baseq + (rss_i * i));
...
}
Enabling VFs alone does not seem to set up this binding. The trigger
appears to be a macvlan with l2-fwd-offload on an ixgbe or i40e parent.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..d5ce251448062 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2900,6 +2900,14 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
> /* If queue belongs to subordinate dev use its map */
> dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;
>
> + /* dev may now be the subordinate device, whose queue
> + * count can be smaller than the parent's. index was only
> + * validated against the parent above, so re-check it before
> + * it is used to index the (possibly different) dev->_tx[].
> + */
> + if (index >= dev->real_num_tx_queues)
> + return -EINVAL;
[Severity: High]
Does this compare a parent-space index against the subordinate's queue
count?
netdev_bind_sb_channel_queue() checks offset + count against the parent's
real_num_tx_queues and stores the parent-space offset in
sb_dev->tc_to_txq[]. So index here is a parent queue number:
net/core/dev.c:netdev_bind_sb_channel_queue() {
...
/* We cannot hand out queues we don't have */
if ((offset + count) > dev->real_num_tx_queues)
return -EINVAL;
...
WRITE_ONCE(sb_dev->tc_to_txq[tc].combined, res.combined);
...
}
netdev_set_sb_channel() refuses multiqueue devices, and ixgbe_fwd_add()
rejects multiqueue macvlans. So sb_dev->real_num_tx_queues is always 1:
net/core/dev.c:netdev_set_sb_channel() {
/* Do not use a multiqueue device to represent a subordinate channel */
if (netif_is_multiqueue(dev))
return -ENODEV;
...
}
ixgbe_fwd_ring_up() binds at baseq + (rss_i * i), where baseq is
pool * queues_per_pool. Pool 0 is reserved for the PF, so pool is at least
1. i40e_fwd_ring_up() binds at iter->base_queue, which comes after the
PF's own queues. In both cases every bound index is >= 1.
With that, won't every xps_cpus and xps_rxqs write to a parent TX queue
bound to an offloaded macvlan now fail with -EINVAL? Before this patch the
map update itself worked. Only the trailing NUMA hint write was out of
bounds.
Driver-internal callers are hit too. ixgbe_configure_tx_ring() ignores the
return value of netif_set_xps_queue() after test_and_set_bit() has already
set __IXGBE_TX_XPS_INIT_DONE. XPS setup for those rings is then never
retried.
XPS_RXQS writes through xps_rxqs_store() are now rejected as well, even
though that path never indexes _tx[index]. Meanwhile xps_cpus_show() still
accepts the same parent index on the same queue, so show and store now
disagree.
The faulty access also seems to be left unchanged:
net/core/dev.c:__netif_set_xps_queue() {
...
out_no_new_maps:
if (type == XPS_CPUS)
/* update Tx queue numa node */
netdev_queue_numa_node_write(netdev_get_tx_queue(dev, index),
(numa_node_id >= 0) ?
numa_node_id : NUMA_NO_NODE);
...
}
If an index ever gets past the new check, won't the NUMA hint go to
sb_dev->_tx[index] instead of the parent's queue? The parent queue would
then keep a stale value.
Would it be better to save the original parent device (or its txq) before
the rebind and use it for the NUMA write, rather than bounding index by the
subordinate's queue count?
[Severity: Medium]
This check is inside if (dev_num_tc) and runs whether or not a rebind
happened. When the queue has no sb_dev, dev is still the parent.
On any device with num_tc != 0, the accepted range therefore shrinks from
num_tx_queues to real_num_tx_queues. num_tx_queues is the existing
contract, per the WARN_ON_ONCE() at the top of the function. Devices with
num_tc == 0 get no such check.
Can this break drivers that configure XPS before raising
real_num_tx_queues? ixgbe_open() does things in this order:
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_open() {
...
ixgbe_configure(adapter);
...
/* Notify the stack of the actual queue counts. */
queues = adapter->num_tx_queues;
err = netif_set_real_num_tx_queues(netdev, queues);
...
}
ixgbe_configure() reaches netif_set_xps_queue() through
ixgbe_configure_tx_ring(). When DCB is enabled through ixgbe_setup_tc(),
the flow is netdev_set_num_tc() -> new rings -> ixgbe_open(). If the new
ring count is larger than the previous real_num_tx_queues, XPS for the
extra queues would now fail with -EINVAL.
ixgbe_configure_tx_ring() ignores that error, and __IXGBE_TX_XPS_INIT_DONE
is already set. XPS would then be silently and permanently missing for
those queues. i40e_vsi_open() has the same ordering:
i40e_vsi_configure() runs before i40e_netif_set_realnum_tx_rx_queues().
> tc = netdev_txq_to_tc(dev, index);
> if (tc < 0 || tc >= num_tc)
> return -EINVAL;
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021155555.68167-1-phx0fer%40gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread