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
