[PATCH net v2] net/mana: Fix auxiliary device double-delete race

[PATCH net v2] net/mana: Fix auxiliary device double-delete race

[Konstantin Taranov]

From: Shiraz Saleem <shirazsaleem@microsoft.com>

Make remove_adev() safe to call concurrently from the service reset
and PCI eject paths by using xchg() to atomically claim the adev
pointer. This prevents double auxiliary_device_delete/uninit when
hv_eject_device_work races with the service reset workqueue.

Fixes: 505cc26bcae0 ("net: mana: Add support for auxiliary device servicing events")
Signed-off-by: Shiraz Saleem <shirazsaleem@microsoft.com>
Signed-off-by: Konstantin Taranov <kotaranov@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
---
v2: rebased on the latest net
 drivers/net/ethernet/microsoft/mana/mana_en.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index 9017e806e..9ae5f01d8 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -3410,14 +3410,18 @@ static void adev_release(struct device *dev)
 
 static void remove_adev(struct gdma_dev *gd)
 {
-	struct auxiliary_device *adev = gd->adev;
-	int id = adev->id;
+	struct auxiliary_device *adev = xchg(&gd->adev, NULL);
+	int id;
+
+	if (!adev)
+		return;
+
+	id = adev->id;
 
 	auxiliary_device_delete(adev);
 	auxiliary_device_uninit(adev);
 
 	mana_adev_idx_free(id);
-	gd->adev = NULL;
 }
 
 static int add_adev(struct gdma_dev *gd, const char *name)
@@ -3481,7 +3485,7 @@ static void mana_rdma_service_handle(struct work_struct *work)
 
 	switch (serv_work->event) {
 	case GDMA_SERVICE_TYPE_RDMA_SUSPEND:
-		if (!gd->adev || gd->is_suspended)
+		if (gd->is_suspended)
 			break;
 
 		remove_adev(gd);
@@ -3684,8 +3688,7 @@ void mana_remove(struct gdma_dev *gd, bool suspending)
 	cancel_delayed_work_sync(&ac->gf_stats_work);
 
 	/* adev currently doesn't support suspending, always remove it */
-	if (gd->adev)
-		remove_adev(gd);
+	remove_adev(gd);
 
 	for (i = 0; i < ac->num_ports; i++) {
 		ndev = ac->ports[i];
@@ -3774,8 +3777,7 @@ void mana_rdma_remove(struct gdma_dev *gd)
 	if (gc->service_wq)
 		flush_workqueue(gc->service_wq);
 
-	if (gd->adev)
-		remove_adev(gd);
+	remove_adev(gd);
 
 	mana_gd_deregister_device(gd);
 }
-- 
2.43.0

Re: [PATCH net v2] net/mana: Fix auxiliary device double-delete race

[Jakub Kicinski]

On Tue, 17 Mar 2026 07:39:43 -0700 Konstantin Taranov wrote:
> Make remove_adev() safe to call concurrently from the service reset
> and PCI eject paths by using xchg() to atomically claim the adev
> pointer. This prevents double auxiliary_device_delete/uninit when
> hv_eject_device_work races with the service reset workqueue.

Really seems like you should add proper locking to these paths
instead. Are the accesses to is_suspended, rdma_teardown etc
really safe as is?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 9017e806e..9ae5f01d8 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -3410,14 +3410,18 @@ static void adev_release(struct device *dev)
>  
>  static void remove_adev(struct gdma_dev *gd)
>  {
> -	struct auxiliary_device *adev = gd->adev;
> -	int id = adev->id;
> +	struct auxiliary_device *adev = xchg(&gd->adev, NULL);

nit: avoid falling functions with side effects as variable init

> +	int id;
> +
> +	if (!adev)
> +		return;

RE: [EXTERNAL] Re: [PATCH net v2] net/mana: Fix auxiliary device double-delete race

[Shiraz Saleem]

> Subject: [EXTERNAL] Re: [PATCH net v2] net/mana: Fix auxiliary device double-
> delete race
> 
> On Tue, 17 Mar 2026 07:39:43 -0700 Konstantin Taranov wrote:
> > Make remove_adev() safe to call concurrently from the service reset
> > and PCI eject paths by using xchg() to atomically claim the adev
> > pointer. This prevents double auxiliary_device_delete/uninit when
> > hv_eject_device_work races with the service reset workqueue.
> 
> Really seems like you should add proper locking to these paths instead. Are the
> accesses to is_suspended, rdma_teardown etc really safe as is?

is_suspended is only accessed from mana_rdma_service_handle on the ordered service_wq - single-threaded by definition.

rdma_teardown is a one-way stop flag set in mana_rdma_remove() via WRITE_ONCE, with flush_workqueue providing ordering against the
READ_ONCE in the service handler. Concurrent writers are idempotent (both set true).

The field that actually races is gd->adev. Two remove_adev() callers on different workqueues can race - mana_serv_func on the events workqueue
vs hv_eject_device_work on PCI hot-remove - and this patch fixes it via xchg(). If we think mutex makes intent clearer, can switch.

> 
> > diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > index 9017e806e..9ae5f01d8 100644
> > --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> > +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> > @@ -3410,14 +3410,18 @@ static void adev_release(struct device *dev)
> >
> >  static void remove_adev(struct gdma_dev *gd)  {
> > -	struct auxiliary_device *adev = gd->adev;
> > -	int id = adev->id;
> > +	struct auxiliary_device *adev = xchg(&gd->adev, NULL);
> 
> nit: avoid falling functions with side effects as variable init

Sure. Can fix.
> 
> > +	int id;
> > +
> > +	if (!adev)
> > +		return;