[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Clearing GC_IN_SERVICE before a PCI rescan allows a redundant 10-second reset cycle to race with device teardown.
--

--- Patch [1]: [PATCH net] net: mana: fix reset work race with device removal ---
commit 09d0d942fe66b8d000decbfd0fc00c78cb2cea3c
Author: Fan Wu <fanwu01@zju.edu.cn>

net: mana: fix reset work race with device removal

This commit serializes the reset service work with device removal by holding
the PCI device lock. It ensures the GDMA context is not freed while a reset
is in progress. The lock is dropped before rescanning the PCI bus or when the
reset completes.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
[ ... ]
> @@ -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]
If mana_gd_resume() fails, the driver clears GC_IN_SERVICE and releases the
device lock before calling pci_stop_and_remove_bus_device() via
mana_serv_rescan().

If the hardware repeatedly asserts errors and issues another reset request
during this window, mana_schedule_serv_work() could queue a new reset work.

That new work would immediately grab the device lock, observe a non-NULL gc,
and sleep for 10 seconds. This could block the PCI core from completing the
device teardown, as it requires the device lock to invoke the remove callback.

Does clearing GC_IN_SERVICE here before the PCI rescan open a race window
that delays device teardown?

>  	}
>  
>  	if (ret)
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805141937.217114-1-fanwu01@zju.edu.cn?part=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: [PATCH net] net: mana: fix reset work race with device removal

[sashiko-bot]

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

New issues:
- [High] The patch introduces a 10-second sleep while holding the device core lock (`device_lock`), which will block system PM and driver core operations.

Pre-existing issues:
- [High] Driver unbind unexpectedly triggers a full PCI bus rescan due to the `!gc` check.
--

--- Patch [1]: [PATCH net] net: mana: fix reset work race with device removal ---
commit 839c641df5112c8f696d580233f5a31b4e6686dd
Author: Fan Wu <fanwu01@zju.edu.cn>

net: mana: fix reset work race with device removal

The reset service work runs on the system workqueue and obtains the
GDMA context through PCI drvdata. It can race with device removal,
which frees the context, so this patch serializes mana_serv_reset()
with device removal by taking the PCI device lock.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> --- 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);

[Severity: High]
Does holding device_lock() here cause a regression by stalling system PM
and driver core operations?

mana_serv_reset() calls msleep(MANA_SERVICE_PERIOD * 1000) (which is a 10
second sleep) further down in this function:

mana_serv_reset() {
        ...
        mana_gd_suspend(pdev, PMSG_SUSPEND);
        msleep(MANA_SERVICE_PERIOD * 1000);
        ...
}

Because system suspend loops over all devices and sequentially acquires
device_lock, would a suspend attempt or a user-space unbind operation
unconditionally hang for up to 10 seconds while waiting for this lock?

> +	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;

[Severity: High]
This is a pre-existing issue, but if mana_gd_remove() is called due to a
standard driver unbind, it clears drvdata. If a pending reset work then
executes, acquires the lock, and observes gc == NULL, it jumps to the rescan
label which forcefully removes the device and triggers a full PCI bus rescan.

Will this effectively defeat the user's attempt to unbind the driver and
disrupt the PCI topology?

>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805143812.220509-1-fanwu01@zju.edu.cn?part=1

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);
>  }

[ ... ]