vdpa/octeon_ep: Fix race condition in device deletion

PATCH In review Yuho Choi 2 participants 3 messages Sep 14 – Sep 15

Subsystems (3)

Stable branch virtio core Marvell octeon endpoint virtio data path accelerator

lore

Share & tools

lore

Yuho Choi

author

2026-09-14 04:03 UTC #0 lore raw

PATCH 1/1 vdpa/octeon_ep: Fix race condition in device deletion

When firmware delivers OCTEP_VDPA_DEV_DEL_EVENT, octep_event_work() checks if mgmt_dev->status is OCTEP_VDPA_DEV_STATUS_ADDED, but does not claim ownership before calling octep_vdpa_dev_del(). Meanwhile, userspace management deletion via octep_vdpa_dev_del() unconditionally calls _vdpa_unregister_device() and marks status as REMOVED.

If a queued hardware DEL event checks the ADDED state and the management path subsequently deletes the same oct_vdpa, the worker proceeds to call octep_vdpa_dev_del() on the already unregistered and freed device. This results in _vdpa_unregister_device() being invoked twice on the same object, triggering a use-after-free or double release.

Fix this race by:
1. Atomically claiming deletion ownership in octep_vdpa_dev_del() using atomic_cmpxchg() from ADDED to REMOVED, and returning early if the device was already claimed for deletion.
2. Clearing mgmt_dev->oct_vdpa upon unregistration and on dev_add failure so dangling pointers are not referenced.
3. Reading mgmt_dev->oct_vdpa safely in octep_event_work() before invoking octep_vdpa_dev_del().
4. Canceling event work and ensuring child devices are unregistered before marking the management device UNINIT in octep_vdpa_remove_vf().

Fixes a5786561649a ("vdpa/octeon_ep: Add vDPA device event handling for firmware notifications")

Cc [email protected]

Signed-off-by Yuho Choi <[email protected]>

---
 drivers/vdpa/octeon_ep/octep_vdpa_main.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

drivers/vdpa/octeon_ep/octep_vdpa_main.c +16 -6

index 85a3d35ea1e4..e29647bcf196 100644
@@ -523,14 +523,16 @@ static void octep_vdpa_remove_vf(struct pci_dev *pdev)
 	int status;
 
 	oct_hw = &mgmt_dev->oct_hw;
-	status = atomic_read(&mgmt_dev->status);
-	atomic_set(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_UNINIT);
-
 	cancel_work_sync(&mgmt_dev->setup_task);
+	cancel_work_sync(&mgmt_dev->event_wk.work);
+
+	status = atomic_read(&mgmt_dev->status);
 	if ((status == OCTEP_VDPA_DEV_STATUS_READY) || (status == OCTEP_VDPA_DEV_STATUS_ADDED) ||
 	    (status == OCTEP_VDPA_DEV_STATUS_REMOVED))
 		vdpa_mgmtdev_unregister(&mgmt_dev->mdev);
 
+	atomic_set(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_UNINIT);
+
 	if (oct_hw->base[OCTEP_HW_CAPS_BAR])
 		octep_iounmap_region(pdev, oct_hw->base, OCTEP_HW_CAPS_BAR);

@@ -612,6 +614,7 @@ static int octep_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
 	return 0;
 
 vdpa_dev_put:
+	mgmt_dev->oct_vdpa = NULL;
 	put_device(&oct_vdpa->vdpa.dev);
 	return ret;
 }

@@ -619,8 +622,13 @@ static int octep_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
 static void octep_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *vdpa_dev)
 {
 	struct octep_vdpa_mgmt_dev *mgmt_dev = container_of(mdev, struct octep_vdpa_mgmt_dev, mdev);
+
+	if (atomic_cmpxchg(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_ADDED,
+			   OCTEP_VDPA_DEV_STATUS_REMOVED) != OCTEP_VDPA_DEV_STATUS_ADDED)
+		return;
+
 	_vdpa_unregister_device(vdpa_dev);
-	atomic_set(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_REMOVED);
+	mgmt_dev->oct_vdpa = NULL;
 }
 
 static const struct vdpa_mgmtdev_ops octep_vdpa_mgmt_dev_ops = {

@@ -653,6 +661,7 @@ static void octep_event_work(struct work_struct *work)
 	u8 event = readb(addr + OCTEP_VF_EVENT_REG(0));
 	struct vdpa_dev_set_config config = {0};
 	char name[OCTEP_VDPA_NAME_BUFSIZE];
+	struct octep_vdpa *oct_vdpa;
 	int ret = 0;
 
 	switch (event) {

@@ -663,8 +672,9 @@ static void octep_event_work(struct work_struct *work)
 		}
 		break;
 	case OCTEP_VDPA_DEV_DEL_EVENT:
-		if (atomic_read(&mgmt_dev->status) == OCTEP_VDPA_DEV_STATUS_ADDED)
-			octep_vdpa_dev_del(&mgmt_dev->mdev, &mgmt_dev->oct_vdpa->vdpa);
+		oct_vdpa = READ_ONCE(mgmt_dev->oct_vdpa);
+		if (atomic_read(&mgmt_dev->status) == OCTEP_VDPA_DEV_STATUS_ADDED && oct_vdpa)
+			octep_vdpa_dev_del(&mgmt_dev->mdev, &oct_vdpa->vdpa);
 		break;
 	default:
 		break;

Signature · 1 line

2.43.0

[email protected]

AI review

2026-09-14 04:17 UTC #1 lore raw

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

New issues:
- [High] Incorrect teardown ordering in octep_vdpa_remove_vf allows hardware interrupts to re-queue the event worker after it has been cancelled, leading to a Use-After-Free of mgmt_dev.
- [High] The lockless atomic_cmpxchg synchronization introduced in octep_vdpa_dev_del is vulnerable to an ABA problem, allowing a Use-After-Free of a stale vdpa_device pointer.
- [High] Early return in octep_vdpa_dev_del silently skips unregistering devices if multiple vDPA devices are mistakenly attached, leading to a permanent device leak.
- [Medium] Calling cancel_work_sync() on a potentially uninitialized work struct triggers a kernel warning (WARN_ON(!work->func)).

Pre-existing issues:
- [High] The worker thread octep_event_work directly invokes vDPA subsystem registration routines without holding the mandatory vdpa_dev_lock, causing lockdep assertions and data races. --

commit b72e8cd6cc3fa52be059d7ac0ebe059c31816e52
Author: Yuho Choi <[email protected]> vdpa/octeon_ep: Fix race condition in device deletion

This patch addresses a race condition in device deletion where hardware deletion events could race with userspace management deletions, triggering a double release. It introduces atomic operations to claim ownership and safely read the device pointer before unregistering.

drivers/vdpa/octeon_ep/octep_vdpa_main.c static void octep_vdpa_remove_vf(struct pci_dev *pdev) · line 527

Quoted 13 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -3

index 85a3d35ea1e47..e29647bcf196d 100644
@@ -523,14 +523,16 @@ static void octep_vdpa_remove_vf(struct pci_dev *pdev)
 	int status;
 
 	oct_hw = &mgmt_dev->oct_hw;
-	status = atomic_read(&mgmt_dev->status);
-	atomic_set(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_UNINIT);
-
 	cancel_work_sync(&mgmt_dev->setup_task);
+	cancel_work_sync(&mgmt_dev->event_wk.work);

[Severity: High]
Can hardware interrupts re-queue the event worker after this cancellation?

In octep_vdpa_remove_vf(), event_wk.work is cancelled here, but hardware interrupts are not yet masked or freed until octep_free_irqs(oct_hw) is called later in the function.

If a hardware interrupt fires in this window, the interrupt handler can execute schedule_work(&mgmt_dev->event_wk.work). After teardown finishes, devres frees mgmt_dev. The scheduled work then executes, accessing the freed memory and causing a use-after-free.

[Severity: Medium]
Will this unconditional cancellation trigger a kernel warning if the work struct is uninitialized?

The mgmt_dev structure is allocated with devm_kzalloc(), which zero-initializes event_wk.work. The initialization of this work struct happens asynchronously at the end of octep_vdpa_setup_task().

If the driver is unbound before the setup task reaches INIT_WORK(), or if the setup task encounters an error and aborts, event_wk.work remains zero-initialized. Calling cancel_work_sync() detects the NULL work->func and triggers a WARN_ON(!work->func).

Quoted 5 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +2 -0

+
+	status = atomic_read(&mgmt_dev->status);
 	if ((status == OCTEP_VDPA_DEV_STATUS_READY) || (status == OCTEP_VDPA_DEV_STATUS_ADDED) ||
 	    (status == OCTEP_VDPA_DEV_STATUS_REMOVED))
 		vdpa_mgmtdev_unregister(&mgmt_dev->mdev);

[ ... ]

drivers/vdpa/octeon_ep/octep_vdpa_main.c static int octep_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, · line 628

Quoted 8 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +4 -0

@@ -619,8 +622,13 @@ static int octep_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
 static void octep_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *vdpa_dev)
 {
 	struct octep_vdpa_mgmt_dev *mgmt_dev = container_of(mdev, struct octep_vdpa_mgmt_dev, mdev);
+
+	if (atomic_cmpxchg(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_ADDED,
+			   OCTEP_VDPA_DEV_STATUS_REMOVED) != OCTEP_VDPA_DEV_STATUS_ADDED)
+		return;

[Severity: High]
Does this early return silently skip unregistering devices if multiple vDPA devices are mistakenly attached?

octep_vdpa_dev_add() allows userspace to repeatedly create vDPA devices on the same mgmt_dev, blindly overwriting mgmt_dev->oct_vdpa and setting status = ADDED.

When userspace deletes the first device, octep_vdpa_dev_del() executes, the cmpxchg succeeds, and the device is unregistered. When userspace attempts to delete the second device, the cmpxchg from ADDED to REMOVED fails. The function returns early without unregistering the second device, permanently leaking it.

drivers/vdpa/octeon_ep/octep_vdpa_main.c static int octep_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, · line 630

Quoted 2 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -0
+
 	_vdpa_unregister_device(vdpa_dev);

[Severity: High]
This is a pre-existing issue, but does calling _vdpa_unregister_device() from the asynchronous workqueue violate the vDPA bus locking requirements?

octep_event_work() responds to hardware events by invoking octep_vdpa_dev_del() directly, which in turn calls _vdpa_unregister_device(). The vDPA core strictly requires vdpa_dev_lock to be held when invoking this underscored variant.

Since the asynchronous workqueue does not acquire this lock, it unconditionally triggers lockdep splats (lockdep_assert_held(&vdpa_dev_lock)) and corrupts the vDPA bus device list when racing with concurrent netlink operations. (The same applies to _vdpa_register_device() in octep_vdpa_dev_add()).

Quoted 5 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -1

-	atomic_set(&mgmt_dev->status, OCTEP_VDPA_DEV_STATUS_REMOVED);
+	mgmt_dev->oct_vdpa = NULL;
 }
 
 static const struct vdpa_mgmtdev_ops octep_vdpa_mgmt_dev_ops = {

[ ... ]

Quoted 9 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +1 -0

@@ -653,6 +661,7 @@ static void octep_event_work(struct work_struct *work)
 	u8 event = readb(addr + OCTEP_VF_EVENT_REG(0));
 	struct vdpa_dev_set_config config = {0};
 	char name[OCTEP_VDPA_NAME_BUFSIZE];
+	struct octep_vdpa *oct_vdpa;
 	int ret = 0;
 
 	switch (event) {

drivers/vdpa/octeon_ep/octep_vdpa_main.c static void octep_event_work(struct work_struct *work) · line 677

Quoted 8 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +3 -2

@@ -663,8 +672,9 @@ static void octep_event_work(struct work_struct *work)
 		}
 		break;
 	case OCTEP_VDPA_DEV_DEL_EVENT:
-		if (atomic_read(&mgmt_dev->status) == OCTEP_VDPA_DEV_STATUS_ADDED)
-			octep_vdpa_dev_del(&mgmt_dev->mdev, &mgmt_dev->oct_vdpa->vdpa);
+		oct_vdpa = READ_ONCE(mgmt_dev->oct_vdpa);
+		if (atomic_read(&mgmt_dev->status) == OCTEP_VDPA_DEV_STATUS_ADDED && oct_vdpa)
+			octep_vdpa_dev_del(&mgmt_dev->mdev, &oct_vdpa->vdpa);

[Severity: High]
Is this lockless synchronization vulnerable to an ABA problem that allows a use-after-free?

In octep_event_work(), mgmt_dev->oct_vdpa is cached locally. If the thread is preempted:

1. A concurrent netlink command calls octep_vdpa_dev_del(), successfully setting status to REMOVED and unregistering the device.

2. A subsequent netlink command adds a new device, allocating a new object and setting status to ADDED.

3. When octep_event_work() resumes, it sees status == ADDED and calls octep_vdpa_dev_del() with the STALE pointer.

The atomic_cmpxchg() in octep_vdpa_dev_del() succeeds because the status is ADDED (for the new device), but it blindly unregisters the stale device passed to it.

Quoted 3 lines

drivers/vdpa/octeon_ep/octep_vdpa_main.c +0 -0
 		break;
 	default:
 		break;

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Yuho Choi

author

2026-09-15 15:48 UTC #2 lore raw

Quoted 8 lines · On Mon, 14 Sept 2026 at 00:17, <[email protected]> wrote:

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] Incorrect teardown ordering in octep_vdpa_remove_vf allows hardware interrupts to re-queue the event worker after it has been cancelled, leading to a Use-After-Free of mgmt_dev.
- [High] The lockless atomic_cmpxchg synchronization introduced in octep_vdpa_dev_del is vulnerable to an ABA problem, allowing a Use-After-Free of a stale vdpa_device pointer.
- [High] Early return in octep_vdpa_dev_del silently skips unregistering devices if multiple vDPA devices are mistakenly attached, leading to a permanent device leak.
- [Medium] Calling cancel_work_sync() on a potentially uninitialized work struct triggers a kernel warning (WARN_ON(!work->func)).

Thanks for the review.
Please drop this version.
I will rework it and send v2 based on the review.

Please also drop the Cc: stable -- it was premature on v1.

Thanks,
Yuho