Re: [PATCH v7 09/10] s390/vfio_ccw: selectively expand io_mutex

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] TOCTOU race in cp_iova_pinned ignores IOMMU unmap requests, leading to hardware DMA into unmapped memory.
- [High] Data race and list corruption on private->crw between process and IRQ contexts.
--

--- Patch [9]: [PATCH v7 09/10] s390/vfio_ccw: selectively expand io_mutex ---
commit e5ee662e08e75a748fbe4dadeba55875910744f0
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: selectively expand io_mutex

This patch expands the scope of the io_mutex to serialize I/O regions
alongside I/O operations themselves. It also defers cp_free() to a
workqueue to avoid acquiring mutexes in interrupt context.

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index f3015132d4b5f..9269b54f5cfd4 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -98,13 +98,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
>  
> +	mutex_lock(&private->io_mutex);
>  	crw = list_first_entry_or_null(&private->crw,
>  				       struct vfio_ccw_crw, next);
>  
>  	if (crw)
>  		list_del(&crw->next);

[Severity: High]
This is a pre-existing issue, but does holding only io_mutex provide enough
protection here?

In vfio_ccw_queue_crw(), which is called from hardware interrupt context,
items appear to be appended to private->crw locklessly:

vfio_ccw_queue_crw() {
        ...
        list_add_tail(&crw->next, &private->crw);
        queue_work(vfio_ccw_work_q, &private->crw_work);
}

Since io_mutex is a sleeping lock and cannot be acquired in IRQ context,
will concurrent list manipulations between the IRQ context and this read
corrupt the doubly-linked list pointers?

> -	mutex_lock(&private->io_mutex);
>  	if (i >= private->num_regions) {
>  		ret = -EINVAL;
>  		goto out;
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 5ef082b8289af..58722c4baa251 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -977,17 +977,23 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
>   */
>  bool cp_iova_pinned(struct channel_program *cp, u64 iova, u64 length)
>  {
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
>  	struct ccwchain *chain;
>  	int i;
>  
>  	if (!cp->initialized)
>  		return false;

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to check
cp->initialized without holding io_mutex?

If a VFIO unmap ioctl occurs concurrently while cp_init() is pinning pages
(but before cp_init() sets cp->initialized = true at the very end), could
this lockless check return false, causing the unmap request to be
incorrectly ignored?

If the unmap request is ignored while the I/O is submitted to the hardware,
could this result in the hardware performing DMA into memory the guest
considers unmapped?

> +	mutex_lock(&private->io_mutex);
>  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
>  		for (i = 0; i < chain->ch_len; i++)
> -			if (page_array_iova_pinned(&chain->ch_pa[i], iova, length))
> +			if (page_array_iova_pinned(&chain->ch_pa[i], iova, length)) {
> +				mutex_unlock(&private->io_mutex);
>  				return true;
> +			}
>  	}
> +	mutex_unlock(&private->io_mutex);
>  
>  	return false;
>  }

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