Re: [PATCH net v3] net: mana: fix reset work race with device removal

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/20260905023602.425827-1-fanwu01@zju.edu.cn (local)
  https://lore.kernel.org/netdev/20260805143812.220509-1-fanwu01@zju.edu.cn (local)

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

[ ... ]

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

[ ... ]

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

[ ... ]

[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