Re: [PATCH v4 8/9] s390/vfio_ccw: implement a channel program mutex

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pending asynchronous work items are not cancelled or flushed during device release, leading to a use-after-free and locking a destroyed mutex.
- [High] Unprotected update of `private->state` in `vfio_ccw_sch_io_todo()` causes a race condition that permanently bricks the device.
--

--- Patch [8]: [PATCH v4 8/9] s390/vfio_ccw: implement a channel program mutex ---
commit dd565d50b000e987452a43655c08441c643c355d
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program mutex

This patch introduces a cp_mutex to protect the channel_program struct
from concurrent manipulation. It updates various functions to acquire
this mutex instead of relying on the cio layer's subchannel lock.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index c197ad5ab5805..4830f0dd9c3a3 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -91,6 +91,8 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>  	is_final = !(scsw_actl(&irb->scsw) &
>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
> +
> +	mutex_lock(&private->cp_mutex);
>  	if (scsw_is_solicited(&irb->scsw)) {
>  		cp_update_scsw(&private->cp, &irb->scsw);
>  		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
> @@ -98,6 +100,8 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  			cp_is_finished = true;
>  		}
>  	}
> +	mutex_unlock(&private->cp_mutex);

[Severity: High]
This is a pre-existing issue, but does this unlocked state update in
vfio_ccw_sch_io_todo() cause a race condition with concurrent close
operations?

In vfio_ccw_sch_io_todo(), private->state = VFIO_CCW_STATE_IDLE is assigned
unconditionally outside of any locks shortly after this mutex_unlock. If a
user closes the device concurrently, fsm_close() disables the subchannel and
sets the state to VFIO_CCW_STATE_STANDBY.

If vfio_ccw_sch_io_todo() then overwrites this state with IDLE, a subsequent
device open will encounter fsm_notoper() because the state machine handles
OPEN in the IDLE state by transitioning to NOT_OPER, which bricks the device
until it is manually recreated.

> +
>  	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
>  	mutex_unlock(&private->io_mutex);

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 6c74d596be9de..9242a37677a06 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -142,6 +149,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
>  	kmem_cache_free(vfio_ccw_io_region, private->io_region);
>  	kfree(private->cp.guest_cp);
> +	mutex_destroy(&private->cp_mutex);
>  	mutex_destroy(&private->io_mutex);
>  }

[Severity: High]
This is a pre-existing issue, but could destroying these mutexes here cause
a use-after-free and crash if pending asynchronous work items are still
active?

If an I/O interrupt fires right before the device is released (for example,
during unbind) and cio_disable_subchannel() returns 0 because the subchannel
is idle, the workqueue flush in vfio_ccw_sch_quiesce() gets skipped.

vfio_ccw_mdev_release_dev() will then destroy cp_mutex and io_mutex, and
free the private structure without cancelling queued work like io_work or
notoper_work. When vfio_ccw_sch_io_todo() or vfio_ccw_notoper_todo()
subsequently runs, it accesses the freed private struct and attempts to lock
the destroyed cp_mutex.

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