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. > diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c > b/drivers/vdpa/octeon_ep/octep_vdpa_main.c > index 85a3d35ea1e47..e29647bcf196d 100644 > --- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c > +++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c > @@ -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). > + > + 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); [ ... ] > @@ -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. > + > _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()). > - 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); [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. > break; > default: > break; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
