Re: [PATCH net-next v3 12/13] net: mana: keep the surviving queues when the channel count is reduced

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 2

- [Medium] Use-after-free read of the RSS indirection table in the new
  channel-reduction branch of mana_set_channels().
- [Low] Stale/contradictory in-tree contract comments in
  mana_publish_qset() for the new channel-reduction caller.
- [Low] mana_split_qset()'s header comment at
  drivers/net/ethernet/microsoft/mana/mana_en.c:4335 says "Allocates two…

--

net: mana: keep the surviving queues when the channel count is reduced

A channel-count reduction no longer rebuilds the queues it keeps. The live
set is carved into a kept prefix and a tail by the new mana_split_qset(),
the prefix is published and only the tail is retired. Increases and
per-queue property changes still rebuild a full set.

[Severity: Low]
This isn't a bug, but does the allocation inventory in this comment match
what the body does?

Four pointer arrays are allocated, not two, and mana_rss_table_alloc()
allocates two tables (indir_table and rxobj_table), both of which
mana_discard_split() and mana_set_channels() free separately.

The commit message carries the same count:

  "a reduction allocates two pointer arrays and a steering table
   instead of a full queue set"

Should both read four pointer arrays and two tables?

+int mana_split_qset(struct mana_port_context *apc,
+		    struct mana_port_context *scratch, unsigned int new_count,
+		    struct mana_qset *out_new, struct mana_qset *out_tail)
+{

[ ... ]

+	scratch->num_queues = new_count;
+	err = mana_rss_table_alloc(scratch);
+	if (err)
+		return err;
+
+	if (mana_rss_table_keep(apc, new_count, &indir_lost))
+		memcpy(scratch->indir_table, apc->indir_table,
+		       apc->indir_table_sz * sizeof(*apc->indir_table));
+	else
+		mana_rss_table_init(scratch);
+
+	new_tx = kzalloc_objs(struct mana_tx_qp *, new_count);
+	new_rx = kzalloc_objs(struct mana_rxq *, new_count);
+	tail_tx = kzalloc_objs(struct mana_tx_qp *, tail_count);
+	tail_rx = kzalloc_objs(struct mana_rxq *, tail_count);

[ ... ]

+	/* The tail. It owns no steering table; bpf_prog is carried so that
+	 * retiring it drops exactly the tail's per-queue program references
+	 * and leaves the kept ones alone.
+	 */

[Severity: Low]
Where does the number of program references dropped for the tail come
from?  mana_free_qset() installs the tail into @scratch and then calls:

	retiring_prog = mana_chn_xdp_peek(scratch);
	retiring_queues = scratch->num_queues;
	...
	mana_chn_xdp_release(retiring_prog, retiring_queues);

so the count comes from the tail's queue count, not from
out_tail->bpf_prog.

Related to that, is the comment in mana_publish_qset() still accurate for
the set this helper builds?

	/* Carry the XDP program over before steering can reach the new RXQs:
	 * they were created with bpf_prog == NULL, so a packet arriving first
	 * would bypass an attached program. This also takes the per-queue
	 * references that mana_free_qset() drops for the old set.
	 */
	mana_chn_setxdp(apc, mana_xdp_get(apc));

For a reduction, newq->rxqs[] are the live queues, so they already carry
apc->bpf_prog and mana_chn_setxdp() returns immediately:

drivers/net/ethernet/microsoft/mana/mana_bpf.c:mana_chn_setxdp() {
	struct bpf_prog *old_prog = mana_chn_xdp_get(apc);
	...
	if (old_prog == prog)
		return;

	if (prog)
		bpf_prog_add(prog, num_queues);
	...
}

The refcount does balance today: old_count references are held going in,
mana_free_qset() puts tail_count, and new_count are left for the new_count
live queues.  Read together though, the two comments describe old_count +
new_count taken against tail_count released, and the balance rests on that
undocumented early return in another file.  Could the publish comment
mention the carried-over case?

The same function's header contract ("On failure the old set is reinstalled
and the caller frees only @newq") also reads oddly for this caller, which
must free only @newq's containers via mana_discard_split() because @newq's
queues are the live ones.

[Severity: Medium]
Can this kfree() release the indirection table while the transmit path is
still reading it?

mana_get_tx_queue() dereferences apc->indir_table with no lock and no
port_is_up gate:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_get_tx_queue() {
	txq = apc->indir_table[hash & (apc->indir_table_sz - 1)];
	...
}

It is reached from ndo_select_queue, netdev_core_pick_tx() ->
mana_select_queue() -> mana_get_tx_queue(), inside __dev_queue_xmit()'s
rcu_read_lock_bh() section.

The only synchronize_net() on the publish success path runs before the
pointer is replaced, and transmits are running again by the time publish
returns:

mana_publish_qset()
	synchronize_net();		/* before the swap */
	mana_qset_set_retiring(newq, NULL, false);
	mana_qset_install(apc, newq);	/* apc->indir_table replaced here */
	...
	WRITE_ONCE(apc->port_is_up, true);
	mana_start_txqs(apc);

mana_set_channels()
	kfree(oldq.indir_table);	/* no grace period since the swap */

So a CPU that loaded the old apc->indir_table before mana_qset_install()
and has not yet done the array load can read freed memory.
mana_config_rss() only waits on an HWC completion, which says nothing about
other CPUs' read-side sections.

Before this patch the reduction freed the old table inside mana_free_qset()
-> mana_cleanup_indir_table(scratch), which happens after that function's
synchronize_net() and therefore after the new set is installed.

Would a synchronize_net() before these frees, or moving them after
mana_free_qset(), close that window?  The same pattern appears in the grow
branch added later in the series ("net: mana: keep the existing queues when
the channel count is raised"), where oldq.indir_table is freed the same way,
so both branches look affected at the end of the series.

+
+		mana_free_qset(apc, scratch, &tailq);
+		goto free_scratch;
+	}
+

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901014442.2945689-1-longli%40microsoft.com