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