Re: [PATCH v8 10/10] s390/vfio_ccw: implement a crw lock

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing cancellation of `notoper_work` during device release leads to Use-After-Free.

Pre-existing issues:
- [Critical] Lockless modification of eventfd triggers in `vfio_ccw_mdev_set_irqs` leads to Use-After-Free of `eventfd_ctx` in asynchronous workers.
- [High] Race condition between asynchronous CSS callbacks and mdev removal leads to Use-After-Free of the `private` structure.
--

--- Patch [10]: [PATCH v8 10/10] s390/vfio_ccw: implement a crw lock ---
commit e956337911477e5c5887e514827c14569954a32d
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a crw lock

This patch adds a lock to the vfio_ccw driver to manage the list of
asynchronous CRW events. It serializes access to the CRW payload list
to ensure cohesive reads between hardware events and userspace.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 757ff5b2556e3..ab6b518cc353f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> [ ... ]
> @@ -118,11 +118,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
> +	unsigned long flags;
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem with how
crw_trigger is accessed here.

Does private->crw_lock actually protect private->crw_trigger from
concurrent modification?

Looking at vfio_ccw_mdev_set_irqs(), it replaces and frees eventfd contexts
without holding any locks:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
    ...
    if (*ctx)
        eventfd_ctx_put(*ctx);

    *ctx = efdctx;
}

If the VFIO_DEVICE_SET_IRQS ioctl swaps and frees the eventfd_ctx while
this asynchronous worker is preparing to signal it, won't eventfd_signal()
dereference a freed pointer?

> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  }
> [ ... ]
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 1cca3ecdae45b..4a0fe2f8fdb44 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> [ ... ]
> @@ -131,6 +132,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	struct vfio_ccw_private *private =
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  	struct vfio_ccw_crw *crw, *temp;
> +	unsigned long flags;
>  
>  	/*
>  	 * Ensure these work items are fully drained, so none can
>  	 * fire after being released. The notoper_work struct is
>  	 * only meaningful if the device had been opened, which
>  	 * means it would have been cleaned in an earlier close.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);

[Severity: High]
The comment above suggests notoper_work is only meaningful if the device
had been opened, but could a race occur here?

If the device was never opened (state is VFIO_CCW_STATE_STANDBY), can an
asynchronous CSS callback still trigger a VFIO_CCW_EVENT_NOT_OPER event
(e.g., if the subchannel is unregistered)?

If this happens and queues notoper_work, and the device is subsequently
released, won't release_dev free the private structure while notoper_work
is still queued?

When the workqueue executes:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_notoper_todo() {
    ...
    mutex_lock(&private->io_mutex);
    cp_free(&private->cp);
    mutex_unlock(&private->io_mutex);
}

Won't this dereference the freed private pointer? Should we add
cancel_work_sync(&private->notoper_work) here?


[Severity: High]
This is a pre-existing issue, but since we are looking at the lifecycle of
the private structure, is there a race condition with asynchronous CSS
callbacks during mdev removal?

In vfio_ccw_sch_event(), the private pointer is fetched locklessly:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
    struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    unsigned long flags;
    ...
    spin_lock_irqsave(&sch->lock, flags);
}

If vfio_ccw_mdev_remove() runs concurrently, it clears the drvdata
locklessly and drops the device reference:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_remove() {
    vfio_unregister_group_dev(&private->vdev);
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
}

If the CSS callback is preempted after fetching the pointer but before
acquiring sch->lock, and resumes after the removal path completes, won't
it use the freed private structure?

> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
>  	}
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
> [ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728013509.1551753-1-farman@linux.ibm.com?part=10