Re: [PATCH v7 07/10] s390/vfio_ccw: cancel existing workqueues

[sashiko-bot@kernel.org]

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

New issues:
- [High] Suboptimal ordering of `cancel_work_sync()` and `private->crw` list teardown introduces a memory leak.

Pre-existing issues:
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) race condition leads to Use-After-Free during device removal.
- [High] Lockless modification and traversal of the shared `private->crw` list leads to data races and list corruption.
- [High] Eventfd contexts for device triggers are leaked when the device is closed or released.
--

--- Patch [7]: [PATCH v7 07/10] s390/vfio_ccw: cancel existing workqueues ---
commit e5fd510ce3193c957c167ef6f613d80a32133fe5
Author: Eric Farman <farman@linux.ibm.com>
s390/vfio_ccw: cancel existing workqueues

The patch adds cleanup tags to un-initialize the io_work and crw_work
workqueues in the release and close paths. This ensures work isn't dispatched
after the private structure is freed.

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..bd488e40e1531 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -136,6 +136,10 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  		kfree(crw);
>  	}
>  
> +	/* Should be empty, but just in case */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);

[Severity: High]
Does this order of operations leave room for a memory leak?

If a concurrent channel path event triggers vfio_ccw_queue_crw() and adds a
new crw element to private->crw right after the teardown loop finishes draining
the list, but before cancel_work_sync() executes, those newly added elements
will be left on the list permanently.

Would reversing the order, so cancel_work_sync() happens first, guarantee
that no concurrently queued background processing remains before the list
drain is final?

[Severity: High]
This is a pre-existing issue, but is there a data race when modifying
the private->crw list?

It looks like elements are added locklessly in vfio_ccw_queue_crw():

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
    ...
    list_add_tail(&crw->next, &private->crw);
    ...
}

And unlinked concurrently in vfio_ccw_crw_region_read():

drivers/s390/cio/vfio_ccw_chp.c:vfio_ccw_crw_region_read() {
    ...
    list_del(&crw->next);
    ...
}

Could this concurrent access without a lock lead to list corruption if a
userspace process reads the CRW region while a concurrent channel path hardware
event occurs?

[Severity: Critical]
This is a pre-existing issue, but could a use-after-free occur during
device removal?

In vfio_ccw_chp_event(), private is read while handling an event:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_chp_event() {
    ...
    private = dev_get_drvdata(&parent->dev);
    ...
    vfio_ccw_queue_crw(private, ...);
    ...
}

Concurrently, vfio_ccw_mdev_remove() unregisters the device and clears drvdata
locklessly:

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

If the event handler resumes after vfio_ccw_mdev_release_dev() frees the
private structure, will it blindly use the freed pointer to queue work?

> +
>  	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
>  	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
>  	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
[ ... ]
> @@ -202,6 +206,10 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> +
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);

[Severity: High]
This is a pre-existing issue, but are eventfd contexts being leaked here?

If userspace sets the eventfd IRQs but closes the device descriptor without
unregistering them, they appear to be left behind.

Unlike other VFIO drivers, vfio_ccw_mdev_close_device() and
vfio_ccw_mdev_release_dev() do not seem to call eventfd_ctx_put() on
io_trigger, crw_trigger, or req_trigger.

Will these contexts leak and pin inodes in memory over time?

> +
>  	vfio_ccw_unregister_dev_regions(private);
>  }

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