Re: [PATCH net-next v3 13/13] net: mana: keep the existing queues when the channel count is raised

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

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

- [Low] Claim of latent double-free/use-after-free: the new helper
  mana_destroy_txq_from()…

--

net: mana: keep the existing queues when the channel count is raised

Growing the channel count now carries the running queues over and builds
only the [old, new) tail, instead of building a full second queue set and
discarding the old one. mana_alloc_qset() loses its num_queues argument
since a count change goes through mana_split_qset() or the new
mana_grow_qset(), and the self-recovery-on-resize-failure capability bit is
advertised to the firmware.

[ ... ]

[Severity: Low]
This isn't a bug today, but should this loop clear the slot after the
kvfree(), the way the RX sibling added in this same patch does?

The comment above mana_destroy_txq_from() says the array is deliberately
left in place, and the loop guard is:

	if (!apc->tx_qp[i])
		continue;

but nothing sets apc->tx_qp[i] = NULL after kvfree(), so for the
[first, num_queues) range that guard can no longer tell "already
destroyed" from "live".

mana_destroy_rxqs_from() does clear its slot:

	mana_destroy_rxq(apc, rxq, true);
	apc->rxqs[rxq_idx] = NULL;

I traced the three call sites and none of them re-reads a stale slot at the
moment:

  mana_destroy_txq()                 -> first == 0, immediately followed by
                                        kfree(apc->tx_qp); apc->tx_qp = NULL;
  mana_create_txq() out: (first != 0) -> caller mana_grow_qset() jumps to
                                        cleanup_rss, skipping cleanup_rxq
  mana_grow_qset() cleanup_rxq       -> falls into free_arrays, which just
                                        kfree()s new_tx without walking it

So the only thing keeping the tail from being torn down twice is the label
choice in mana_grow_qset() below. Would a single store keep the two helpers
symmetric and make that independent of the caller?

 
+static void mana_destroy_txq(struct mana_port_context *apc)
+{
+	if (!apc->tx_qp)
+		return;
+
+	mana_destroy_txq_from(apc, 0);
 
 	kfree(apc->tx_qp);
 	apc->tx_qp = NULL;

[ ... ]

[Severity: Low]
This is the first standalone caller of mana_destroy_txq_from(): it tears
down [first, num_queues) and returns without freeing the container, so the
freed-but-non-NULL pointers survive back into mana_grow_qset().

[ ... ]

[ ... ]

[ ... ]

+	err = mana_create_txq(scratch, ndev, old_count);
+	if (err)
+		goto cleanup_rss; /* create_txq already undid its own work */
+
+	err = mana_add_rx_queues(scratch, ndev, old_count);
+	if (err)
+		goto cleanup_rxq;

[Severity: Low]
Related to the comment on mana_destroy_txq_from(): correctness here rests
entirely on this goto targeting cleanup_rss rather than cleanup_rxq.

If a later change routed the mana_create_txq() failure to cleanup_rxq
instead, mana_destroy_txq_from(scratch, old_count) would run a second time
over the tail slots that mana_create_txq()'s out: path already kvfree()d,
re-running napi_disable_locked() / cancel_work_sync() on freed memory.

Is that invariant worth making explicit, either with the NULL store in the
helper or a note at this label?

[ ... ]

+cleanup_rxq:
+	mana_destroy_rxqs_from(scratch, old_count);
+	mana_destroy_txq_from(scratch, old_count);
+cleanup_rss:
+	mana_cleanup_indir_table(scratch);
+free_arrays:
+	/* Only the containers: every queue they name is still live on @apc. */
+	scratch->tx_qp = NULL;
+	scratch->rxqs = NULL;
+	kfree(new_tx);
+	kfree(new_rx);
+	kfree(fresh_tx);
+	kfree(fresh_rx);

[ ... ]

[Severity: Low]
One more consumer of the same convention, now that this is no longer
static:

	if (apc->tx_qp && apc->tx_qp[i] &&
	    IS_ERR_OR_NULL(apc->tx_qp[i]->mana_tx_debugfs))

It treats any non-NULL tx_qp[i] as live and dereferences it. I confirmed it
cannot be reached with a stale array today, because every grow-failure path
frees the container without publishing it, and after a failed
mana_publish_qset() the rollback reinstalls the original apc->tx_qp.

Does relying on that reachability argument, rather than on the slot being
cleared, seem like the right long-term contract for the helper?

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