Re: [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild

[sashiko-bot@kernel.org]

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 <longli@microsoft.com>

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.

> +
>  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?

[ ... ]

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905004401.3937066-1-longli@microsoft.com?part=1