Re: [PATCH v3 7/8] s390/vfio_ccw: implement a channel program mutex

[Eric Farman <farman@linux.ibm.com>]

On 7/24/26 4:18 PM, Matthew Rosato wrote:
> On 7/23/26 1:47 PM, Eric Farman wrote:
>> The channel_program struct is manipulated without a serialization
>> mechanism to ensure consistent behavior. Take a broad stroke of
>> putting the entire structure behind a mutex, and ensure everything
>> that needs private->cp holds this mutex.
>>
>> There are a couple where the cio layer's subchannel->lock performs
>> this role in this code, which isn't correct (it should only be used
>> when touching the actual subchannel, like cio_enable_subchannel()),
>> so adjust the locations where that spinlock is acquired/released
>> to correctly coexist with this new mutex.
>>
>> Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c     | 10 +++++++++-
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 22 +++++++++++++++-------
>>   drivers/s390/cio/vfio_ccw_ops.c     | 10 +++++++++-
>>   drivers/s390/cio/vfio_ccw_private.h |  3 +++
>>   4 files changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 1a095085bc72..1d8c2ed9da50 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);
>> +
>>   	mutex_lock(&private->io_mutex);
>>   	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
>>   	mutex_unlock(&private->io_mutex);
>> @@ -259,12 +263,16 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>>   	rc = 0;
>>   
>>   	if (cio_update_schib(sch)) {
>> -		if (private)
>> +		if (private) {
>> +			spin_unlock_irqrestore(&sch->lock, flags);
>>   			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
>> +			goto out;
>> +		}
>>   	}
>>   
>>   out_unlock:
>>   	spin_unlock_irqrestore(&sch->lock, flags);
>> +out:
>>   
>>   	return rc;
>>   }
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index 4d7988ea47ef..9fbe97bd23f9 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -25,17 +25,15 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>>   	unsigned long flags;
>>   	int ret;
>>   
>> -	spin_lock_irqsave(&sch->lock, flags);
>> -
>>   	orb = cp_get_orb(&private->cp, sch);
> 
> Can you extend the documentation for cp_get_orb to indicate that
> cp_mutex must be held on entry?
> 
> Actually, better yet, can you use lockdep_assert_held()?

Of course; should've added that in the first place.

> 
> Would be a good idea to put these also in other functions that have now
> an implicit expectation that the mutex will be held, here's the list I
> came up with?
> 
> cp_get_orb
> cp_init
> cp_prefetch
> cp_free
> fsm_io_helper
> cp_iova_pinned	
> cp_update_scsw

I'm going to add them to cp_* functions, which means no fsm_io_helper. 
It's only interaction is for calling cp_get_orb anyway (and is the only 
one to do so), so it'll get covered in that path.

> 
> In some cases (like cp_get_orb) you'd have to do something like:
>      struct vfio_ccw_private *private = container_of(cp, struct vfio_ccw_private, cp);
>      lockdep_assert_held(&private->cp_mutex);
> 
> Then this lets us also use lockdep to make sure the mutex is OK now
> and in the future.
> 
>> -	if (!orb) {
>> -		ret = -EIO;
>> -		goto out;
>> -	}
>> +	if (!orb)
>> +		return -EIO;
>>   
>>   	VFIO_CCW_TRACE_EVENT(5, "stIO");
>>   	VFIO_CCW_TRACE_EVENT(5, dev_name(&sch->dev));
>>   
>> +	spin_lock_irqsave(&sch->lock, flags);
>> +
>>   	/* Issue "Start Subchannel" */
>>   	ccode = ssch(sch->schid, orb);
>>   
>> @@ -71,7 +69,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
>>   	default:
>>   		ret = ccode;
>>   	}
>> -out:
>>   	spin_unlock_irqrestore(&sch->lock, flags);
>>   	return ret;
>>   }
>> @@ -171,7 +168,9 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>>   	private->state = VFIO_CCW_STATE_NOT_OPER;
>>   
>>   	/* This is usually handled during CLOSE event */
>> +	mutex_lock(&private->cp_mutex);
>>   	cp_free(&private->cp);
>> +	mutex_unlock(&private->cp_mutex);
>>   }
>>   
>>   /*
>> @@ -252,6 +251,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>   	private->state = VFIO_CCW_STATE_CP_PROCESSING;
>>   	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
>>   
>> +	mutex_lock(&private->cp_mutex);
>> +
>>   	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {
>>   		orb = (union orb *)io_region->orb_area;
>>   
>> @@ -300,6 +301,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>   			cp_free(&private->cp);
>>   			goto err_out;
>>   		}
>> +
>> +		mutex_unlock(&private->cp_mutex);
>>   		return;
>>   	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
>>   		VFIO_CCW_MSG_EVENT(2,
>> @@ -320,6 +323,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>>   	}
>>   
>>   err_out:
>> +	mutex_unlock(&private->cp_mutex);
>>   	private->state = VFIO_CCW_STATE_IDLE;
>>   	trace_vfio_ccw_fsm_io_request(scsw->cmd.fctl, schid,
>>   				      io_region->ret_code, errstr);
>> @@ -410,7 +414,11 @@ static void fsm_close(struct vfio_ccw_private *private,
>>   
>>   	private->state = VFIO_CCW_STATE_STANDBY;
>>   	spin_unlock_irq(&sch->lock);
>> +
>> +	mutex_lock(&private->cp_mutex);
>>   	cp_free(&private->cp);
>> +	mutex_unlock(&private->cp_mutex);
>> +
>>   	return;
>>   
>>   err_unlock:
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index 032a1cdf4df7..04800cfa779b 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -38,8 +38,13 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
>>   		container_of(vdev, struct vfio_ccw_private, vdev);
>>   
>>   	/* Drivers MUST unpin pages in response to an invalidation. */
>> -	if (!cp_iova_pinned(&private->cp, iova, length))
>> +	mutex_lock(&private->cp_mutex);
>> +	if (!cp_iova_pinned(&private->cp, iova, length)) {
>> +		mutex_unlock(&private->cp_mutex);
>>   		return;
>> +	}
>> +
>> +	mutex_unlock(&private->cp_mutex);
>>   
>>   	vfio_ccw_mdev_reset(private);
>>   }
>> @@ -50,6 +55,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
>>   		container_of(vdev, struct vfio_ccw_private, vdev);
>>   
>>   	mutex_init(&private->io_mutex);
>> +	mutex_init(&private->cp_mutex);
>>   	private->state = VFIO_CCW_STATE_STANDBY;
>>   	INIT_LIST_HEAD(&private->crw);
>>   	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
>> @@ -90,6 +96,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
>>   out_free_cp:
>>   	kfree(private->cp.guest_cp);
>>   out_free_private:
>> +	mutex_destroy(&private->cp_mutex);
>>   	mutex_destroy(&private->io_mutex);
>>   	return -ENOMEM;
>>   }
>> @@ -141,6 +148,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);
>>   }
>>   
>> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
>> index 0501d4bbcdbd..ac5aaa78a74b 100644
>> --- a/drivers/s390/cio/vfio_ccw_private.h
>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>> @@ -94,6 +94,7 @@ struct vfio_ccw_parent {
>>    * @schib_region: MMIO region for SCHIB information
>>    * @crw_region: MMIO region for getting channel report words
>>    * @num_regions: number of additional regions
>> + * @cp_mutex: protect against concurrent update of CP resources
>>    * @cp: channel program for the current I/O operation
>>    * @irb: irb info received from interrupt
>>    * @scsw: scsw info
>> @@ -115,7 +116,9 @@ struct vfio_ccw_private {
>>   	struct ccw_crw_region	*crw_region;
>>   	int num_regions;
>>   
>> +	struct mutex		cp_mutex;
>>   	struct channel_program	cp;
>> +
>>   	struct irb		irb;
>>   	union scsw		scsw;
>>   	struct list_head	crw;
>