[PATCH v3 1/8] s390/vfio_ccw: free all memory if cp_init() fails

[PATCH v3 1/8] s390/vfio_ccw: free all memory if cp_init() fails

[Eric Farman]

The routine cp_free() is called to unpin/free any memory once an I/O
is completed successfully, or if cp_prefetch() fails. But if cp_init()
fails, and cp->initialized is not enabled, the same routine cannot be
used to free all the memory.

An attempt to address this exists in ccwchain_handle_ccw(), where a
single call to ccwchain_free() is made for the currently-processed
CCW segment. But this will leak other segments (created as a result
of a Transfer in Channel) that had been allocated as part of the same
channel program.

Address this by performing the cleanup outside of the recursive
ccwchain_handle_ccw()/ccwchain_loop_tic() logic.

Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free in cp_init")
Cc: stable@vger.kernel.org
Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 7561aa7d3e01..086d1b54bdb0 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -455,9 +455,6 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
 	/* Loop for tics on this new chain. */
 	ret = ccwchain_loop_tic(chain, cp);
 
-	if (ret)
-		ccwchain_free(chain);
-
 	return ret;
 }
 
@@ -486,6 +483,23 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp)
 	return 0;
 }
 
+static int ccwchain_build_ccws(dma32_t cda, struct channel_program *cp)
+{
+	struct ccwchain *chain, *temp;
+	int ret;
+
+	ret = ccwchain_handle_ccw(cda, cp);
+
+	if (ret) {
+		/* Cleanup if an error occurred */
+		list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
+			ccwchain_free(chain);
+		}
+	}
+
+	return ret;
+}
+
 static int ccwchain_fetch_tic(struct ccw1 *ccw,
 			      struct channel_program *cp)
 {
@@ -735,7 +749,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
 	memcpy(&cp->orb, orb, sizeof(*orb));
 
 	/* Build a ccwchain for the first CCW segment */
-	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
+	ret = ccwchain_build_ccws(orb->cmd.cpa, cp);
 
 	if (!ret)
 		cp->initialized = true;
-- 
2.53.0

Re: [PATCH v3 1/8] s390/vfio_ccw: free all memory if cp_init() fails

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> The routine cp_free() is called to unpin/free any memory once an I/O
> is completed successfully, or if cp_prefetch() fails. But if cp_init()
> fails, and cp->initialized is not enabled, the same routine cannot be
> used to free all the memory.
> 
> An attempt to address this exists in ccwchain_handle_ccw(), where a
> single call to ccwchain_free() is made for the currently-processed
> CCW segment. But this will leak other segments (created as a result
> of a Transfer in Channel) that had been allocated as part of the same
> channel program.
> 
> Address this by performing the cleanup outside of the recursive
> ccwchain_handle_ccw()/ccwchain_loop_tic() logic.
> 
> Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free in cp_init")
> Cc: stable@vger.kernel.org
> Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7561aa7d3e01..086d1b54bdb0 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -455,9 +455,6 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
>  	/* Loop for tics on this new chain. */
>  	ret = ccwchain_loop_tic(chain, cp);
>  
> -	if (ret)
> -		ccwchain_free(chain);
> -
>  	return ret;
>  }
>  
> @@ -486,6 +483,23 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp)
>  	return 0;
>  }
>  
> +static int ccwchain_build_ccws(dma32_t cda, struct channel_program *cp)
> +{
> +	struct ccwchain *chain, *temp;
> +	int ret;
> +
> +	ret = ccwchain_handle_ccw(cda, cp);
> +
> +	if (ret) {
> +		/* Cleanup if an error occurred */
> +		list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
> +			ccwchain_free(chain);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int ccwchain_fetch_tic(struct ccw1 *ccw,
>  			      struct channel_program *cp)
>  {
> @@ -735,7 +749,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
>  	memcpy(&cp->orb, orb, sizeof(*orb));
>  
>  	/* Build a ccwchain for the first CCW segment */
> -	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> +	ret = ccwchain_build_ccws(orb->cmd.cpa, cp);
>  
>  	if (!ret)
>  		cp->initialized = true;

[PATCH v3 2/8] s390/vfio_ccw: limit the number of channel program segments

[Eric Farman]

The processing of channel programs, and the CCWs within them, is done
recursively. As such, there is an arbitrary (but not architectural)
limit to the number of CCWs that can exist in a single channel program.

The vfio-ccw logic breaks these channel programs into segments whenever
it encounters a Transfer-In-Channel (TIC) CCW, and the combined number
of segments count towards the global limit. Impose an equivalent limit
to the number of segments until such logic can be made non-recursive.

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_cp.c | 6 ++++++
 drivers/s390/cio/vfio_ccw_cp.h | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 086d1b54bdb0..1c2890d139c6 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -332,6 +332,7 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
 		goto out_err;
 
 	list_add_tail(&chain->next, &cp->ccwchain_list);
+	cp->ccwchain_count++;
 
 	return chain;
 
@@ -441,6 +442,10 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
 	if (len < 0)
 		return len;
 
+	/* Limit number of chains in a single channel program */
+	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
+		return -EINVAL;
+
 	/* Need alloc a new chain for this one. */
 	chain = ccwchain_alloc(cp, len);
 	if (!chain)
@@ -745,6 +750,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
 			vdev->dev,
 			"Prefetching channel program even though prefetch not specified in ORB");
 
+	cp->ccwchain_count = 0;
 	INIT_LIST_HEAD(&cp->ccwchain_list);
 	memcpy(&cp->orb, orb, sizeof(*orb));
 
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index fc31eb699807..dc91a317ef19 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -23,6 +23,11 @@
  */
 #define CCWCHAIN_LEN_MAX	256
 
+/*
+ * Maximum number of chains
+ */
+#define CCWCHAIN_COUNT_MAX	16
+
 /**
  * struct channel_program - manage information for channel program
  * @ccwchain_list: list head of ccwchains
@@ -38,6 +43,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	int ccwchain_count;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

Re: [PATCH v3 2/8] s390/vfio_ccw: limit the number of channel program segments

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> The processing of channel programs, and the CCWs within them, is done
> recursively. As such, there is an arbitrary (but not architectural)
> limit to the number of CCWs that can exist in a single channel program.
> 
> The vfio-ccw logic breaks these channel programs into segments whenever
> it encounters a Transfer-In-Channel (TIC) CCW, and the combined number
> of segments count towards the global limit. Impose an equivalent limit
> to the number of segments until such logic can be made non-recursive.
> 

At a future point if/when this is made non-recursive, we should at that
time keep limits with sane defaults in-place but at also consider making
it kernel-configurable (like the vfio dma limit).

...

>   * struct channel_program - manage information for channel program
>   * @ccwchain_list: list head of ccwchains
> @@ -38,6 +43,7 @@ struct channel_program {
>  	union orb orb;
>  	bool initialized;
>  	struct ccw1 *guest_cp;
> +	int ccwchain_count;

Nit: Unsigned?

Regardless:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

>  };
>  
>  int cp_init(struct channel_program *cp, union orb *orb);

[PATCH v3 3/8] s390/vfio_ccw: fix out of bounds check on CCW array

[Eric Farman]

The routine ccwchain_calc_length() counts the number of channel
command words (CCWs) that are chained together in a single channel
program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.

The loop itself is "do..while (count < 257)", and while the logic in
is_cpa_within_range() correctly adjusts between the 0-index array of
CCWs and the count of CCWs starting at 1, this means it would look
at a possible 257th CCW before ending the loop and (correctly)
returning an error.

Fix this by restructuring the loop to break as soon as 256 CCWs
(thus indexes 0-255) are examined, without looking at memory
outside the range.

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_cp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 1c2890d139c6..258a99930c6a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -393,11 +393,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
 			break;
 
-		ccw++;
-	} while (cnt < CCWCHAIN_LEN_MAX + 1);
+		/* Exit the loop when we reach the maximum */
+		if (cnt >= CCWCHAIN_LEN_MAX) {
+			cnt = -EINVAL;
+			break;
+		}
 
-	if (cnt == CCWCHAIN_LEN_MAX + 1)
-		cnt = -EINVAL;
+		ccw++;
+	} while (1);
 
 	return cnt;
 }
-- 
2.53.0

Re: [PATCH v3 3/8] s390/vfio_ccw: fix out of bounds check on CCW array

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> The routine ccwchain_calc_length() counts the number of channel
> command words (CCWs) that are chained together in a single channel
> program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.
> 
> The loop itself is "do..while (count < 257)", and while the logic in
> is_cpa_within_range() correctly adjusts between the 0-index array of
> CCWs and the count of CCWs starting at 1, this means it would look
> at a possible 257th CCW before ending the loop and (correctly)
> returning an error.
> 
> Fix this by restructuring the loop to break as soon as 256 CCWs
> (thus indexes 0-255) are examined, without looking at memory
> outside the range.
> 
> 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_cp.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 

So, I think your approach is correct, but I tried for a bit to come up
with a more concise solution because the while(1) was bothering me.
The best I've managed to cook up is something like:

static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
{
	struct ccw1 *ccw = cp->guest_cp;
	int cnt;

	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
			return cnt;
	}

	return -EINVAL;
}

Assume CCWCHAIN_LEN_MAX=2
We enter with cnt=1, ccw=guest_cp[0], and 1 <=2 so we enter loop
either return 1 or loop, assume loop so we increment
now cnt=2, ccw=guest_cp[1], 2 <= 2 so we enter loop
either return 2 or loop, assume loop so we increment
now cnt=3, ccw=guest_cp[2]*, 3 <= 2 so we break out of loop and return -EINVAL

*So the only catch is that we will increment ccw past the end, but unlike the
original implementation we will break out of the loop before ever attempting to
use that ccw pointer at that point.

What do you think?

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 1c2890d139c6..258a99930c6a 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -393,11 +393,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
>  			break;
>  
> -		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> +		/* Exit the loop when we reach the maximum */
> +		if (cnt >= CCWCHAIN_LEN_MAX) {
> +			cnt = -EINVAL;
> +			break;
> +		}
>  
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> -		cnt = -EINVAL;
> +		ccw++;
> +	} while (1);
>  
>  	return cnt;
>  }

Re: [PATCH v3 3/8] s390/vfio_ccw: fix out of bounds check on CCW array

[Eric Farman]

On 7/24/26 12:49 PM, Matthew Rosato wrote:
> On 7/23/26 1:47 PM, Eric Farman wrote:
>> The routine ccwchain_calc_length() counts the number of channel
>> command words (CCWs) that are chained together in a single channel
>> program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.
>>
>> The loop itself is "do..while (count < 257)", and while the logic in
>> is_cpa_within_range() correctly adjusts between the 0-index array of
>> CCWs and the count of CCWs starting at 1, this means it would look
>> at a possible 257th CCW before ending the loop and (correctly)
>> returning an error.
>>
>> Fix this by restructuring the loop to break as soon as 256 CCWs
>> (thus indexes 0-255) are examined, without looking at memory
>> outside the range.
>>
>> 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_cp.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
> 
> So, I think your approach is correct, but I tried for a bit to come up
> with a more concise solution because the while(1) was bothering me.

Ah, yes, it was bothering me too, but I guess I forgot about it.

> The best I've managed to cook up is something like:
> 
> static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
> {
> 	struct ccw1 *ccw = cp->guest_cp;
> 	int cnt;
> 
> 	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
> 		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
> 			return cnt;
> 	}
> 
> 	return -EINVAL;
> }
> 

This seems reasonable, and is definitely nicer to read than what I had 
before (which contained artifacts of the old/busted setup). I'm running 
tests on this now, but will include in a v4.

> Assume CCWCHAIN_LEN_MAX=2
> We enter with cnt=1, ccw=guest_cp[0], and 1 <=2 so we enter loop
> either return 1 or loop, assume loop so we increment
> now cnt=2, ccw=guest_cp[1], 2 <= 2 so we enter loop
> either return 2 or loop, assume loop so we increment
> now cnt=3, ccw=guest_cp[2]*, 3 <= 2 so we break out of loop and return -EINVAL
> 
> *So the only catch is that we will increment ccw past the end, but unlike the
> original implementation we will break out of the loop before ever attempting to
> use that ccw pointer at that point.
> 
> What do you think?
> 
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 1c2890d139c6..258a99930c6a 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -393,11 +393,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>>   		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
>>   			break;
>>   
>> -		ccw++;
>> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
>> +		/* Exit the loop when we reach the maximum */
>> +		if (cnt >= CCWCHAIN_LEN_MAX) {
>> +			cnt = -EINVAL;
>> +			break;
>> +		}
>>   
>> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
>> -		cnt = -EINVAL;
>> +		ccw++;
>> +	} while (1);
>>   
>>   	return cnt;
>>   }
> 

[PATCH v3 4/8] s390/vfio_ccw: ensure first IDAW remains constant

[Eric Farman]

The first IDAW in a list does not need to be on a 2K/4K boundary
like all others, and so is read separately to accurately calculate
the size of the buffer needed to read the full IDAL.

Verify that the address found in the first IDAW is unchanged between
reads, to ensure a consistent set of IDAWs being worked with.

Fixes: 01aa26c672c0 ("s390/cio: Combine direct and indirect CCW paths")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 15 +++++++++++++++
 drivers/s390/cio/vfio_ccw_cp.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 258a99930c6a..06b6bc6142a0 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -533,6 +533,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
 	dma64_t *idaws;
 	dma32_t *idaws_f1;
+	u64 first_idaw;
 	int idal_len = idaw_nr * sizeof(*idaws);
 	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
 	int idaw_mask = ~(idaw_size - 1);
@@ -549,6 +550,17 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 			kfree(idaws);
 			return ERR_PTR(ret);
 		}
+
+		if (cp->orb.cmd.c64)
+			first_idaw = dma64_to_u64(idaws[0]);
+		else
+			first_idaw = dma32_to_u32(idaws[0] >> 32);
+
+		/* Unexpected mismatch from earlier read */
+		if (first_idaw != cp->guest_iova) {
+			kfree(idaws);
+			return ERR_PTR(-EINVAL);
+		}
 	} else {
 		/* Fabricate an IDAL based off CCW data address */
 		if (cp->orb.cmd.c64) {
@@ -614,6 +626,9 @@ static int ccw_count_idaws(struct ccw1 *ccw,
 		iova = dma32_to_u32(ccw->cda);
 	}
 
+	/* Save the read address for later */
+	cp->guest_iova = iova;
+
 	/* Format-1 IDAWs operate on 2K each */
 	if (!cp->orb.cmd.c64)
 		return idal_2k_nr_words((void *)iova, bytes);
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index dc91a317ef19..d547dbc969cc 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -43,6 +43,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	u64 guest_iova;
 	int ccwchain_count;
 };
 
-- 
2.53.0

Re: [PATCH v3 4/8] s390/vfio_ccw: ensure first IDAW remains constant

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> The first IDAW in a list does not need to be on a 2K/4K boundary
> like all others, and so is read separately to accurately calculate
> the size of the buffer needed to read the full IDAL.
> 
> Verify that the address found in the first IDAW is unchanged between
> reads, to ensure a consistent set of IDAWs being worked with.
> 
> Fixes: 01aa26c672c0 ("s390/cio: Combine direct and indirect CCW paths")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 15 +++++++++++++++
>  drivers/s390/cio/vfio_ccw_cp.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 258a99930c6a..06b6bc6142a0 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -533,6 +533,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
> +	u64 first_idaw;
>  	int idal_len = idaw_nr * sizeof(*idaws);
>  	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);
> @@ -549,6 +550,17 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  			kfree(idaws);
>  			return ERR_PTR(ret);
>  		}
> +
> +		if (cp->orb.cmd.c64)
> +			first_idaw = dma64_to_u64(idaws[0]);
> +		else
> +			first_idaw = dma32_to_u32(idaws[0] >> 32);

Sashiko mentions this one, since first_idaw is a u64 would
first_idaw = dma64_to_u64(idaws[0]) >> 32;
work?

Then you are doing the >> 32 against a u64 just like what
is done in ccw_count_idaws() where we set guest_iova for
comparison below.

Other than this line, this patch LGTM.

> +
> +		/* Unexpected mismatch from earlier read */
> +		if (first_idaw != cp->guest_iova) {
> +			kfree(idaws);
> +			return ERR_PTR(-EINVAL);
> +		}
>  	} else {
>  		/* Fabricate an IDAL based off CCW data address */
>  		if (cp->orb.cmd.c64) {
> @@ -614,6 +626,9 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  		iova = dma32_to_u32(ccw->cda);
>  	}
>  
> +	/* Save the read address for later */
> +	cp->guest_iova = iova;
> +
>  	/* Format-1 IDAWs operate on 2K each */
>  	if (!cp->orb.cmd.c64)
>  		return idal_2k_nr_words((void *)iova, bytes);
> diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
> index dc91a317ef19..d547dbc969cc 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.h
> +++ b/drivers/s390/cio/vfio_ccw_cp.h
> @@ -43,6 +43,7 @@ struct channel_program {
>  	union orb orb;
>  	bool initialized;
>  	struct ccw1 *guest_cp;
> +	u64 guest_iova;
>  	int ccwchain_count;
>  };
>  

[PATCH v3 5/8] s390/vfio_ccw: calculate idal length based on idaw type

[Eric Farman]

Sashiko pointed out that get_guest_idal() unconditionally calculates
the length of the IDAL presuming everything is a Format-2 IDAW.

The output of vfio-ccw is always Format-2, but the input can be either
Format-1 (31-bit addresses) or Format-2 (64-bit addresses). As a result,
the size of the guest IDAL may be incorrect and should be trimmed down.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260720203400.7328E1F000E9@smtp.kernel.org/
Fixes: 1b676fe3d9d3 ("vfio/ccw: handle a guest Format-1 IDAL")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 06b6bc6142a0..b4a041d35bba 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -233,6 +233,7 @@ static void convert_ccw0_to_ccw1(struct ccw1 *source, unsigned long len)
 }
 
 #define idal_is_2k(_cp) (!(_cp)->orb.cmd.c64 || (_cp)->orb.cmd.i2k)
+#define get_idaw_size(_cp) ((_cp)->orb.cmd.c64 ? sizeof(u64) : sizeof(u32))
 
 /*
  * Helpers to operate ccwchain.
@@ -534,7 +535,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 	dma64_t *idaws;
 	dma32_t *idaws_f1;
 	u64 first_idaw;
-	int idal_len = idaw_nr * sizeof(*idaws);
+	int idal_len = idaw_nr * get_idaw_size(cp);
 	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
 	int idaw_mask = ~(idaw_size - 1);
 	int i, ret;
@@ -602,7 +603,7 @@ static int ccw_count_idaws(struct ccw1 *ccw,
 	struct vfio_device *vdev =
 		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
 	u64 iova;
-	int size = cp->orb.cmd.c64 ? sizeof(u64) : sizeof(u32);
+	int size = get_idaw_size(cp);
 	int ret;
 	int bytes = 1;
 
-- 
2.53.0

Re: [PATCH v3 5/8] s390/vfio_ccw: calculate idal length based on idaw type

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> Sashiko pointed out that get_guest_idal() unconditionally calculates
> the length of the IDAL presuming everything is a Format-2 IDAW.
> 
> The output of vfio-ccw is always Format-2, but the input can be either
> Format-1 (31-bit addresses) or Format-2 (64-bit addresses). As a result,
> the size of the guest IDAL may be incorrect and should be trimmed down.
> 
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/r/20260720203400.7328E1F000E9@smtp.kernel.org/
> Fixes: 1b676fe3d9d3 ("vfio/ccw: handle a guest Format-1 IDAL")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 06b6bc6142a0..b4a041d35bba 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -233,6 +233,7 @@ static void convert_ccw0_to_ccw1(struct ccw1 *source, unsigned long len)
>  }
>  
>  #define idal_is_2k(_cp) (!(_cp)->orb.cmd.c64 || (_cp)->orb.cmd.i2k)
> +#define get_idaw_size(_cp) ((_cp)->orb.cmd.c64 ? sizeof(u64) : sizeof(u32))
>  
>  /*
>   * Helpers to operate ccwchain.
> @@ -534,7 +535,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
>  	u64 first_idaw;
> -	int idal_len = idaw_nr * sizeof(*idaws);
> +	int idal_len = idaw_nr * get_idaw_size(cp);
>  	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);
>  	int i, ret;
> @@ -602,7 +603,7 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  	struct vfio_device *vdev =
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	u64 iova;
> -	int size = cp->orb.cmd.c64 ? sizeof(u64) : sizeof(u32);
> +	int size = get_idaw_size(cp);
>  	int ret;
>  	int bytes = 1;
>  

[PATCH v3 6/8] s390/vfio_ccw: ensure index for read/write regions are within range

[Eric Farman]

The introduction of the capability chain rightly clamped the
region indexes to the range of the capabilities itself, but
neglected to do so for the existing read/write regions which
should also be enforced.

Fixes: db8e5d17ac03 ("vfio-ccw: add capabilities chain")
Cc: stable@vger.kernel.org
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_async.c | 4 ++++
 drivers/s390/cio/vfio_ccw_chp.c   | 4 ++++
 drivers/s390/cio/vfio_ccw_ops.c   | 7 +++----
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 420d89ba7f83..f213f62f6641 100644
--- a/drivers/s390/cio/vfio_ccw_async.c
+++ b/drivers/s390/cio/vfio_ccw_async.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/vfio.h>
+#include <linux/nospec.h>
 
 #include "vfio_ccw_private.h"
 
@@ -24,6 +25,8 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 	if (copy_to_user(buf, (void *)region + pos, count))
 		ret = -EFAULT;
@@ -48,6 +51,7 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
 	if (!mutex_trylock(&private->io_mutex))
 		return -EAGAIN;
 
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 	if (copy_from_user((void *)region + pos, buf, count)) {
 		ret = -EFAULT;
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 38c176cf6295..bf678270c457 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/nospec.h>
 #include <linux/vfio.h>
 #include "vfio_ccw_private.h"
 
@@ -26,6 +27,8 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (cio_update_schib(sch)) {
@@ -97,6 +100,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		list_del(&crw->next);
 
 	mutex_lock(&private->io_mutex);
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (crw)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 45ec722d25ea..032a1cdf4df7 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -243,6 +243,7 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
 		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
 	default:
 		index -= VFIO_CCW_NUM_REGIONS;
+		index = array_index_nospec(index, private->num_regions);
 		return private->region[index].ops->read(private, buf, count,
 							ppos);
 	}
@@ -295,6 +296,7 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
 		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
 	default:
 		index -= VFIO_CCW_NUM_REGIONS;
+		index = array_index_nospec(index, private->num_regions);
 		return private->region[index].ops->write(private, buf, count,
 							 ppos);
 	}
@@ -338,11 +340,8 @@ static int vfio_ccw_mdev_ioctl_get_region_info(struct vfio_device *vdev,
 		    VFIO_CCW_NUM_REGIONS + private->num_regions)
 			return -EINVAL;
 
-		info->index = array_index_nospec(info->index,
-						 VFIO_CCW_NUM_REGIONS +
-						 private->num_regions);
-
 		i = info->index - VFIO_CCW_NUM_REGIONS;
+		i = array_index_nospec(i, private->num_regions);
 
 		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
 		info->size = private->region[i].size;
-- 
2.53.0

Re: [PATCH v3 6/8] s390/vfio_ccw: ensure index for read/write regions are within range

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> The introduction of the capability chain rightly clamped the
> region indexes to the range of the capabilities itself, but
> neglected to do so for the existing read/write regions which
> should also be enforced.
> 
> Fixes: db8e5d17ac03 ("vfio-ccw: add capabilities chain")
> Cc: stable@vger.kernel.org
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_async.c | 4 ++++
>  drivers/s390/cio/vfio_ccw_chp.c   | 4 ++++
>  drivers/s390/cio/vfio_ccw_ops.c   | 7 +++----
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
> index 420d89ba7f83..f213f62f6641 100644
> --- a/drivers/s390/cio/vfio_ccw_async.c
> +++ b/drivers/s390/cio/vfio_ccw_async.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #include <linux/vfio.h>
> +#include <linux/nospec.h>
>  
>  #include "vfio_ccw_private.h"
>  
> @@ -24,6 +25,8 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
>  		return -EINVAL;
>  
>  	mutex_lock(&private->io_mutex);
> +
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  	if (copy_to_user(buf, (void *)region + pos, count))

Couldn't using array_index_nospec cause us to accidentally operate on
region[0] if ppos were ever out of range?

By comparison, vfio_pci_ioctl_get_region_info() does an explicit check
first, so to do something similar here and elsewhere in this patch:

if (i >= private->num_regions)
   return -EINVAL;
i = array_index_nospec(i, private->num_regions);

(Note that for this case and vfio_ccw_async_region_write my pseudocode
doesn't account for io_mutex)

>  		ret = -EFAULT;
> @@ -48,6 +51,7 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
>  	if (!mutex_trylock(&private->io_mutex))
>  		return -EAGAIN;
>  
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  	if (copy_from_user((void *)region + pos, buf, count)) {
>  		ret = -EFAULT;
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 38c176cf6295..bf678270c457 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/slab.h>
> +#include <linux/nospec.h>
>  #include <linux/vfio.h>
>  #include "vfio_ccw_private.h"
>  
> @@ -26,6 +27,8 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
>  		return -EINVAL;
>  
>  	mutex_lock(&private->io_mutex);
> +
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  
>  	if (cio_update_schib(sch)) {
> @@ -97,6 +100,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		list_del(&crw->next);
>  
>  	mutex_lock(&private->io_mutex);
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;
>  
>  	if (crw)
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 45ec722d25ea..032a1cdf4df7 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -243,6 +243,7 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
>  		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
> +		index = array_index_nospec(index, private->num_regions);
>  		return private->region[index].ops->read(private, buf, count,
>  							ppos);
>  	}
> @@ -295,6 +296,7 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
>  		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
> +		index = array_index_nospec(index, private->num_regions);
>  		return private->region[index].ops->write(private, buf, count,
>  							 ppos);
>  	}
> @@ -338,11 +340,8 @@ static int vfio_ccw_mdev_ioctl_get_region_info(struct vfio_device *vdev,
>  		    VFIO_CCW_NUM_REGIONS + private->num_regions)
>  			return -EINVAL;
>  
> -		info->index = array_index_nospec(info->index,
> -						 VFIO_CCW_NUM_REGIONS +
> -						 private->num_regions);
> -
>  		i = info->index - VFIO_CCW_NUM_REGIONS;
> +		i = array_index_nospec(i, private->num_regions);
>  
>  		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
>  		info->size = private->region[i].size;

Re: [PATCH v3 6/8] s390/vfio_ccw: ensure index for read/write regions are within range

[Eric Farman]

On 7/24/26 3:01 PM, Matthew Rosato wrote:
> On 7/23/26 1:47 PM, Eric Farman wrote:
>> The introduction of the capability chain rightly clamped the
>> region indexes to the range of the capabilities itself, but
>> neglected to do so for the existing read/write regions which
>> should also be enforced.
>>
>> Fixes: db8e5d17ac03 ("vfio-ccw: add capabilities chain")
>> Cc: stable@vger.kernel.org
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_async.c | 4 ++++
>>   drivers/s390/cio/vfio_ccw_chp.c   | 4 ++++
>>   drivers/s390/cio/vfio_ccw_ops.c   | 7 +++----
>>   3 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
>> index 420d89ba7f83..f213f62f6641 100644
>> --- a/drivers/s390/cio/vfio_ccw_async.c
>> +++ b/drivers/s390/cio/vfio_ccw_async.c
>> @@ -8,6 +8,7 @@
>>    */
>>   
>>   #include <linux/vfio.h>
>> +#include <linux/nospec.h>
>>   
>>   #include "vfio_ccw_private.h"
>>   
>> @@ -24,6 +25,8 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
>>   		return -EINVAL;
>>   
>>   	mutex_lock(&private->io_mutex);
>> +
>> +	i = array_index_nospec(i, private->num_regions);
>>   	region = private->region[i].data;
>>   	if (copy_to_user(buf, (void *)region + pos, count))
> 
> Couldn't using array_index_nospec cause us to accidentally operate on
> region[0] if ppos were ever out of range?

Hrm, yeah as I keep pondering this I think you're right. The parent 
routines (vfio_ccw_mdev_read(), for example) checks that it's not above 
num_regions + NUM_REGIONS, but there's no check against the subset in 
any of these. I'll add them; thanks.

> 
> By comparison, vfio_pci_ioctl_get_region_info() does an explicit check
> first, so to do something similar here and elsewhere in this patch:
> 
> if (i >= private->num_regions)
>     return -EINVAL;
> i = array_index_nospec(i, private->num_regions);
> 
> (Note that for this case and vfio_ccw_async_region_write my pseudocode
> doesn't account for io_mutex)
> 
>>   		ret = -EFAULT;
>> @@ -48,6 +51,7 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
>>   	if (!mutex_trylock(&private->io_mutex))
>>   		return -EAGAIN;
>>   
>> +	i = array_index_nospec(i, private->num_regions);
>>   	region = private->region[i].data;
>>   	if (copy_from_user((void *)region + pos, buf, count)) {
>>   		ret = -EFAULT;
>> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
>> index 38c176cf6295..bf678270c457 100644
>> --- a/drivers/s390/cio/vfio_ccw_chp.c
>> +++ b/drivers/s390/cio/vfio_ccw_chp.c
>> @@ -9,6 +9,7 @@
>>    */
>>   
>>   #include <linux/slab.h>
>> +#include <linux/nospec.h>
>>   #include <linux/vfio.h>
>>   #include "vfio_ccw_private.h"
>>   
>> @@ -26,6 +27,8 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
>>   		return -EINVAL;
>>   
>>   	mutex_lock(&private->io_mutex);
>> +
>> +	i = array_index_nospec(i, private->num_regions);
>>   	region = private->region[i].data;
>>   
>>   	if (cio_update_schib(sch)) {
>> @@ -97,6 +100,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>>   		list_del(&crw->next);
>>   
>>   	mutex_lock(&private->io_mutex);
>> +	i = array_index_nospec(i, private->num_regions);
>>   	region = private->region[i].data;
>>   
>>   	if (crw)
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index 45ec722d25ea..032a1cdf4df7 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -243,6 +243,7 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
>>   		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
>>   	default:
>>   		index -= VFIO_CCW_NUM_REGIONS;
>> +		index = array_index_nospec(index, private->num_regions);
>>   		return private->region[index].ops->read(private, buf, count,
>>   							ppos);
>>   	}
>> @@ -295,6 +296,7 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
>>   		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
>>   	default:
>>   		index -= VFIO_CCW_NUM_REGIONS;
>> +		index = array_index_nospec(index, private->num_regions);
>>   		return private->region[index].ops->write(private, buf, count,
>>   							 ppos);
>>   	}
>> @@ -338,11 +340,8 @@ static int vfio_ccw_mdev_ioctl_get_region_info(struct vfio_device *vdev,
>>   		    VFIO_CCW_NUM_REGIONS + private->num_regions)
>>   			return -EINVAL;
>>   
>> -		info->index = array_index_nospec(info->index,
>> -						 VFIO_CCW_NUM_REGIONS +
>> -						 private->num_regions);
>> -
>>   		i = info->index - VFIO_CCW_NUM_REGIONS;
>> +		i = array_index_nospec(i, private->num_regions);
>>   
>>   		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
>>   		info->size = private->region[i].size;
> 

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

[Eric Farman]

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

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

[Matthew Rosato]

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()?

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

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;

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

[Eric Farman]

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

[PATCH v3 8/8] s390/vfio_ccw: implement a crw lock

[Eric Farman]

Unlike the channel_program struct, which covers synchronous I/O
submissions and asynchronous interrupts, the CRW region relies
exclusively on asynchronous events coming from hardware.

Implement a lock to manage the list of those payloads, to ensure
they are read cohesively.

Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org
Cc: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 17 +++++++++++------
 drivers/s390/cio/vfio_ccw_drv.c     |  4 ++++
 drivers/s390/cio/vfio_ccw_ops.c     |  3 +++
 drivers/s390/cio/vfio_ccw_private.h |  2 ++
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index bf678270c457..3d05aaf790fa 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -93,15 +93,19 @@ 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);
+	i = array_index_nospec(i, private->num_regions);
+	region = private->region[i].data;
+
+	spin_lock(&private->crw_lock);
 	crw = list_first_entry_or_null(&private->crw,
 				       struct vfio_ccw_crw, next);
 
 	if (crw)
 		list_del(&crw->next);
 
-	mutex_lock(&private->io_mutex);
-	i = array_index_nospec(i, private->num_regions);
-	region = private->region[i].data;
+	/* Drop CRW lock while copying to userspace */
+	spin_unlock(&private->crw_lock);
 
 	if (crw)
 		memcpy(&region->crw, &crw->crw, sizeof(region->crw));
@@ -112,14 +116,15 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		ret = count;
 
 	region->crw = 0;
-
-	mutex_unlock(&private->io_mutex);
-
 	kfree(crw);
 
 	/* Notify the guest if more CRWs are on our queue */
+	spin_lock(&private->crw_lock);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock(&private->crw_lock);
+
+	mutex_unlock(&private->io_mutex);
 
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1d8c2ed9da50..2c0a27851342 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -125,8 +125,10 @@ void vfio_ccw_crw_todo(struct work_struct *work)
 
 	private = container_of(work, struct vfio_ccw_private, crw_work);
 
+	spin_lock(&private->crw_lock);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock(&private->crw_lock);
 }
 
 /*
@@ -300,7 +302,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 	crw->crw.erc = erc;
 	crw->crw.rsid = rsid;
 
+	spin_lock(&private->crw_lock);
 	list_add_tail(&crw->next, &private->crw);
+	spin_unlock(&private->crw_lock);
 	queue_work(vfio_ccw_work_q, &private->crw_work);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 04800cfa779b..277138f14b25 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -60,6 +60,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
+	spin_lock_init(&private->crw_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -138,10 +139,12 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 		container_of(vdev, struct vfio_ccw_private, vdev);
 	struct vfio_ccw_crw *crw, *temp;
 
+	spin_lock(&private->crw_lock);
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
 		kfree(crw);
 	}
+	spin_unlock(&private->crw_lock);
 
 	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
 	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ac5aaa78a74b..2562679b5ef5 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -121,6 +121,8 @@ struct vfio_ccw_private {
 
 	struct irb		irb;
 	union scsw		scsw;
+
+	spinlock_t		crw_lock;
 	struct list_head	crw;
 
 	struct eventfd_ctx	*io_trigger;
-- 
2.53.0

Re: [PATCH v3 8/8] s390/vfio_ccw: implement a crw lock

[Matthew Rosato]

On 7/23/26 1:47 PM, Eric Farman wrote:
> Unlike the channel_program struct, which covers synchronous I/O
> submissions and asynchronous interrupts, the CRW region relies
> exclusively on asynchronous events coming from hardware.
> 
> Implement a lock to manage the list of those payloads, to ensure
> they are read cohesively.
> 
> Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
> Cc: stable@vger.kernel.org
> Cc: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_chp.c     | 17 +++++++++++------
>  drivers/s390/cio/vfio_ccw_drv.c     |  4 ++++
>  drivers/s390/cio/vfio_ccw_ops.c     |  3 +++
>  drivers/s390/cio/vfio_ccw_private.h |  2 ++
>  4 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index bf678270c457..3d05aaf790fa 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -93,15 +93,19 @@ 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);
> +	i = array_index_nospec(i, private->num_regions);

Same comment as patch 6 wrt to array_index_nospec

...

> index ac5aaa78a74b..2562679b5ef5 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -121,6 +121,8 @@ struct vfio_ccw_private {
>  
>  	struct irb		irb;
>  	union scsw		scsw;
> +
> +	spinlock_t		crw_lock;

This should have a comment either here or, in keeping with the rest of
the struct, in the comment block above.

This looks good if you're trying to protect private->crw list, I don't
think it's complete if you're also trying to protect
private->crw_trigger - it looks to me like you hold the new lock over
most accesses to crw_trigger but at least not vfio_ccw_mdev_set_irqs
where you set/clear it.


>  	struct list_head	crw;
>  
>  	struct eventfd_ctx	*io_trigger;

Re: [PATCH v3 8/8] s390/vfio_ccw: implement a crw lock

[Eric Farman]

On 7/24/26 4:44 PM, Matthew Rosato wrote:
> On 7/23/26 1:47 PM, Eric Farman wrote:
>> Unlike the channel_program struct, which covers synchronous I/O
>> submissions and asynchronous interrupts, the CRW region relies
>> exclusively on asynchronous events coming from hardware.
>>
>> Implement a lock to manage the list of those payloads, to ensure
>> they are read cohesively.
>>
>> Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
>> Cc: stable@vger.kernel.org
>> Cc: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_chp.c     | 17 +++++++++++------
>>   drivers/s390/cio/vfio_ccw_drv.c     |  4 ++++
>>   drivers/s390/cio/vfio_ccw_ops.c     |  3 +++
>>   drivers/s390/cio/vfio_ccw_private.h |  2 ++
>>   4 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
>> index bf678270c457..3d05aaf790fa 100644
>> --- a/drivers/s390/cio/vfio_ccw_chp.c
>> +++ b/drivers/s390/cio/vfio_ccw_chp.c
>> @@ -93,15 +93,19 @@ 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);
>> +	i = array_index_nospec(i, private->num_regions);
> 
> Same comment as patch 6 wrt to array_index_nospec
> 
> ...
> 
>> index ac5aaa78a74b..2562679b5ef5 100644
>> --- a/drivers/s390/cio/vfio_ccw_private.h
>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>> @@ -121,6 +121,8 @@ struct vfio_ccw_private {
>>   
>>   	struct irb		irb;
>>   	union scsw		scsw;
>> +
>> +	spinlock_t		crw_lock;
> 
> This should have a comment either here or, in keeping with the rest of
> the struct, in the comment block above.

Thanks, missed that. The crw field itself was never added to the comment 
block, so I'll add that as well.

Similarly, I didn't add definitions for the new fields in struct 
channel_program earlier in this series, so I've gone back and done those.

> 
> This looks good if you're trying to protect private->crw list, I don't
> think it's complete if you're also trying to protect
> private->crw_trigger - it looks to me like you hold the new lock over
> most accesses to crw_trigger but at least not vfio_ccw_mdev_set_irqs
> where you set/clear it.

I suspect you're right that crw_trigger needs some love and be moved 
under this umbrella, but I'm drawing the line at private->crw for the now.

> 
> 
>>   	struct list_head	crw;
>>   
>>   	struct eventfd_ctx	*io_trigger;
>