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
