Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] A timed-out HWC request recycles its msg_id immediately, which can cause a delayed hardware response to corrupt the memory of a subsequent request that reused the same msg_id. - [High] The dynamically allocated `mana_serv_work` item is not cancelled during device removal, leading to a race condition where the work item can access a freed `gdma_context` (`gc`). -- --- Patch [5]: [PATCH net v7 5/7] net: mana: fix HWC teardown safety with setup_active flag and destroy ordering --- commit f7262210dd3ba2257a4ad8d7840cc3065a491ae1 Author: Long Li <[email protected]> net: mana: fix HWC teardown safety with setup_active flag and destroy ordering This patch addresses teardown hazards that could allow the hardware to access freed memory. It introduces a setup_active flag to ensure teardowns only occur when necessary, reorders queue destruction to prevent interrupts from accessing freed buffers, and intentionally leaks resources if the teardown itself fails. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c [ ... ] > @@ -837,6 +844,16 @@ int mana_hwc_create_channel(struct gdma_context *gc) > u16 q_depth_max; > int err; > > + /* A previous teardown may have failed and left the old context > + * reachable. Retry it before building a new channel; if it still > + * fails, return an error so mana_serv_reset() does a full PCI rescan. > + */ [Severity: High] This isn't a bug introduced by this patch, but since mana_serv_reset() is mentioned here, is there a use-after-free risk during device removal? If an error triggers mana_schedule_serv_work(), it schedules a dynamically allocated work item on the system workqueue: drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_schedule_serv_work() { ... INIT_WORK(&mns_wk->serv_work, mana_serv_func); schedule_work(&mns_wk->serv_work); ... } If the driver is unbound concurrently, mana_gd_remove() frees the gdma_context without waiting for or cancelling this work item: drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_remove() { ... vfree(gc); ... } Could the pending work item then execute mana_serv_reset(), read the context via pci_get_drvdata(), and dereference the freed gc pointer? [ ... ] > @@ -914,6 +953,12 @@ void mana_hwc_destroy_channel(struct gdma_context *gc) > */ > gc->max_num_cqs = 0; > > + if (hwc->txq) > + mana_hwc_destroy_wq(hwc, hwc->txq); > + > + if (hwc->rxq) > + mana_hwc_destroy_wq(hwc, hwc->rxq); > + > kfree(hwc->caller_ctx); > hwc->caller_ctx = NULL; [Severity: High] This is a pre-existing issue, but while reviewing the caller context cleanup, is there a memory corruption risk if a hardware channel request times out? If a request times out in mana_hwc_send_request(), the msg_id is immediately recycled: drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_send_request() { ... out: mana_hwc_put_msg_index(hwc, msg_id); return err; } If another thread allocates the same msg_id, and the hardware later sends a delayed response for the original timed-out request, could mana_hwc_handle_resp() overwrite the new request's memory? drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_handle_resp() { ... memcpy(ctx->output_buf, resp_msg, resp_len); ... complete(&ctx->comp_event); } Could this delayed response blindly overwrite the newly allocated caller's output_buf and prematurely wake it up? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
