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

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

[Fan Wu]

The reset service work runs on the system workqueue and obtains the
GDMA context through PCI drvdata.  It can race with device removal
(mana_gd_remove()), which frees the context.  A reset work that runs
after removal can therefore dereference the freed context.

Serialize mana_serv_reset() with device removal by taking the PCI
device lock across its drvdata access and reset sequence.  The driver
core holds the same lock while invoking the remove callback, so remove
waits for an in-progress reset.  Work that runs after remove observes
the drvdata cleared before the context is freed.

Drop the lock before rescanning, since the rescan path may remove the
device and acquire the device lock again.  Also clear GC_IN_SERVICE
before rescanning after a failed resume, so this exit follows the same
service-state cleanup as the other reset exits.

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
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/net/ethernet/microsoft/mana/gdma_main.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index e8b7ffb47eb9..ae03d7a53 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -591,15 +591,16 @@ static void mana_serv_fpga(struct pci_dev *pdev)

 static void mana_serv_reset(struct pci_dev *pdev)
 {
-	struct gdma_context *gc = pci_get_drvdata(pdev);
+	struct gdma_context *gc;
 	struct hw_channel_context *hwc;
 	int ret;

+	device_lock(&pdev->dev);
+	gc = pci_get_drvdata(pdev);
 	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);
-		return;
+		goto rescan;
 	}

 	hwc = gc->hwc.driver_data;
@@ -621,8 +622,8 @@ 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);
-		return;
+		clear_bit(GC_IN_SERVICE, &gc->flags);
+		goto rescan;
 	}

 	if (ret)
@@ -632,6 +633,12 @@ static void mana_serv_reset(struct pci_dev *pdev)

 out:
 	clear_bit(GC_IN_SERVICE, &gc->flags);
+	device_unlock(&pdev->dev);
+	return;
+
+rescan:
+	device_unlock(&pdev->dev);
+	mana_serv_rescan(pdev);
 }

 static void mana_do_service(enum gdma_eqe_type type, struct pci_dev *pdev)
@@ -2436,6 +2443,7 @@ static void mana_gd_remove(struct pci_dev *pdev)

 	pci_iounmap(pdev, gc->bar0_va);

+	pci_set_drvdata(pdev, NULL);
 	vfree(gc);

 	pci_release_regions(pdev);
--
2.34.1

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

[Fan Wu]

The reset service work runs on the system workqueue and obtains the
GDMA context through PCI drvdata.  It can race with device removal
(mana_gd_remove()), which frees the context.  A reset work that runs
after removal can therefore dereference the freed context.

Serialize mana_serv_reset() with device removal by taking the PCI
device lock across its drvdata access and reset sequence.  The driver
core holds the same lock while invoking the remove callback, so remove
waits for an in-progress reset.  Work that runs after remove observes
the drvdata cleared before the context is freed.

Drop the lock before rescanning, since the rescan path may remove the
device and acquire the device lock again.  Also clear GC_IN_SERVICE
before rescanning after a failed resume, so this exit follows the same
service-state cleanup as the other reset exits.

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
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/net/ethernet/microsoft/mana/gdma_main.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
index e8b7ffb47eb9..ae03d7a53 100644
--- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
+++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
@@ -591,15 +591,16 @@ static void mana_serv_fpga(struct pci_dev *pdev)

 static void mana_serv_reset(struct pci_dev *pdev)
 {
-	struct gdma_context *gc = pci_get_drvdata(pdev);
+	struct gdma_context *gc;
 	struct hw_channel_context *hwc;
 	int ret;

+	device_lock(&pdev->dev);
+	gc = pci_get_drvdata(pdev);
 	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);
-		return;
+		goto rescan;
 	}

 	hwc = gc->hwc.driver_data;
@@ -621,8 +622,8 @@ 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);
-		return;
+		clear_bit(GC_IN_SERVICE, &gc->flags);
+		goto rescan;
 	}

 	if (ret)
@@ -632,6 +633,12 @@ static void mana_serv_reset(struct pci_dev *pdev)

 out:
 	clear_bit(GC_IN_SERVICE, &gc->flags);
+	device_unlock(&pdev->dev);
+	return;
+
+rescan:
+	device_unlock(&pdev->dev);
+	mana_serv_rescan(pdev);
 }

 static void mana_do_service(enum gdma_eqe_type type, struct pci_dev *pdev)
@@ -2436,6 +2443,7 @@ static void mana_gd_remove(struct pci_dev *pdev)

 	pci_iounmap(pdev, gc->bar0_va);

+	pci_set_drvdata(pdev, NULL);
 	vfree(gc);

 	pci_release_regions(pdev);
--
2.34.1

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

[Haiyang Zhang]

> -----Original Message-----
> From: Fan Wu <fanwu01@zju.edu.cn>
> Sent: Wednesday, August 5, 2026 10:38 AM
> To: netdev@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Wei Liu <wei.liu@kernel.org>; Dexuan Cui
> <DECUI@microsoft.com>; Long Li <longli@microsoft.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S . Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; linux-hyperv@vger.kernel.org; linux-
> kernel@vger.kernel.org; stable@vger.kernel.org
> Subject: [EXTERNAL] [PATCH net] net: mana: fix reset work race with device
> removal
> 
> [Some people who received this message don't often get email from
> fanwu01@zju.edu.cn. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> The reset service work runs on the system workqueue and obtains the
> GDMA context through PCI drvdata.  It can race with device removal
> (mana_gd_remove()), which frees the context.  A reset work that runs
> after removal can therefore dereference the freed context.
> 
> Serialize mana_serv_reset() with device removal by taking the PCI
> device lock across its drvdata access and reset sequence.  The driver
> core holds the same lock while invoking the remove callback, so remove
> waits for an in-progress reset.  Work that runs after remove observes
> the drvdata cleared before the context is freed.
> 
> Drop the lock before rescanning, since the rescan path may remove the
> device and acquire the device lock again.  Also clear GC_IN_SERVICE
> before rescanning after a failed resume, so this exit follows the same
> service-state cleanup as the other reset exits.
> 
> 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
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/net/ethernet/microsoft/mana/gdma_main.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)

Adding some of my teammates: @Dipayaan Roy @Sahil Chandna

- Haiyang

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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805143812.220509-1-fanwu01@zju.edu.cn?part=1
---
--- Patch [1]: [PATCH net] net: mana: fix reset work race with device removal ---
commit 789cdee15635f51361a37bb4e10ca06127d69f71
Author: Fan Wu <fanwu01@zju.edu.cn>

net: mana: fix reset work race with device removal

The patch makes mana_serv_reset() take the driver-core device lock around
the pci_get_drvdata() read and the whole reset sequence, drops the lock
before calling mana_serv_rescan(), and has mana_gd_remove() clear the PCI
drvdata before vfree()ing the gdma_context.  It also clears GC_IN_SERVICE
on the resume-failure exit before rescanning.

A couple of questions about the new locking and the new clear_bit().

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index e8b7ffb47eb98..ae03d7a53348e 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -591,15 +591,16 @@ static void mana_serv_fpga(struct pci_dev *pdev)
>  
>  static void mana_serv_reset(struct pci_dev *pdev)
>  {
> -	struct gdma_context *gc = pci_get_drvdata(pdev);
> +	struct gdma_context *gc;
>  	struct hw_channel_context *hwc;
>  	int ret;
>  
> +	device_lock(&pdev->dev);
> +	gc = pci_get_drvdata(pdev);

[Severity: High]
[Finding: fable-5-5-8]
[Sources: fable-5, opus-5]
Is it intended that &pdev->dev's mutex is now held across the entire reset
cycle rather than just the drvdata read?  With the new labels the lock is
only dropped at out: and rescan:, so it covers this part of
mana_serv_reset():

	mana_gd_suspend(pdev, PMSG_SUSPEND);

	msleep(MANA_SERVICE_PERIOD * 1000);

	ret = mana_gd_resume(pdev);

That is an unconditional 10 second msleep(), plus mana_gd_resume(), which
re-runs mana_gd_setup()/mana_probe()/mana_rdma_probe(); the first HWC
command there can wait up to HW_CHANNEL_WAIT_RESOURCE_TIMEOUT_MS (30000
ms) when the adapter is unresponsive.

This runs from a system_wq work item (mana_serv_func() -> mana_do_service()
-> mana_serv_reset()), so everything that serializes on the same device
mutex blocks for that whole time:

  device_release_driver_internal() -> __device_driver_lock()  (sysfs
    unbind, driver removal)
  device_shutdown()  (reboot/shutdown, takes device_lock(parent) plus
    device_lock(dev))
  drivers/base/power/main.c device_suspend()/device_resume(), which arm
    the DPM watchdog before taking device_lock(dev)
  pci_stop_and_remove_bus_device() -> pci_stop_dev() ->
    device_release_driver(), which blocks on this mutex while holding the
    global pci_rescan_remove_lock

Can this stall system suspend/resume, reboot and all PCI hotplug/rescan on
the machine for tens of seconds?  And with CONFIG_DPM_WATCHDOG set below
the hold time, would the PM core's watchdog fire because it is armed before
device_lock()?

The trigger is not driver-internal: mana_schedule_serv_work() is called
from mana_gd_process_eqe() for a device-supplied
GDMA_EQE_HWC_RESET_REQUEST, and GC_IN_SERVICE only prevents overlap, not
back-to-back requests.

Would narrowing the lock to the pci_get_drvdata() read, and getting the
lifetime guarantee from a driver-private mutex, a refcounted
gdma_context, or cancelling the service work in mana_gd_remove(), be
enough for the stated goal?

There is also a structural concern with the wide hold: under this lock the
code reaches mana_rdma_remove() (flush_workqueue(gc->service_wq)),
mana_gd_cleanup_device() (destroy_workqueue(gc->service_wq)) and
mana_remove() (disable_work_sync(&apc->queue_reset_work),
cancel_delayed_work_sync(&ac->gf_stats_work), rtnl_lock()).  Would a later
flush or cancel of the service work from the remove path then deadlock
against this lock?

>  	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);
> -		return;
> +		goto rescan;
>  	}
>  
>  	hwc = gc->hwc.driver_data;
> @@ -621,8 +622,8 @@ 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);
> -		return;
> +		clear_bit(GC_IN_SERVICE, &gc->flags);
> +		goto rescan;
>  	}

[Severity: Medium]
[Finding: fable-5-2-2]
[Sources: fable-5, opus-5]
Does clearing GC_IN_SERVICE before the rescan change the service gating
semantics on this exit?  Previously this path returned with the bit still
set, and it only stopped mattering because mana_serv_rescan() removed the
device and vfree()d gc (a freshly probed device starts with flags == 0).

GC_IN_SERVICE looks like the only "a service cycle is in flight" gate:

  mana_schedule_serv_work():
	if (test_and_set_bit(GC_IN_SERVICE, &gc->flags)) {
		dev_info(gc->dev, "Already in service\n");
		return -EBUSY;
	}

  mana_tx_timeout() in mana_en.c:
	/* Already in service, hence tx queue reset is not required.*/
	if (test_bit(GC_IN_SERVICE, &gc->flags))
		return;

With the bit cleared first, the flag reads "not in service" for the whole
teardown window: device_unlock() happens, then mana_serv_rescan() can block
for a long time on pci_lock_rescan_remove() and again on device_lock inside
pci_stop_and_remove_bus_device() -> pci_stop_dev() ->
device_release_driver().

Is that window reachable with the HWC still alive?  mana_gd_resume()
returns -ETIMEDOUT when mana_rdma_probe() -> mana_gd_register_device()
times out on HWC, and in that case only mana_rdma_remove() has been undone
- the HWC channel, its EQ and the MSI-X IRQs are still up and the netdevs
are still registered.  In that state two triggers can queue a second
service work:

  mana_gd_process_eqe()          (incoming GDMA_EQE_HWC_RESET_REQUEST)
      -> mana_schedule_serv_work()

  mana_gf_stats_work_handler()   (re-arms every 2 s, on -ETIMEDOUT)
      -> mana_schedule_serv_work(gc, GDMA_EQE_HWC_RESET_REQUEST)

Can that second work then win the unfair device_lock ahead of the in-flight
removal, observe a still non-NULL gc, and run another full
mana_gd_suspend() + msleep(10 s) + mana_gd_resume() cycle while the first
work is stuck inside pci_stop_and_remove_bus_device() holding
pci_rescan_remove_lock?

Separately, with the bit cleared, is mana_tx_timeout() still suppressed
during teardown, or can it now queue apc->queue_reset_work and drive HWC
and queue programming against an adapter that just requested a reset and is
being removed?

The commit message describes this hunk only as:

    Also clear GC_IN_SERVICE before rescanning after a failed resume, so
    this exit follows the same service-state cleanup as the other reset
    exits.

Could the changelog also mention the change in service-gating behaviour,
given this is tagged for stable?

>  
>  	if (ret)
> @@ -632,6 +633,12 @@ static void mana_serv_reset(struct pci_dev *pdev)
>  
>  out:
>  	clear_bit(GC_IN_SERVICE, &gc->flags);
> +	device_unlock(&pdev->dev);
> +	return;
> +
> +rescan:
> +	device_unlock(&pdev->dev);
> +	mana_serv_rescan(pdev);
>  }

[ ... ]