> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Saturday, September 5, 2026 5:45 PM
> To: Long Li <[email protected]>
> Cc: [email protected]
> Subject: [EXTERNAL] Re: [PATCH net] net: mana: do not discard an ethtool-
> configured RSS table on a queue rebuild
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> New issues:
> - [High] State desynchronization causes newly added queues to be left
> unutilized after a sequence of channel count changes.
>
> Pre-existing issues:
> - [Critical] Pre-existing Use-After-Free in stats collection due to ad-hoc
> synchronization flag.
> --
>
> commit dc8331f7b2434de9035275921f9768699946cdcb
> Author: Long Li <[email protected]>
>
> net: mana: do not discard an ethtool-configured RSS table on a queue
> rebuild
>
> mana_alloc_queues() regenerates the RSS indirection table from the driver
> default every time the queues are built, so a table installed with "ethtool
> -X"
> is silently replaced by any operation that rebuilds them.
>
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 7a1ac853e3abc..97386e17b9421 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -3306,6 +3306,28 @@ static void mana_rss_table_init(struct
> mana_port_context *apc)
> > ethtool_rxfh_indir_default(i, apc->num_queues); }
> >
> > +/* Whether the current indirection table can be kept for
> > +apc->num_queues,
> > + * rather than rebuilt from the driver default.
> > + *
> > + * Only a user table ("ethtool -X") is worth keeping; a
> > +driver-generated one
> > + * is rebuilt so that it spreads over every queue. An entry pointing
> > +past the
> > + * last queue cannot be kept: mana_config_rss() uses these entries to
> > +index
> > + * apc->rxqs[], which holds apc->num_queues pointers.
> > + */
> > +static bool mana_rss_table_keep(struct mana_port_context *apc) {
> > + u32 i;
> > +
> > + if (!netif_is_rxfh_configured(apc->ndev))
> > + return false;
> > +
> > + for (i = 0; i < apc->indir_table_sz; i++)
> > + if (apc->indir_table[i] >= apc->num_queues)
> > + return false;
> > +
> > + return true;
> > +}
>
> [Severity: High]
> Does this check wrongly trust netif_is_rxfh_configured() without validating if
> the table was previously replaced by a driver fallback?
>
> If a user sets a custom RSS table via ethtool -X and then reduces the channel
> count, mana_alloc_queues() detects the user table indices are out of bounds
> and generates a narrow driver-default table, but intentionally avoids
> notifying
> the core via ethtool_rxfh_indir_lost(). The core's
> netif_is_rxfh_configured() remains true.
>
> When the user subsequently increases the channel count, could this function
> incorrectly believe the narrow driver-generated fallback table is still a
> user-
> configured table? Since its bounds fit the new queue count, it seems this
> might permanently freeze the table at the smaller size.
The two steps are mutually exclusive, so the sequence cannot occur.
The premise is that a channel reduction can strand a user table. It cannot:
both ethtool entry points call ethtool_check_max_channel() (channels.c and
ioctl.c), which refuses the request. On this hardware:
# ethtool -X ens1 equal 16
# ethtool -L ens1 combined 4
netlink error: requested channel counts are too low for existing
indirection table (15)
Note ethtool_get_max_rxfh_channel() derives that bound by calling
ops->get_rxfh(), i.e. the table the driver currently reports, so the guard
follows whatever mana last installed rather than a remembered user table.
mana_set_rxfh() also rejects any entry >= apc->num_queues, so a user table
is never out of range when it is installed.
That leaves exactly one way for mana_rss_table_keep() to return false: the
clamp in mana_init_port(), where the device reports fewer queues after a
reset or resume. But that clamp is
if (apc->num_queues > apc->max_queues)
apc->num_queues = apc->max_queues;
so num_queues ends up equal to max_queues, and max_queues only ever shrinks -
both assignments in mana_init_port() are guarded by "if (apc->max_queues >
...)", and the only initialisation is in mana_probe_port(), which does not run
on resume. A subsequent increase would need new_count > max_combined ==
apc->max_queues, which the core rejects with "requested channel count exceeds
maximum".
So the fallback and the later increase cannot both happen to one port.
On the wider point of not calling ethtool_rxfh_indir_lost(): 16 drivers test
netif_is_rxfh_configured(), and two call ethtool_rxfh_indir_lost() - bnxt
(__bnxt_reserve_rings(), when firmware grants fewer rings than asked) and
mlx5e (mlx5e_attach_netdev(), when max_nch drops). The other fourteen, among
them ice, idpf, hns3, fbnic, gve and nfp, keep a user table when one is set,
regenerate the default otherwise, and do not notify the core. This patch
follows that majority.
mana cannot follow bnxt or mlx5e here even if it wanted to. All three drivers
require the netdev instance lock for the notification, since it sends
ETHTOOL_MSG_RSS_NTF; mlx5e says so at the rtnl_lock()/netdev_lock() pair in
mlx5e_attach_netdev(). The difference is the call sites, not the capability.
bnxt reaches its call only with dev->lock held, and mlx5e keys off
reg_state == NETREG_REGISTERED. mana_alloc_queues() is reached from ndo_open
with dev->lock held and from mana_attach() on the reset and resume paths
without it, and the netdev is registered in both cases, so neither taking the
lock nor testing reg_state works. Making mana notify would mean holding
netdev_lock across mana_attach() in the TX-timeout handler and the resume
loop, which is not something I would put in a fix for net.
>
> > +
> > int mana_disable_vport_rx(struct mana_port_context *apc) {
> > return mana_cfg_vport_steering(apc, TRI_STATE_FALSE, false, false,
> > @@ -3621,7 +3643,20 @@ int mana_alloc_queues(struct net_device
> *ndev)
> > goto destroy_rxq;
> > }
> >
> > - mana_rss_table_init(apc);
> > + /* Keep a user-configured table across the rebuild: its entries are
> > + * queue indices and stay meaningful while they are all still in range.
> > + * Only a driver-generated table is regenerated here.
> > + *
> > + * A table that cannot be kept is replaced by the default without
> > + * telling the core, which keeps reporting the table as user
> > + * configured. That is what this function did for every table before,
> > + * and reporting it here is not an option: ethtool_rxfh_indir_lost()
> > + * sends ETHTOOL_MSG_RSS_NTF, which requires the netdev instance
> lock,
> > + * and this runs both with that lock held (ndo_open) and without it
> > + * (mana_attach() from the reset and resume paths).
> > + */
> > + if (!mana_rss_table_keep(apc))
> > + mana_rss_table_init(apc);
>
> [Severity: High]
> If mana_rss_table_keep() incorrectly returns true due to the
> desynchronization described above, does it skip the table regeneration and
> leave newly added queues unutilized?
Yes, that consequence would follow - it is the same branch. But the
antecedent cannot hold, for the reasons in my reply to the first half: the
fallback and a later increase cannot both happen to one port.
Worth separating the two cases, though, because the symptom you describe is
also the intended behaviour here. When a real user table is carried across an
increase, the added queues do stay unsteered:
# ethtool -X ens1 equal 2
# ethtool -L ens1 combined 16 /* queues 2..15 get no traffic */
That is deliberate. The alternative is to silently discard a table the user
asked for, which is what this patch exists to stop, and it is what ice
(ice_set_channels()), idpf, hns3, fbnic and the rest do as well. The user
recovers with another ethtool -X, and "ethtool -x" always shows the table
actually in use.
So unsteered queues after a grow are not a bug signature by themselves. The
bug would be reaching that state with a table the user never asked for, and
that needs the desync, which cannot occur.
In summary, this patch behavior is consistent with other drivers on this
scenario.
>
> [ ... ]
>
> [Severity: Critical]
> This is a pre-existing issue, but is there a Use-After-Free in the stats
> collection
> due to the ad-hoc port_is_up synchronization flag?
>
> Looking at mana_get_stats64(), the reader path relies on the ad-hoc
> port_is_up flag and dereferences the queue structure, but can be preempted
> before accessing it:
>
> drivers/net/ethernet/microsoft/mana/mana_en.c:mana_get_stats64() {
> if (!apc->port_is_up)
> return;
> ...
> for (q = 0; q < num_queues; q++) {
> rx_stats = &apc->rxqs[q]->stats;
> ...
> }
>
> Concurrently, a teardown path clears the flag and proceeds to free the
> underlying memory structures without waiting for concurrent readers:
>
> drivers/net/ethernet/microsoft/mana/mana_en.c:mana_detach() {
> apc->port_st_save = apc->port_is_up;
> apc->port_is_up = false;
>
> /* Ensure port state updated before txq state */
> smp_wmb();
>
> netif_tx_disable(ndev);
>
> if (apc->port_st_save) {
> err = mana_dealloc_queues(ndev);
> ...
> }
>
> The memory is then synchronously freed, guaranteeing a Use-After-Free if a
> reader is mid-execution:
>
> drivers/net/ethernet/microsoft/mana/mana_en.c:mana_destroy_txq() { ...
> mana_deinit_txq(apc, &apc->tx_qp[i]->txq);
>
> kvfree(apc->tx_qp[i]);
> ...
> }
>
> Could a preempted reader accessing the freed memory lead to memory
> corruption?
This is a pre-existing issue and out of scope from this patch.
I needed, I suggest submitting another patch to fix this.
>
> --
> Sashiko AI
> review * https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Fsashiko.dev%2F%23%2Fpatchset%2F20260905004401.3937066-1-
> longli%40microsoft.com%3Fpart%3D1&data=05%7C02%7Clongli%40microsoft.
> com%7C18ce764cb5f6435395b608df0bb00dee%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C639242522911822221%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
> OIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=IML49OomMP48q
> RaUNNmX7zbI%2BIQ31FQZH5OhErqzovQ%3D&reserved=0
