Re: [PATCH v5 09/10] 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:

New issues:
- [High] Channel program resources are leaked if `notoper_work` is cancelled during device release.

Pre-existing issues:
- [High] Unprotected access to `private->irb` could lead to data corruption on concurrent interrupts.
--

--- Patch [9]: [PATCH v5 09/10] s390/vfio_ccw: implement a channel program mutex ---
commit cd6c396d349e5308a7574cecefee3c092057158e
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program mutex

This commit introduces a mutex to serialize the manipulation of the channel
program structure (private->cp). It also adjusts where the subchannel
spinlock is acquired and released so it coexists properly with the new
mutex.

> 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));

[Severity: High]
This is a pre-existing issue, but does this code locklessly access the
shared irb structure?

In fsm_irq(), private->irb can be overwritten locklessly from hardirq
context:

fsm_irq()
    memcpy(&private->irb, irb, sizeof(*irb));

Concurrently, vfio_ccw_sch_io_todo() reads private->irb here and evaluates
its contents without acquiring a lock like sch->lock.

If a spurious or unsolicited hardware interrupt arrives while the workqueue
is processing a previous interrupt, could a torn or partially updated read
result in incorrect I/O completion status being passed to the guest?

> +
> +	mutex_lock(&private->cp_mutex);
>  	if (scsw_is_solicited(&irb->scsw)) {
>  		cp_update_scsw(&private->cp, &irb->scsw);

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 26c19f0e5e474..b8e702f36106c 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -146,6 +153,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);

[Severity: High]
Can this leak channel program resources?

If fsm_close() fails, it triggers a VFIO_CCW_EVENT_NOT_OPER event, which
queues notoper_work to free the channel program.

If the device is subsequently released before the workqueue executes,
vfio_ccw_mdev_release_dev() cancels the work:

vfio_ccw_mdev_release_dev() {
    ...
    cancel_work_sync(&private->notoper_work);
    ...
}

Since cancel_work_sync() aborts the cleanup without a fallback, and
vfio_ccw_mdev_release_dev() does not explicitly call cp_free(), do the
channel program structures (ccwchain) and pinned guest pages remain
leaked indefinitely?

>  	mutex_destroy(&private->io_mutex);
>  }

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