RE: [EXTERNAL] 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?

Pre-existing behavior exactly the same in upstream, and the ordering already accounts for it:  mana_destroy_txq() / mana_destroy_rxq()  call  mana_destroy_wq_obj()  first, which tells the device to stop using the queue, and only then free the rings and page pool. This patch set doesn't attempt to address this.

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

This is the pre-existing upstream locking mechanism. The patch set doesn't attempt to change it.

Long