Re: [PATCH net-next v2 07/13] net: mana: do not bail out of mana_detach on dealloc failure

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The 'channel_changing' boolean flag is used as an ad-hoc lock to guard code sections and prevent concurrent access, violating kernel synchronization guidelines.

Pre-existing issues:
- [Critical] Hardware queue memory and RX page pools are freed while the device may still be DMAing to them if it fails to quiesce during teardown (DMA Use-After-Free).
--

--- Patch [7]: [PATCH net-next v2 07/13] net: mana: do not bail out of mana_detach on dealloc failure ---
commit 04170ba1361c402d70e39fad639e6d6455da30a2
Author: Dipayaan Roy <[email protected]>

net: mana: do not bail out of mana_detach on dealloc failure

This commit modifies mana_detach() to remove the early return if
mana_dealloc_queues() fails. This ensures that the function completes its
full teardown, including netif_device_detach() and mana_cleanup_port_context(),
avoiding an inconsistent port state.

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index e54ad4db918ce..2b3250483b439 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -4502,10 +4502,8 @@ int mana_detach(struct net_device *ndev, bool from_close)
>  
>  	if (apc->port_st_save) {
>  		err = mana_dealloc_queues(ndev);

[Severity: Critical]
This is a pre-existing issue, but does mana_dealloc_queues() safely handle a
failure to quiesce the device?

If mana_drain_txqs() times out and pci_try_reset_function() fails, it skips
freeing the TX SKB buffers and returns false:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_drain_txqs() {
    ...
		err = pci_try_reset_function(to_pci_dev(gd->gdma_context->dev));
		if (err) {
			netdev_err(apc->ndev,
				   "function reset failed: %d, %d pkts pending in txq %u\n",
				   err, atomic_read(&txq->pending_sends),
				   txq->gdma_txq_id);
			quiesced = false;
    ...
	if (!quiesced) {
		netdev_err(apc->ndev,
			   "device not quiesced, leaking pending TX buffers instead of unmapping memory it can still DMA from\n");
		return reset;
	}
    ...
}

However, mana_dealloc_queues() ignores this false return value and proceeds
with the teardown:

drivers/net/ethernet/microsoft/mana/mana_en.c:mana_dealloc_queues() {
    ...
	if (mana_drain_txqs(apc)) {
		struct mana_context *ac = apc->ac;
		unsigned int i;
    ...
	/* Even in err case, still need to cleanup the vPort */
	mana_destroy_rxqs(apc);
	mana_destroy_txq(apc);
    ...
}

Because the device was never successfully quiesced, it remains active. Will
unconditionally freeing the GDMA SQ and RQ hardware ring buffers and RX page
pool buffers here lead to a DMA use-after-free when the hardware continues to
DMA read/write to the now-freed memory?

> -		if (err) {
> +		if (err)
>  			netdev_err(ndev, "%s failed to deallocate queues: %d\n", __func__, err);
> -			return err;
> -		}
>  	}

[Severity: High]
Is the channel_changing boolean flag being used as an ad-hoc lock to guard
code sections and prevent concurrent access?

drivers/net/ethernet/microsoft/mana/mana_ethtool.c:mana_set_ringparam() {
    ...
	mutex_lock(&apc->vport_mutex);
	if (apc->channel_changing) {
		mutex_unlock(&apc->vport_mutex);
		return -EBUSY;
	}
	apc->channel_changing = true;
	mutex_unlock(&apc->vport_mutex);
    ...
}

Using a boolean flag to protect entire blocks of code bypasses standard kernel
locking mechanisms. This prevents lockdep from analyzing lock dependencies and
forces concurrent callers to fail with -EBUSY instead of gracefully waiting.

Could a proper mutex or rwsem be used here instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7