* Re: [PATCH net] net: xps: bound the cleanup loop by the old map's num_tc
2026-10-21 15:55 [PATCH net] net: xps: bound the cleanup loop by the old map's num_tc Dudu Lu
@ 2026-10-23 10:07 ` Antoine Tenart
2026-10-25 17:32 ` netdev-bot+sashiko
2026-10-27 11:20 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Antoine Tenart @ 2026-10-23 10:07 UTC (permalink / raw)
To: Dudu Lu; +Cc: davem, edumazet, kuba, pabeni, netdev
On Wed, Oct 21, 2026 at 11:55:38PM +0800, Dudu Lu wrote:
> __netif_set_xps_queue() walks the OLD dev_maps to free its entries. The
> per-row trip count uses the NEW num_tc while the row stride uses the OLD
> dev_maps->num_tc, so when dev->num_tc was raised without rebuilding the
> XPS map the last row indexes attr_map[] past the allocation:
>
> BUG: KASAN: slab-out-of-bounds in __netif_set_xps_queue+0x1458/0x1ee8
> Read of size 8 at addr ffff000014f69458 by task insmod/112
>
> The loop frees entries in dev_maps, so both the trip count and the stride
> must come from dev_maps->num_tc. The map is sized
> XPS_CPU_DEV_MAPS_SIZE(old num_tc)/XPS_RXQ_DEV_MAPS_SIZE(old num_tc, ...),
> so iterating the new (larger) num_tc reads past the end.
>
> Fixes: 255c04a87f43 ("net: embed num_tc in the xps maps")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
Reviewed-by: Antoine Tenart <atenart@kernel.org>
> ---
> 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. The guest has no ndo_setup_tc NIC, so the reproducer stands in for
> that hardware with a small module that raises dev->num_tc the same way an
> in-tree driver does. 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. dev->num_tc is raised in-tree by the
> drivers that implement ndo_setup_tc (the Intel i40e/ice/ixgbe/iavf, mlx5
> and mvneta families, among others) and by the mqprio/taprio qdiscs. The
> QEMU guest used for the reproducer has no such NIC, so the num_tc change
> was driven 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360..3023f0ca4713 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3003,7 +3003,7 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
> goto out_no_old_maps;
>
> for (j = 0; j < dev_maps->nr_ids; j++) {
> - for (i = num_tc, tci = j * dev_maps->num_tc; i--; tci++) {
> + for (i = dev_maps->num_tc, tci = j * dev_maps->num_tc; i--; tci++) {
> map = xmap_dereference(dev_maps->attr_map[tci]);
> if (!map)
> continue;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net: xps: bound the cleanup loop by the old map's num_tc
2026-10-21 15:55 [PATCH net] net: xps: bound the cleanup loop by the old map's num_tc Dudu Lu
2026-10-23 10:07 ` Antoine Tenart
@ 2026-10-25 17:32 ` netdev-bot+sashiko
2026-10-27 11:20 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 17:32 UTC (permalink / raw)
To: phx0fer; +Cc: davem, edumazet, kuba, pabeni, netdev
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
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] The commit message names the wrong trigger.
- [Low] The commit message understates the impact and does not mention a
second behaviour change in the same line.
--
net: xps: bound the cleanup loop by the old map's num_tc
This commit changes the old-map cleanup loop in __netif_set_xps_queue().
The per-row trip count now comes from dev_maps->num_tc instead of the new
num_tc. Before the change, the trip count and the row stride could disagree
when dev->num_tc changed while an old XPS map was still installed.
> Triggering conditions
> ---------------------
> CAP_NET_ADMIN to set the XPS mask. dev->num_tc is raised in-tree by the
> drivers that implement ndo_setup_tc (the Intel i40e/ice/ixgbe/iavf, mlx5
> and mvneta families, among others) and by the mqprio/taprio qdiscs. The
> QEMU guest used for the reproducer has no such NIC, so the num_tc change
> was driven through a small module; the defect itself is reached in-tree
> on that hardware.
[Severity: Medium]
Can the drivers and qdiscs named here reach this loop with a stale map?
As far as I can tell, they all change num_tc through netdev_set_num_tc()
or netdev_reset_tc(). Both of those reset XPS on the whole device first:
net/core/dev.c:netdev_set_num_tc() {
...
#ifdef CONFIG_XPS
netif_reset_xps_queues_gt(dev, 0);
#endif
netdev_unbind_all_sb_channels(dev);
WRITE_ONCE(dev->num_tc, num_tc);
...
}
Removing every tx queue leaves the map inactive. clean_xps_maps() then
calls reset_xps_maps(), which sets dev->xps_maps[type] to NULL. The next
__netif_set_xps_queue() sees dev_maps == NULL and takes:
if (!dev_maps)
goto out_no_old_maps;
So it never enters the loop this patch changes.
The in-tree paths that do seem to raise num_tc with an old map still
installed are the sfc Falcon and Siena drivers. ef4_setup_tc() and
efx_siena_setup_tc() write num_tc directly:
drivers/net/ethernet/sfc/siena/tx.c:efx_siena_setup_tc() {
...
WRITE_ONCE(net_dev->num_tc, num_tc);
return netif_set_real_num_tx_queues(net_dev,
max_t(int, num_tc, 1) *
efx->n_tx_channels);
}
netif_set_real_num_tx_queues() only resets XPS when the queue count
shrinks:
if (disabling) {
synchronize_net();
qdisc_reset_all_tx_gt(dev, txq);
#ifdef CONFIG_XPS
netif_reset_xps_queues_gt(dev, txq);
#endif
}
For example, going from num_tc 0 to 2 leaves a live map with
dev_maps->num_tc == 1. The next XPS write then uses a trip count of 2 with
a stride of 1, and the last row overruns attr_map[].
netif_setup_tc() also writes num_tc = 0 without a full reset, but that is
the shrink direction.
Subordinate-device maps are reset using sb_dev->num_tx_queues as the
count. A macvlan offload (sb_dev) path, for example on ixgbe, might also
leave a stale map behind. That would work differently from what is
described here for the main device.
Could the trigger description be updated to name the sfc Falcon/Siena
paths? This also applies to the claim in the reproducer notes that the
module raises num_tc "the same way an in-tree driver does". Otherwise
stable and security triage will be pointed at hardware where the
described trigger does not seem to fire.
> The loop frees entries in dev_maps, so both the trip count and the stride
> must come from dev_maps->num_tc. The map is sized
> XPS_CPU_DEV_MAPS_SIZE(old num_tc)/XPS_RXQ_DEV_MAPS_SIZE(old num_tc, ...),
> so iterating the new (larger) num_tc reads past the end.
[Severity: Low]
Is "reads past the end" the full impact here? In the old loop body, each
slot visited is dereferenced, cleared and freed:
RCU_INIT_POINTER(dev_maps->attr_map[tci], NULL);
kfree_rcu(map, rcu);
copy is always false when num_tc != dev_maps->num_tc, so the
map == new_map skip never applies. Suppose an out-of-bounds slot holds a
non-NULL value. The old code then writes NULL past the end of
attr_map[]. kfree_rcu() also writes an rcu_head into whatever that
pointer points to, and frees it later.
When the neighbouring slab data is non-zero, that is an out-of-bounds
write plus a free of an arbitrary pointer, not only a read.
The patch also changes behaviour in the other direction, where the new
num_tc is smaller than dev_maps->num_tc. The old loop only visited the
first num_tc slots of each old row, so the xps_map objects in the
remaining slots were never freed.
The later "removes tx-queue from unused" loop walks the new map with
old_dev_maps passed as NULL. kfree_rcu(old_dev_maps, rcu) only frees the
container, so those xps_map objects leaked.
One way to reach this is mqprio in software mode with TC offsets that are
not in ascending order. mqprio_validate_queue_counts() only rejects
overlap, so TC0 can sit at a higher offset than TC1.
Shrinking the channel count then makes netif_setup_tc() set num_tc = 0.
Meanwhile netif_reset_xps_queues_gt(dev, txq) leaves the lower-indexed
TC1 queues mapped.
Would it make sense for the commit message to say that this also fixes a
leak of xps_map objects, and that the overrun includes a write and a free?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021155538.67867-1-phx0fer%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread