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

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

[Fan Wu]

The reset service work is queued on the system workqueue and can
outlive mana_gd_remove(), which frees the GDMA context. It may then
dereference gc through the stale service work.

Embed the service work in gdma_context and guard its lifecycle with a
new serv_lock (hard IRQ safe, never held across a sleep) plus a
waitqueue. Removal, shutdown and the probe unwind close admission and
wait for an in-flight cycle to retire before clearing drvdata and
freeing gc. Service exits retire before taking the PCI rescan/remove
lock, avoiding a lock-cycle with remove.

Do not admit service work while probe is still constructing or
unwinding the device. Latch reset events seen during probe under the
same lock and let the probe rollback/recovery path handle them; an
event racing probe completion is admitted rather than lost. A service
exit that bails out of a rescan without removing the device (no parent
bus) retires without closing admission, so service is never
permanently disabled on a bound device.

The service work stays on the system workqueue because a reset cycle
destroys and re-creates gc->service_wq.

This issue was found by an in-house static analysis tool.

Fixes: fbe346ce9d62 ("net: mana: Handle Reset Request from MANA NIC")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 .../net/ethernet/microsoft/mana/gdma_main.c   | 273 ++++++++++++++----
 drivers/net/ethernet/microsoft/mana/mana_en.c |  13 +-
 include/net/mana/gdma.h                       |  29 +-
 3 files changed, 238 insertions(+), 77 deletions(-)

Changes since v3 (<20260909042529.652301-1-fanwu01@zju.edu.cn>,
https://lore.kernel.org/netdev/20260909042529.652301-1-fanwu01@zju.edu.cn):

- Replaced the flag protocol with a gc-embedded spinlock and waitqueue
  (Jakub Kicinski): admission, retirement and the probe boundary
  handshake now run under serv_lock instead of atomic bit pairings.

- A rescan exit that bails out without removing the device (no parent
  bus) now retires without closing admission, so service is not
  permanently disabled on a bound device.

- mana_gd_shutdown() also drains an in-flight cycle before tearing
  down the same hardware paths as remove().

Changes since v2 (<20260905023602.425827-1-fanwu01@zju.edu.cn>,
https://lore.kernel.org/netdev/20260905023602.425827-1-fanwu01@zju.edu.cn):

- Dropped the now-unused cleanup_mana_rdma and cleanup_mana labels in
  the probe error path (Simon Horman).

Changes since v1 (<20260805143812.220509-1-fanwu01@zju.edu.cn>,
https://lore.kernel.org/netdev/20260805143812.220509-1-fanwu01@zju.edu.cn):

- Dropped the device_lock() serialisation: holding the driver-core
  device lock across mana_gd_suspend() + msleep() + mana_gd_resume()
  blocks unbind, reboot, device PM and all PCI hotplug for up to a
  full reset cycle, and can deadlock against the
  flush_workqueue()/destroy_workqueue() of gc->service_wq.

- The freeing paths close admission and wait for an admitted cycle to
  retire before clearing drvdata and calling vfree(); the failed-resume
  rescan no longer reopens admission, and mana_tx_timeout() also skips
  queue-reset work during removal.

- No service work is admitted before the probe completes; the FPGA
  reconfig exit and the probe-failure recovery path are covered as
  well. The stats-work gate during probe is deliberate: a failing
  probe cannot drain a cycle, and the probe's own -ETIMEDOUT rollback
  plus the recovery rescan service the skipped event.

- Reference series for the HWC lifecycle model:
  https://lore.kernel.org/netdev/20260813174243.3044348-1-longli@microsoft.com

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index 8e9bfc1..b91d254 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -659,63 +659,149 @@ EXPORT_SYMBOL_NS(mana_gd_ring_dim, "NET_MANA");
 
 #define MANA_SERVICE_PERIOD 10
 
-static void mana_serv_rescan(struct pci_dev *pdev)
+/* Retire one service cycle: serv_waitq is embedded in gc, so the wake
+ * must happen before the lock is dropped - a drainer that observes
+ * idle cannot free gc until the wake has stopped touching gc.
+ */
+static void mana_service_retire(struct gdma_context *gc)
 {
-	struct pci_bus *parent;
+	unsigned long flags;
 
-	pci_lock_rescan_remove();
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	gc->serv_in_flight = false;
+	wake_up(&gc->serv_waitq);
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+}
+
+/* Close admission atomically with the retire itself. */
+static void mana_service_retire_removing(struct gdma_context *gc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	gc->serv_removing = true;
+	gc->serv_in_flight = false;
+	wake_up(&gc->serv_waitq);
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+}
+
+static bool mana_service_idle(struct gdma_context *gc)
+{
+	unsigned long flags;
+	bool idle;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	idle = !gc->serv_in_flight;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+	return idle;
+}
+
+/* Close admission and wait until an in-flight cycle has stopped touching
+ * gc.  The cycle retires before taking the PCI rescan/remove lock, so
+ * this wait completes even when the caller holds that lock; serv_lock is
+ * never held across a sleep.
+ */
+static void mana_service_quiesce(struct gdma_context *gc)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	gc->serv_removing = true;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+
+	wait_event(gc->serv_waitq, mana_service_idle(gc));
+}
+
+/* May run in softirq context (tx timeout). */
+bool mana_service_active(struct gdma_context *gc)
+{
+	unsigned long flags;
+	bool active;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	active = gc->serv_in_flight || gc->serv_removing;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+	return active;
+}
+
+bool mana_service_probe_done(struct gdma_context *gc)
+{
+	unsigned long flags;
+	bool done;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	done = gc->serv_probe_done;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+	return done;
+}
+
+static void mana_serv_rescan(struct pci_dev *pdev, struct gdma_context *gc)
+{
+	struct pci_bus *parent;
 
 	parent = pdev->bus;
 	if (!parent) {
 		dev_err(&pdev->dev, "MANA service: no parent bus\n");
-		goto out;
+		/* Still bound: keep admission open for later events. */
+		if (gc)
+			mana_service_retire(gc);
+		return;
 	}
 
+	if (gc)
+		mana_service_retire_removing(gc);
+
+	pci_lock_rescan_remove();
+
 	pci_stop_and_remove_bus_device(pdev);
 	pci_rescan_bus(parent);
 
-out:
 	pci_unlock_rescan_remove();
 }
 
-static void mana_serv_fpga(struct pci_dev *pdev)
+static void mana_serv_fpga(struct pci_dev *pdev, struct gdma_context *gc)
 {
 	struct pci_bus *bus, *parent;
 
-	pci_lock_rescan_remove();
-
 	bus = pdev->bus;
 	if (!bus) {
 		dev_err(&pdev->dev, "MANA service: no bus\n");
-		goto out;
+		if (gc)
+			mana_service_retire(gc);
+		return;
 	}
 
 	parent = bus->parent;
 	if (!parent) {
 		dev_err(&pdev->dev, "MANA service: no parent bus\n");
-		goto out;
+		if (gc)
+			mana_service_retire(gc);
+		return;
 	}
 
+	if (gc)
+		mana_service_retire_removing(gc);
+
+	pci_lock_rescan_remove();
+
 	pci_stop_and_remove_bus_device(bus->self);
 
 	msleep(MANA_SERVICE_PERIOD * 1000);
 
 	pci_rescan_bus(parent);
 
-out:
 	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;
 
 	if (!gc) {
 		/* Perform PCI rescan on device if GC is not set up */
 		dev_err(&pdev->dev, "MANA service: GC not setup, rescanning\n");
-		mana_serv_rescan(pdev);
+		mana_serv_rescan(pdev, NULL);
 		return;
 	}
 
@@ -738,7 +824,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_serv_rescan(pdev);
+		mana_serv_rescan(pdev, gc);
 		return;
 	}
 
@@ -748,22 +834,25 @@ static void mana_serv_reset(struct pci_dev *pdev)
 		dev_info(&pdev->dev, "MANA reset cycle completed\n");
 
 out:
-	clear_bit(GC_IN_SERVICE, &gc->flags);
+	mana_service_retire(gc);
 }
 
-static void mana_do_service(enum gdma_eqe_type type, struct pci_dev *pdev)
+static void mana_do_service(enum gdma_eqe_type type, struct pci_dev *pdev,
+			    struct gdma_context *gc)
 {
 	switch (type) {
 	case GDMA_EQE_HWC_FPGA_RECONFIG:
-		mana_serv_fpga(pdev);
+		mana_serv_fpga(pdev, gc);
 		break;
 
 	case GDMA_EQE_HWC_RESET_REQUEST:
-		mana_serv_reset(pdev);
+		mana_serv_reset(pdev, gc);
 		break;
 
 	default:
 		dev_err(&pdev->dev, "MANA service: unknown type %d\n", type);
+		if (gc)
+			mana_service_retire(gc);
 		break;
 	}
 }
@@ -779,12 +868,26 @@ static void mana_recovery_delayed_func(struct work_struct *w)
 	spin_lock_irqsave(&work->lock, flags);
 
 	while (!list_empty(&work->dev_list)) {
+		struct gdma_context *gc;
+
 		dev = list_first_entry(&work->dev_list,
 				       struct mana_dev_recovery, list);
 		list_del(&dev->list);
 		spin_unlock_irqrestore(&work->lock, flags);
 
-		mana_do_service(dev->type, dev->pdev);
+		/* Serialize the drvdata lookup and admission against
+		 * probe/remove.  Do not call sleeping functions while
+		 * holding the device lock.
+		 */
+		device_lock(&dev->pdev->dev);
+		gc = pci_get_drvdata(dev->pdev);
+		if (gc)
+			mana_schedule_serv_work(gc, dev->type);
+		device_unlock(&dev->pdev->dev);
+
+		if (!gc)
+			mana_do_service(dev->type, dev->pdev, NULL);
+
 		pci_dev_put(dev->pdev);
 		kfree(dev);
 
@@ -796,51 +899,94 @@ static void mana_recovery_delayed_func(struct work_struct *w)
 
 static void mana_serv_func(struct work_struct *w)
 {
-	struct mana_serv_work *mns_wk;
-	struct pci_dev *pdev;
-
-	mns_wk = container_of(w, struct mana_serv_work, serv_work);
-	pdev = mns_wk->pdev;
+	struct gdma_context *gc = container_of(w, struct gdma_context, serv_work);
+	struct pci_dev *pdev = to_pci_dev(gc->dev);
 
-	if (pdev)
-		mana_do_service(mns_wk->type, pdev);
+	mana_do_service(gc->serv_type, pdev, gc);
 
+	/* The rescan exits of mana_do_service() remove the device, which
+	 * frees gc before returning.  Only touch the pdev and the module
+	 * reference from here on; both are held until this point drops them.
+	 */
 	pci_dev_put(pdev);
-	kfree(mns_wk);
 	module_put(THIS_MODULE);
 }
 
+/* Serialize admission with retirement.  Called in hard IRQ context, so
+ * serv_lock is taken with interrupts disabled and nothing that may
+ * sleep runs under it.
+ */
 int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
 {
-	struct mana_serv_work *mns_wk;
+	unsigned long flags;
+	bool busy;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	busy = gc->serv_removing || gc->serv_in_flight;
+	if (!busy)
+		gc->serv_in_flight = true;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
 
-	if (test_and_set_bit(GC_IN_SERVICE, &gc->flags)) {
+	if (busy) {
 		dev_info(gc->dev, "Already in service\n");
 		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_retire(gc);
 		return -ENODEV;
 	}
 
-	mns_wk = kzalloc_obj(*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;
 }
 
+/* EQ-event side of the probe boundary: latch the event while the probe
+ * is still running and let the probe roll back (the recovery path will
+ * rescan), or admit the cycle once the probe has completed.  The probe
+ * tail stores probe_done and reads the latch under the same lock, so an
+ * event racing probe completion is admitted rather than lost.
+ */
+static void mana_service_event(struct gdma_context *gc,
+			       enum gdma_eqe_type type)
+{
+	unsigned long flags;
+	bool admit, first;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	admit = gc->serv_probe_done;
+	first = !gc->serv_during_probe;
+	if (!admit)
+		gc->serv_during_probe = true;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+
+	if (admit)
+		mana_schedule_serv_work(gc, type);
+	else if (first)
+		dev_info(gc->dev, "Service is to be processed in probe\n");
+}
+
+/* Probe-tail side of the probe boundary: record probe completion and
+ * return whether an event was latched meanwhile; the same-lock pairing
+ * with mana_service_event() keeps a racing event from being lost.
+ */
+static bool mana_service_probe_complete(struct gdma_context *gc)
+{
+	unsigned long flags;
+	bool rollback;
+
+	spin_lock_irqsave(&gc->serv_lock, flags);
+	gc->serv_probe_done = true;
+	rollback = gc->serv_during_probe;
+	spin_unlock_irqrestore(&gc->serv_lock, flags);
+	return rollback;
+}
+
 /* Return the CPU address of byte @offset within a queue's ring buffer. */
 static void *mana_gd_ring_ptr(const struct gdma_queue *q, u32 offset)
 {
@@ -956,18 +1102,7 @@ static void mana_gd_process_eqe(struct gdma_queue *eq)
 	case GDMA_EQE_HWC_FPGA_RECONFIG:
 	case GDMA_EQE_HWC_RESET_REQUEST:
 		dev_info(gc->dev, "Recv MANA service type:%d\n", type);
-
-		if (!test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
-			/*
-			 * Device is in probe and we received a hardware reset
-			 * event, the probe function will detect that the flag
-			 * has changed and perform service procedure.
-			 */
-			dev_info(gc->dev,
-				 "Service is to be processed in probe\n");
-			break;
-		}
-		mana_schedule_serv_work(gc, type);
+		mana_service_event(gc, type);
 		break;
 
 	default:
@@ -2502,6 +2637,7 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	void __iomem *bar0_va;
 	int bar = 0;
 	int err;
+	bool rollback;
 
 	/* Each port has 2 CQs, each CQ has at most 1 EQE at a time */
 	BUILD_BUG_ON(2 * MAX_PORTS_IN_MANA_DEV * GDMA_EQE_SIZE > EQ_SIZE);
@@ -2532,6 +2668,9 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	mutex_init(&gc->eq_test_event_mutex);
 	mutex_init(&gc->gic_mutex);
+	INIT_WORK(&gc->serv_work, mana_serv_func);
+	spin_lock_init(&gc->serv_lock);
+	init_waitqueue_head(&gc->serv_waitq);
 	pci_set_drvdata(pdev, gc);
 	gc->bar0_pa = pci_resource_start(pdev, 0);
 	gc->bar0_size = pci_resource_len(pdev, 0);
@@ -2558,22 +2697,26 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	err = mana_rdma_probe(&gc->mana_ib);
 	if (err)
-		goto cleanup_mana;
+		goto service_quiesce;
 
 	/*
 	 * If a hardware reset event has occurred over HWC during probe,
 	 * rollback and perform hardware reset procedure.
 	 */
-	if (test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
+	rollback = mana_service_probe_complete(gc);
+	if (rollback) {
 		err = -EPROTO;
-		goto cleanup_mana_rdma;
+		goto service_quiesce;
 	}
 
 	return 0;
 
-cleanup_mana_rdma:
+service_quiesce:
+	/* The stats work can admit service once mana_probe() has run:
+	 * retire an in-flight cycle before any teardown.
+	 */
+	mana_service_quiesce(gc);
 	mana_rdma_remove(&gc->mana_ib);
-cleanup_mana:
 	mana_remove(&gc->mana, false);
 cleanup_gd:
 	mana_gd_cleanup_device(pdev);
@@ -2581,6 +2724,8 @@ unmap_bar:
 	xa_destroy(&gc->irq_contexts);
 	pci_iounmap(pdev, bar0_va);
 free_gc:
+	/* Backstop: drain before every vfree(). */
+	mana_service_quiesce(gc);
 	pci_set_drvdata(pdev, NULL);
 	vfree(gc);
 release_region:
@@ -2624,6 +2769,9 @@ static void mana_gd_remove(struct pci_dev *pdev)
 {
 	struct gdma_context *gc = pci_get_drvdata(pdev);
 
+	/* Drain the only gc user remove() does not synchronise with. */
+	mana_service_quiesce(gc);
+
 	pci_disable_sriov(pdev);
 
 	mana_rdma_remove(&gc->mana_ib);
@@ -2635,6 +2783,8 @@ static void mana_gd_remove(struct pci_dev *pdev)
 
 	pci_iounmap(pdev, gc->bar0_va);
 
+	/* Prevent late recovery work from using freed gc. */
+	pci_set_drvdata(pdev, NULL);
 	vfree(gc);
 
 	pci_release_regions(pdev);
@@ -2687,6 +2837,9 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
 
 	dev_info(&pdev->dev, "Shutdown was called\n");
 
+	/* Shutdown tears down the same HW paths as remove(). */
+	mana_service_quiesce(gc);
+
 	mana_rdma_remove(&gc->mana_ib);
 	mana_remove(&gc->mana, true);
 
diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 591fb41..0eb23ae 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -924,8 +924,8 @@ static void mana_tx_timeout(struct net_device *netdev, unsigned int txqueue)
 		return;
 	}
 
-	/* Already in service, hence tx queue reset is not required.*/
-	if (test_bit(GC_IN_SERVICE, &gc->flags))
+	/* Skip while a service cycle may still touch gc. */
+	if (mana_service_active(gc))
 		return;
 
 	/* Note: If there are pending queue reset work for this port(apc),
@@ -4060,10 +4060,13 @@ static void mana_gf_stats_work_handler(struct work_struct *work)
 		memset(&ac->hc_stats, 0, sizeof(ac->hc_stats));
 		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.
+		/* As HWC timed out, indicating a faulty HW state and
+		 * needs a 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 (mana_service_probe_done(gc))
+			mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
 		return;
 	}
 	schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 308950f..850de01 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -228,12 +228,6 @@ enum gdma_page_type {
 
 #define GDMA_INVALID_DMA_REGION 0
 
-struct mana_serv_work {
-	struct work_struct serv_work;
-	struct pci_dev *pdev;
-	enum gdma_eqe_type type;
-};
-
 struct gdma_mem_info {
 	struct device *dev;
 
@@ -417,11 +411,6 @@ struct gdma_irq_context {
 	bool dyn_msix;
 };
 
-enum gdma_context_flags {
-	GC_PROBE_SUCCEEDED	= 0,
-	GC_IN_SERVICE		= 1,
-};
-
 struct gdma_context {
 	struct device		*dev;
 	struct dentry		*mana_pci_debugfs;
@@ -479,7 +468,21 @@ struct gdma_context {
 
 	struct workqueue_struct *service_wq;
 
-	unsigned long		flags;
+	/* The in-flight MANA service cycle, queued on the system workqueue:
+	 * a reset cycle destroys and re-creates @service_wq.
+	 */
+	struct work_struct	serv_work;
+	enum gdma_eqe_type	serv_type;
+
+	/* Service-cycle admission/retirement; taken irqsave (the EQ
+	 * event path is hard IRQ) and never held across a sleep.
+	 */
+	spinlock_t		serv_lock;
+	wait_queue_head_t	serv_waitq;
+	bool			serv_in_flight;
+	bool			serv_removing;
+	bool			serv_during_probe;
+	bool			serv_probe_done;
 
 	/* Protect access to GIC context */
 	struct mutex		gic_mutex;
@@ -528,6 +531,8 @@ ssize_t mana_gd_read_ring(struct gdma_queue *q, char __user *buf,
 			  size_t count, loff_t *pos);
 
 int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type);
+bool mana_service_active(struct gdma_context *gc);
+bool mana_service_probe_done(struct gdma_context *gc);
 
 void mana_gd_ring_dim(struct gdma_queue *cq, u32 mod_usec, bool mod_usec_vld,
 		      u32 mod_comps, bool mod_comps_vld);

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mana_gf_stats_work_handler()
  (drivers/net/ethernet/microsoft/mana/mana_en.c:4056-4072) now drops…
- [Low] The comment added at the new service_quiesce label in
  mana_gd_probe() ('The stats work can admit service once mana_probe()…
- [Low] mana_schedule_serv_work() replaces schedule_work() with
  queue_work(system_wq, &gc->serv_work).

Pre-existing issues:
- [High] The new admission/quiesce protocol is applied to
  mana_gd_remove(), mana_gd_shutdown() and the probe unwind, but not to…

--

net: mana: fix reset work race with device removal

The service work item is moved into struct gdma_context and its lifecycle
is guarded by a new serv_lock plus a waitqueue, so removal, shutdown and
the probe unwind can close admission and wait for an in-flight reset cycle
before gc is freed.  The probe boundary is handled by latching reset events
that arrive while the probe is still running.

A few questions below about the new admission gates and the paths that were
not converted.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 8e9bfc1d6a2ae..b91d2542cfe56 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -796,51 +899,94 @@ static void mana_recovery_delayed_func(struct work_struct *w)

[ ... ]

> +/* Serialize admission with retirement.  Called in hard IRQ context, so
> + * serv_lock is taken with interrupts disabled and nothing that may
> + * sleep runs under it.
> + */
>  int mana_schedule_serv_work(struct gdma_context *gc, enum gdma_eqe_type type)
>  {
> -	struct mana_serv_work *mns_wk;
> +	unsigned long flags;
> +	bool busy;

[ ... ]

>  	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]
Is system_wq the intended target here?  include/linux/workqueue.h carries:

    extern struct workqueue_struct *system_wq; /* use system_percpu_wq, this will be removed */

and system_wq is created with __WQ_DEPRECATED, so __queue_work() emits:

    workqueue: work func %ps enqueued on deprecated workqueue. Use system_{percpu|dfl}_wq instead.

The schedule_work() that was removed expanded to queue_work(system_percpu_wq,
...), so this hunk seems to newly pick the deprecated alias and a warning on
every service cycle.

Given that mana_serv_reset()/mana_serv_fpga() do msleep(MANA_SERVICE_PERIOD *
1000) and PCI stop/rescan, would system_long_wq (or system_dfl_long_wq) be a
better fit than the per-CPU queue?

[ ... ]

> @@ -2558,22 +2697,26 @@ static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	err = mana_rdma_probe(&gc->mana_ib);
>  	if (err)
> -		goto cleanup_mana;
> +		goto service_quiesce;
>  
>  	/*
>  	 * If a hardware reset event has occurred over HWC during probe,
>  	 * rollback and perform hardware reset procedure.
>  	 */
> -	if (test_and_set_bit(GC_PROBE_SUCCEEDED, &gc->flags)) {
> +	rollback = mana_service_probe_complete(gc);
> +	if (rollback) {
>  		err = -EPROTO;
> -		goto cleanup_mana_rdma;
> +		goto service_quiesce;
>  	}
>  
>  	return 0;
>  
> -cleanup_mana_rdma:
> +service_quiesce:
> +	/* The stats work can admit service once mana_probe() has run:
> +	 * retire an in-flight cycle before any teardown.
> +	 */
> +	mana_service_quiesce(gc);

[Severity: Low]
Does this comment still describe the code after the change in mana_en.c?

The stats work handler now only admits service when
mana_service_probe_done(gc) is true, and serv_probe_done is set solely by
mana_service_probe_complete(), which runs after mana_rdma_probe() has
succeeded.  On the path that jumps here because mana_rdma_probe() failed,
serv_probe_done is still false, so the stats work cannot have admitted a
cycle at all, and the EQ producer is gated on the same flag.

Could the comment be reworded to match the gate, or is a producer intended
that can still admit during probe?

[ ... ]

> @@ -2687,6 +2837,9 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
>  
>  	dev_info(&pdev->dev, "Shutdown was called\n");
>  
> +	/* Shutdown tears down the same HW paths as remove(). */
> +	mana_service_quiesce(gc);
> +
>  	mana_rdma_remove(&gc->mana_ib);
>  	mana_remove(&gc->mana, true);

[Severity: High]
This isn't a bug introduced by this patch, since the old GC_IN_SERVICE bit
was ignored by the PM callbacks too, but should mana_gd_suspend() and
mana_gd_resume() (installed as mana_driver.suspend/.resume) get the same
treatment as remove() and shutdown()?

An admitted cycle runs:

mana_serv_reset()
    mana_gd_suspend(pdev, PMSG_SUSPEND);
    msleep(MANA_SERVICE_PERIOD * 1000);
    ret = mana_gd_resume(pdev);

with serv_in_flight set for that whole 10+ second window, and the work sits
on system_wq, which is not WQ_FREEZABLE.  If the guest enters system suspend
or hibernate during that window, pci_pm_suspend() -> pci_legacy_suspend()
calls mana_gd_suspend() a second time on the same gc, and neither PM callback
consults serv_in_flight/serv_removing or calls mana_service_quiesce().

Both threads then reach mana_hwc_destroy_channel(), whose guard is not atomic
with the clearing store:

drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_destroy_channel() {
	struct hw_channel_context *hwc = gc->hwc.driver_data;

	if (!hwc)
		return;
	...
	kfree(hwc);
	gc->hwc.driver_data = NULL;
	gc->hwc.gdma_context = NULL;

	vfree(gc->cq_table);
	gc->cq_table = NULL;
}

mana_gd_cleanup_device() has the same shape around gc->service_wq:

	if (gc->service_wq) {
		destroy_workqueue(gc->service_wq);
		gc->service_wq = NULL;
	}

Can this interleaving double-free hwc and gc->cq_table, or call
destroy_workqueue() twice on gc->service_wq?  And can mana_gd_resume() from
the service cycle re-arm DMA/MMIO on a device the PM core has already
suspended?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 591fb4191d90d..0eb23aea2e139 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -4060,10 +4060,13 @@ static void mana_gf_stats_work_handler(struct work_struct *work)
>  		memset(&ac->hc_stats, 0, sizeof(ac->hc_stats));
>  		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.
> +		/* As HWC timed out, indicating a faulty HW state and
> +		 * needs a 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 (mana_service_probe_done(gc))
> +			mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST);
>  		return;
>  	}
>  	schedule_delayed_work(&ac->gf_stats_work, MANA_GF_STATS_PERIOD);

[Severity: Medium]
When mana_service_probe_done(gc) is false here, is the reset request lost for
good?

mana_probe() arms this work with a MANA_GF_STATS_PERIOD delay, while
serv_probe_done is only set later, at the tail of mana_gd_probe():

	err = mana_rdma_probe(&gc->mana_ib);
	if (err)
		goto service_quiesce;
	...
	rollback = mana_service_probe_complete(gc);

So the handler can run with serv_probe_done still false if mana_rdma_probe()
(register device plus the synchronous mana_ib adev probe and its HWC
requests) takes longer than that delay.

In that window an -ETIMEDOUT from mana_query_gf_stats() takes the new branch,
no cycle is admitted, serv_during_probe is not set so
mana_service_probe_complete() reports no rollback, and the return also skips
the schedule_delayed_work() re-arm.  If the probe then succeeds, is there any
remaining path that retries?  The recovery list in mana_gd_probe() is only
populated when the probe itself fails with -ETIMEDOUT/-EPROTO, and
hwc_timeout_occurred only feeds a warning, so hc_stats would stay zeroed for
the life of the binding and a wedged HWC would never be reset.  Before this
patch the request was unconditional and the resulting reset cycle re-armed
the stats work through mana_probe(resuming=true).

The EQ producer solves the same race by latching under serv_lock:

	admit = gc->serv_probe_done;
	first = !gc->serv_during_probe;
	if (!admit)
		gc->serv_during_probe = true;

Would routing this producer through the same latch-or-admit helper work, or
alternatively keeping the delayed work re-armed so the request is retried
once serv_probe_done is set?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922070728.309280-1-fanwu01%40zju.edu.cn