Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Critical: 0 · High: 1 (1) · Medium: 1 · Low: 1 - [High] GC_REMOVING is a one-way latch that is set on service exits which do not necessarily remove the device. - [Medium] The new `if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags))` gate in `mana_gf_stats_work_handler()` (mana_en.c:4068) silently discards… - [Low] The new service-work submission uses `queue_work(system_wq, &gc->serv_work)` (gdma_main.c:886). Pre-existing issues: - [High] `mana_gd_shutdown()` (gdma_main.c:2776) tears down the HWC — `mana_gd_cleanup_device()` -> `mana_hwc_destroy_channel()` ->… -- net: mana: fix reset work race with device removal The service work is moved into struct gdma_context and GC_IN_SERVICE now describes whether the work may still touch gc. Removal and the probe unwind set a new GC_REMOVING bit to close admission and then wait for an in-flight cycle to retire before clearing drvdata and freeing gc. Reset events observed during probe are latched in GC_SERVICE_DURING_PROBE instead of being admitted immediately. No Link: tags in the commit header. Earlier postings referenced in the changelog: https://lore.kernel.org/netdev/[email protected] https://lore.kernel.org/netdev/[email protected] > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index f92b2d0bf926..28544f00e8ce 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > @@ -678,10 +678,38 @@ static void mana_serv_rescan(struct pci_dev *pdev) > pci_unlock_rescan_remove(); > } > > -static void mana_serv_fpga(struct pci_dev *pdev) > +/* Retire one service cycle: GC_IN_SERVICE is the last state published > + * by the work, so remove() and the probe unwind may free gc once it > + * is clear. Must run before the exit paths take pci_lock_rescan_remove(): > + * removal can be waiting on the bit while holding that lock. > + */ > +static void mana_service_done(struct gdma_context *gc) > +{ > + /* Pairs with test_bit_acquire() on the wait side. */ > + clear_bit_unlock(GC_IN_SERVICE, &gc->flags); > + /* wake_up_var() requires a full barrier between the update of the > + * waited-on variable and the wake. > + */ > + smp_mb__after_atomic(); > + wake_up_var(&gc->flags); > +} > + > +/* Retire a cycle whose exit removes the device: close admission > + * first so no new cycle can be admitted in the retire window. > + */ > +static void mana_service_done_removing(struct gdma_context *gc) > +{ > + set_bit(GC_REMOVING, &gc->flags); > + mana_service_done(gc); > +} > + > +static void mana_serv_fpga(struct pci_dev *pdev, struct gdma_context *gc) > { > struct pci_bus *bus, *parent; > > + if (gc) > + mana_service_done_removing(gc); > + > pci_lock_rescan_remove(); > > bus = pdev->bus; > @@ -706,9 +734,8 @@ static void mana_serv_fpga(struct pci_dev *pdev) > pci_unlock_rescan_remove(); > } > > -static void mana_serv_reset(struct pci_dev *pdev) > +static void mana_serv_reset(struct pci_dev *pdev, struct gdma_context *gc) > { > - struct gdma_context *gc = pci_get_drvdata(pdev); > struct hw_channel_context *hwc; > int ret; > > @@ -738,6 +765,7 @@ static void mana_serv_reset(struct pci_dev *pdev) > if (ret == -ETIMEDOUT || ret == -EPROTO) { > /* Perform PCI rescan on device if we failed on HWC */ > dev_err(&pdev->dev, "MANA service: resume failed, > rescanning\n"); > + mana_service_done_removing(gc); > mana_serv_rescan(pdev); > return; > } [Severity: High] Is GC_REMOVING guaranteed to be followed by an actual removal here? Both callers of mana_service_done_removing() can return with the device still bound. In mana_serv_fpga(), the bit is set before the bus checks: if (gc) mana_service_done_removing(gc); pci_lock_rescan_remove(); bus = pdev->bus; if (!bus) { dev_err(&pdev->dev, "MANA service: no bus\n"); goto out; } parent = bus->parent; if (!parent) { dev_err(&pdev->dev, "MANA service: no parent bus\n"); goto out; } bus->parent is NULL whenever the MANA function sits on a root bus, so this exits through "goto out" without calling pci_stop_and_remove_bus_device(). The failed-resume path has the same shape: mana_serv_rescan() has its own "if (!parent)" bail-out that returns without removing the device, after GC_REMOVING has already been latched. I can't find any code that clears GC_REMOVING again. Once it is set on a live gc, mana_schedule_serv_work() refuses every subsequent request: if (test_bit(GC_REMOVING, &gc->flags)) { dev_info(gc->dev, "Device is being removed\n"); mana_service_done(gc); return -EBUSY; } and mana_tx_timeout() in mana_en.c also stops queueing queue-reset work for the rest of the binding: if (test_bit(GC_IN_SERVICE, &gc->flags) || test_bit(GC_REMOVING, &gc->flags)) return; Does this leave a still-bound device permanently without HWC reset service and without TX queue reset? Would it be better to set GC_REMOVING only after the point where removal is known to happen, or to clear it on the bail-out paths? > @@ -796,48 +841,49 @@ static void mana_recovery_delayed_func(struct > work_struct *w) [ ... ] > int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type) > { > - struct mana_serv_work *mns_wk; > - > if (test_and_set_bit(GC_IN_SERVICE, &gc->flags)) { > dev_info(gc->dev, "Already in service\n"); > return -EBUSY; > } > > + /* Pairs with set_bit(GC_REMOVING) + smp_mb__after_atomic() in > + * mana_gd_remove(): the test_and_set_bit() above is fully ordered, > + * so either this read sees GC_REMOVING and the cycle aborts, or > + * remove() observes GC_IN_SERVICE and waits for the cycle to retire > + * before it frees gc. > + */ > + if (test_bit(GC_REMOVING, &gc->flags)) { > + dev_info(gc->dev, "Device is being removed\n"); > + mana_service_done(gc); > + return -EBUSY; > + } > + > if (!try_module_get(THIS_MODULE)) { > dev_info(gc->dev, "Module is unloading\n"); > - clear_bit(GC_IN_SERVICE, &gc->flags); > + mana_service_done(gc); > return -ENODEV; > } > > - mns_wk = kzalloc(sizeof(*mns_wk), GFP_ATOMIC); > - if (!mns_wk) { > - module_put(THIS_MODULE); > - clear_bit(GC_IN_SERVICE, &gc->flags); > - return -ENOMEM; > - } > - > dev_info(gc->dev, "Start MANA service type:%d\n", type); > - mns_wk->pdev = to_pci_dev(gc->dev); > - mns_wk->type = type; > - pci_dev_get(mns_wk->pdev); > - INIT_WORK(&mns_wk->serv_work, mana_serv_func); > - schedule_work(&mns_wk->serv_work); > + > + gc->serv_type = type; > + pci_dev_get(to_pci_dev(gc->dev)); > + queue_work(system_wq, &gc->serv_work); > return 0; > } [Severity: Low] This isn't a bug, but was system_wq intended here rather than system_percpu_wq? In this tree the declaration is: include/linux/workqueue.h: extern struct workqueue_struct *system_wq; /* use system_percpu_wq, this will be removed */ and system_wq is allocated with __WQ_DEPRECATED, so __queue_work() emits: kernel/workqueue.c:__queue_work() { if (unlikely(wq->flags & __WQ_DEPRECATED)) pr_warn_once("workqueue: work func %ps enqueued on deprecated workqueue. " "Use system_{percpu|dfl}_wq instead.\n", work->func); } The code being replaced used schedule_work(), which queues on system_percpu_wq. The stated reason for staying off gc->service_wq is satisfied equally by schedule_work(&gc->serv_work). [ ... ] > @@ -2624,6 +2700,20 @@ static void mana_gd_remove(struct pci_dev *pdev) > { > struct gdma_context *gc = pci_get_drvdata(pdev); > > + /* Close admission and retire an in-flight cycle before any > + * teardown: the service work is the only user of gc that > + * remove() does not otherwise synchronise with. The service > + * exits retire before taking the PCI rescan/remove lock, so > + * this wait cannot deadlock against them. > + */ > + set_bit(GC_REMOVING, &gc->flags); > + /* Pairs with the ordered test_and_set_bit(GC_IN_SERVICE) in > + * mana_schedule_serv_work(). > + */ > + smp_mb__after_atomic(); > + wait_var_event(&gc->flags, > + !test_bit_acquire(GC_IN_SERVICE, &gc->flags)); > + > pci_disable_sriov(pdev); > > mana_rdma_remove(&gc->mana_ib); [Severity: High] This isn't a bug introduced by this patch, but should mana_gd_shutdown() get the same admission-close and drain that is added here? It tears the HWC down with no synchronisation against an in-flight service cycle: static void mana_gd_shutdown(struct pci_dev *pdev) { struct gdma_context *gc = pci_get_drvdata(pdev); dev_info(&pdev->dev, "Shutdown was called\n"); mana_rdma_remove(&gc->mana_ib); mana_remove(&gc->mana, true); mana_gd_cleanup_device(pdev); pci_disable_device(pdev); } Meanwhile mana_serv_reset() caches the HWC pointer and then sleeps for MANA_SERVICE_PERIOD seconds across suspend/resume: hwc = gc->hwc.driver_data; ... hwc->hwc_timeout = 0; ... mana_gd_suspend(pdev, PMSG_SUSPEND); msleep(MANA_SERVICE_PERIOD * 1000); ret = mana_gd_resume(pdev); mana_gd_cleanup_device() -> mana_hwc_destroy_channel() does kfree(hwc), and both paths call it. device_shutdown() takes only device_lock(&pdev->dev), which the service work does not hold, so on reboot or kexec during a reset cycle can this end up as a use-after-free of hwc and a second kfree()/destroy_workqueue() on the same objects? > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c > b/drivers/net/ethernet/microsoft/mana/mana_en.c > index 591fb4191d90..c297de8bb762 100644 > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c [ ... ] > @@ -4061,9 +4061,12 @@ static void mana_gf_stats_work_handler(struct > work_struct *work) > dev_warn(gc->dev, > "Gf stats wk handler: gf stats query timed out.\n"); > /* As HWC timed out, indicating a faulty HW state and needs a > - * reset. > + * reset. Never admit service work before the probe has > + * completed: a probe that is failing unwinds netdevs and the > + * HWC channel itself and cannot drain a cycle. > */ > - mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST); > + if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags)) > + mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST); > return; > } > schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD); [Severity: Medium] What happens to the reset request when this gate is false but the probe then succeeds? mana_probe() arms this work before returning to mana_gd_probe(): schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD); with MANA_GF_STATS_PERIOD being 2 * HZ, while mana_gd_probe() still has mana_rdma_probe() to run before it publishes the flag: set_bit(GC_PROBE_SUCCEEDED, &gc->flags); if (test_and_set_bit(GC_SERVICE_DURING_PROBE, &gc->flags)) { So a mana_query_gf_stats() timeout in that window takes the new branch, drops the request, and the bare "return;" also skips the schedule_delayed_work() re-arm below it. The EQE handler in gdma_main.c latches and re-checks instead: if (!test_bit(GC_PROBE_SUCCEEDED, &gc->flags)) { if (!test_and_set_bit(GC_SERVICE_DURING_PROBE, &gc->flags)) dev_info(gc->dev, "Service is to be processed in probe\n"); else if (test_bit(GC_PROBE_SUCCEEDED, &gc->flags)) mana_schedule_serv_work(gc, type); break; } Here there is neither a latch nor a re-check, and the plain test_bit() also races the set_bit() at the probe boundary. If the probe goes on to succeed, the device is left with ac->hwc_timeout_occurred set, the stats poll never re-armed, and no reset cycle requested, where the previous code called mana_schedule_serv_work() unconditionally. Should this call site latch into GC_SERVICE_DURING_PROBE (or re-check after the boundary) so the request is not lost? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909042529.652301-1-fanwu01%40zju.edu.cn
