Re: [PATCH net] net: xps: bound the cleanup loop by the old map's num_tc

[netdev-bot+sashiko@kernel.org]

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