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

[netdev-bot+sashiko@kernel.org]

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.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 6769faada11bc..ae7c664bf76ef 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -4327,7 +4327,135 @@ void mana_qset_scratch_free(struct mana_port_context *scratch)
>  	kvfree(scratch);
>  }
>  
> -/* Build a queue set in @scratch, sized and configured from the arguments. The
> +/* Carve the live set into a kept prefix [0, @new_count) in @out_new and a
> + * tail to retire in @out_tail. @apc is untouched, on failure too.
> + *
> + * Queue i is built from the ring sizes and the buffer layout and keeps EQ i
> + * at any count, so a reduction carries the survivors over and destroys only
> + * the tail. Allocates two pointer arrays and a steering table, nothing else:
> + * the queues that stay keep their page pools, posted buffers and NAPI.
> + */

[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.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index c7ddd26cada25..b5587764333f1 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -754,6 +755,39 @@ static int mana_set_coalesce(struct net_device *ndev,
>  		goto clear_flag;
>  	}
>  
> +	/* A reduction keeps its queues configured identically, so carry them
> +	 * over and retire only the tail: no DMA ring, no hardware WQ object,
> +	 * and no old+new peak.
> +	 */
> +	if (new_count < apc->num_queues) {
> +		struct mana_qset tailq;
> +
> +		err = mana_split_qset(apc, scratch, new_count, &newq, &tailq);
> +		if (err)
> +			goto free_scratch; /* current qset untouched */
> +
> +		err = mana_publish_qset(apc, &newq, &oldq);
> +		if (err) {
> +			/* The old set is live again; drop only the containers
> +			 * built above, never the queues they point at.
> +			 */
> +			mana_discard_split(&newq, &tailq);
> +			goto free_scratch;
> +		}
> +
> +		/* @oldq holds the original arrays and steering table. Every
> +		 * queue they referenced is now owned by either the published
> +		 * set or the tail, so only the containers are freed here.
> +		 */
> +		kfree(oldq.tx_qp);
> +		kfree(oldq.rxqs);
> +		kfree(oldq.indir_table);
> +		kfree(oldq.rxobj_table);

[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