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. > + > 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/[email protected]?part=1
