[PATCH v8 00/10] s390/vfio_ccw fixes

[PATCH v8 00/10] s390/vfio_ccw fixes

[Eric Farman]

This series addresses some pre-existing issues found in the
s390 vfio_ccw (DASD passthrough) driver.

Patches 1-6 are identical to v6/v7.
Patches 7-10 are a continued attempt to improve access to private->cp,
as the results from v7 showed minor problems compared to previous iterations.

v1: https://lore.kernel.org/r/20260714232208.1683788-1-farman@linux.ibm.com/
v2: https://lore.kernel.org/r/20260720201931.976660-1-farman@linux.ibm.com/
v3: https://lore.kernel.org/r/20260723174751.1180334-1-farman@linux.ibm.com/
v4: https://lore.kernel.org/r/20260725152705.3958100-1-farman@linux.ibm.com/
v5: https://lore.kernel.org/r/20260726040129.2946151-1-farman@linux.ibm.com/
v6: https://lore.kernel.org/r/20260726172153.1818056-1-farman@linux.ibm.com/
v7: https://lore.kernel.org/r/20260727192230.2715207-1-farman@linux.ibm.com/
v7->v8:
 - [FA] Applied r-b to patch 10 (thank you!)
 - [MR] Cleanup commit message on patch 8 (leftover from an earlier version)
 - [MR/sashiko] Move cancel_work_sync() calls prior to the crw list cleanup in
   vfio_ccw_mdev_release_dev (really only crw_work needs to be moved, but to
   keep it tidy..)
 - [MR] Call flush_work(notoper_work) in vfio_ccw_mdev_close_dev,
   to ensure a queued cp is released if pending, rather than arbitrarily
   cancelled and orphaned. Remove the cleanup from _release_dev, because
   any open device will take a trip through _close_dev first to be cleaned
   up, and any closed device will not be able to queue any CP's without
   first being opened

Eric Farman (10):
  s390/vfio_ccw: free all memory if cp_init() fails
  s390/vfio_ccw: limit the number of channel program segments
  s390/vfio_ccw: fix out of bounds check on CCW array
  s390/vfio_ccw: ensure first IDAW remains constant
  s390/vfio_ccw: calculate idal length based on idaw type
  s390/vfio_ccw: ensure index for read/write regions are within range
  s390/vfio_ccw: cancel existing workqueues
  s390/vfio_ccw: move cp cleanup out of not operational
  s390/vfio_ccw: selectively expand io_mutex
  s390/vfio_ccw: implement a crw lock

 drivers/s390/cio/vfio_ccw_async.c   | 16 +++++++
 drivers/s390/cio/vfio_ccw_chp.c     | 31 ++++++++++--
 drivers/s390/cio/vfio_ccw_cp.c      | 74 +++++++++++++++++++++--------
 drivers/s390/cio/vfio_ccw_cp.h      | 10 ++++
 drivers/s390/cio/vfio_ccw_drv.c     | 21 +++++++-
 drivers/s390/cio/vfio_ccw_fsm.c     |  8 +++-
 drivers/s390/cio/vfio_ccw_ops.c     | 30 ++++++++++--
 drivers/s390/cio/vfio_ccw_private.h | 10 +++-
 8 files changed, 167 insertions(+), 33 deletions(-)

-- 
2.53.0

[PATCH v8 01/10] 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>
Reviewed-by: Matthew Rosato <mjrosato@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 v8 01/10] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unbounded recursion in CCW chain processing allows a malicious guest to overflow the host kernel stack.
- [High] Off-by-one out-of-bounds read in `ccwchain_calc_length()`.
--

--- Patch [1]: [PATCH v8 01/10] s390/vfio_ccw: free all memory if cp_init() fails ---
commit 7b177e824046c97e78d70a5cb8daa7a37e59333e
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: free all memory if cp_init() fails

This commit addresses a memory leak in the channel program cleanup logic.
By moving the cleanup outside of the recursive handling functions, it
ensures all segments created by Transfer in Channel operations are
properly freed when initialization fails.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7561aa7d3e017..086d1b54bdb05 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);

[Severity: Critical]
This is a pre-existing issue, but does this recursive call chain allow a
guest to trigger unbounded recursion and exhaust the host kernel stack?

Looking at the code execution path:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_handle_ccw()
   ret = ccwchain_loop_tic(chain, cp);

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_loop_tic()
   ret = ccwchain_handle_ccw(tic->cda, cp);

If a guest constructs a channel program with a long sequence of Transfer
In Channel (TIC) instructions, each pointing to a distinct CCW chain
memory address, tic_target_chain_exists() will return false.

Could this cause ccwchain_loop_tic() to recursively call
ccwchain_handle_ccw() for each segment without any depth limits, leading
to a guest-to-host denial of service?


[Severity: High]
This isn't a bug introduced by this patch, but is there an off-by-one
out-of-bounds read in ccwchain_calc_length() which is called earlier in
this function?

ccwchain_handle_ccw() calls ccwchain_calc_length(), which has the following
loop condition:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
    ...
    } while (cnt < CCWCHAIN_LEN_MAX + 1);
    ...
}

Since guest_cp is allocated with exactly CCWCHAIN_LEN_MAX elements (256),
when cnt becomes 257 during the iteration, evaluating the ccw pointer
accesses guest_cp[256].

If a guest submits a chain of exactly CCWCHAIN_LEN_MAX length with the
command-chaining flag enabled on the last element, could this read past the
end of the allocation and potentially oops if it hits an unmapped page?

> -	if (ret)
> -		ccwchain_free(chain);
> -
>  	return ret;
>  }

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

[PATCH v8 02/10] 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 6 ++++++
 drivers/s390/cio/vfio_ccw_cp.h | 8 ++++++++
 2 files changed, 14 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..a9b1d8dbc6f6 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -23,11 +23,18 @@
  */
 #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
  * @orb: orb for the currently processed ssch request
  * @initialized: whether this instance is actually initialized
+ * @guest_cp: copy of guest channel program
+ * @ccwchain_count: number of channel program segments (linked by TIC)
  *
  * @ccwchain_list is the head of a ccwchain list, that contents the
  * translated result of the guest channel program that pointed out by
@@ -38,6 +45,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	unsigned int ccwchain_count;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

Re: [PATCH v8 02/10] s390/vfio_ccw: limit the number of channel program segments

[sashiko-bot]

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

Pre-existing issues:
- [Critical] A malicious guest can trigger a host kernel out-of-bounds read and potentially execute DMA to arbitrary host physical addresses by concurrently modifying the first IDAW in a channel program.
- [High] An off-by-one error in `ccwchain_calc_length` causes a deterministic out-of-bounds read when processing a channel program with exactly `CCWCHAIN_LEN_MAX` chained CCWs, which can crash the host if KASAN is enabled.
--

--- Patch [2]: [PATCH v8 02/10] s390/vfio_ccw: limit the number of channel program segments ---
commit 8d46ac3adbb3bfeb184e8057b932faa35fe79719
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: limit the number of channel program segments

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.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086d1b54bdb05..1c2890d139c6b 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c

[ ... ]

> @@ -441,6 +442,10 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
>  	if (len < 0)
>  		return len;
>  

[Severity: High]
This is a pre-existing issue, but does ccwchain_calc_length() have an
off-by-one error leading to an out-of-bounds read?

When a guest provides a channel program with exactly CCWCHAIN_LEN_MAX chained
CCWs, the do-while loop in ccwchain_calc_length() executes 256 times:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
    ...
    } while (cnt < CCWCHAIN_LEN_MAX + 1);
    ...
}

This increments cnt to 256 and advances ccw to point to guest_cp[256].
Since the loop condition (256 < 257) evaluates to true, the loop executes
a 257th time, dereferencing ccw in !ccw_is_chain(ccw). Since guest_cp is
allocated with exactly 256 elements, this read crosses the slab boundary.

Could this crash the host if KASAN is enabled?

[Severity: Critical]
This is a pre-existing issue, but can a malicious guest trigger an
out-of-bounds read and potentially execute DMA to arbitrary host physical
addresses by concurrently modifying the first IDAW in a channel program?

When fetching a guest CCW, ccw_count_idaws() performs the first fetch from
guest memory to calculate the required array size:

drivers/s390/cio/vfio_ccw_cp.c:ccw_count_idaws() {
    ...
    ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), &iova, size, false);
    ...
}

It then calls get_guest_idal(), which allocates a kernel buffer and reads the
entire IDAL from guest memory again into the undersized kernel buffer:

drivers/s390/cio/vfio_ccw_cp.c:get_guest_idal() {
    ...
    ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false);
    ...
}

If a guest provides a page-aligned first IDAW during the first fetch, and
concurrently modifies its memory so the second fetch reads a misaligned first
IDAW, the hardware will require more IDAWs than originally allocated.

Does this cause the hardware to read past the end of the idaws kernel buffer
and interpret adjacent memory as host physical addresses for DMA?

> +	/* 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);

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

[PATCH v8 03/10] 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 1c2890d139c6..af632f9d5453 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -377,11 +377,9 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
 static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 {
 	struct ccw1 *ccw = cp->guest_cp;
-	int cnt = 0;
-
-	do {
-		cnt++;
+	int cnt;
 
+	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
 		/*
 		 * We want to keep counting if the current CCW has the
 		 * command-chaining flag enabled, or if it is a TIC CCW
@@ -391,15 +389,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 		 * after the TIC, depending on the results of its operation.
 		 */
 		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
-			break;
-
-		ccw++;
-	} while (cnt < CCWCHAIN_LEN_MAX + 1);
-
-	if (cnt == CCWCHAIN_LEN_MAX + 1)
-		cnt = -EINVAL;
+			return cnt;
+	}
 
-	return cnt;
+	return -EINVAL;
 }
 
 static int tic_target_chain_exists(struct ccw1 *tic, struct channel_program *cp)
-- 
2.53.0

Re: [PATCH v8 03/10] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver unconditionally attempts to read a full 2048-byte chunk (256 CCWs) from guest memory, which will fail if a valid, shorter channel program is located near the end of mapped guest memory.
--

--- Patch [3]: [PATCH v8 03/10] s390/vfio_ccw: fix out of bounds check on CCW array ---
commit f979cac6ff9095b28670a21c1bdc95dfaa156466
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: fix out of bounds check on CCW array

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 is restructured to avoid checking memory outside the valid range.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 1c2890d139c6b..af632f9d54535 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -377,11 +377,9 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
>  static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  {
>  	struct ccw1 *ccw = cp->guest_cp;
> -	int cnt = 0;
> -
> -	do {
> -		cnt++;
> +	int cnt;
>  
> +	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {

[Severity: High]
This is a pre-existing issue, but since we are modifying the CCW bounds
checking here, could the unconditional 2K DMA read in ccwchain_handle_ccw()
inadvertently reject valid channel programs?

The driver unconditionally attempts to read a full 2048-byte chunk (256 CCWs)
from guest memory before calculating the actual length:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_handle_ccw() {
    ...
    /* Copy 2K (the most we support today) of possible CCWs */
    ret = vfio_dma_rw(vdev, gcda, cp->guest_cp,
                      CCWCHAIN_LEN_MAX * sizeof(struct ccw1), false);
    if (ret)
        return ret;
    ...
    /* Count the CCWs in the current chain */
    len = ccwchain_calc_length(gcda, cp);
    ...
}

If a guest uses a strict vIOMMU mapping and places a valid, shorter channel
program near the end of a mapped IOVA region, won't this read cross into an
unmapped page and fail with -EFAULT? This appears to render the device
partially or fully unusable for guests configuring their mappings this way.

>  		/*
>  		 * We want to keep counting if the current CCW has the
>  		 * command-chaining flag enabled, or if it is a TIC CCW

[ ... ]

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

[PATCH v8 04/10] 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 16 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_cp.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index af632f9d5453..6275794751cb 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -523,6 +523,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);
@@ -539,6 +540,18 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 			kfree(idaws);
 			return ERR_PTR(ret);
 		}
+
+		idaws_f1 = (dma32_t *)idaws;
+		if (cp->orb.cmd.c64)
+			first_idaw = dma64_to_u64(idaws[0]);
+		else
+			first_idaw = dma32_to_u32(idaws_f1[0]);
+
+		/* 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) {
@@ -604,6 +617,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 a9b1d8dbc6f6..9af98ff12d67 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -35,6 +35,7 @@
  * @initialized: whether this instance is actually initialized
  * @guest_cp: copy of guest channel program
  * @ccwchain_count: number of channel program segments (linked by TIC)
+ * @guest_iova: first data address of a guest channel program
  *
  * @ccwchain_list is the head of a ccwchain list, that contents the
  * translated result of the guest channel program that pointed out by
@@ -46,6 +47,7 @@ struct channel_program {
 	bool initialized;
 	struct ccw1 *guest_cp;
 	unsigned int ccwchain_count;
+	u64 guest_iova;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

Re: [PATCH v8 04/10] s390/vfio_ccw: ensure first IDAW remains constant

[sashiko-bot]

> 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
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v8 05/10] 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
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 6275794751cb..5ef082b8289a 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.
@@ -524,7 +525,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;
@@ -593,7 +594,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 v8 05/10] s390/vfio_ccw: calculate idal length based on idaw type

[sashiko-bot]

> 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
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v8 06/10] 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>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_async.c | 16 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_chp.c   | 15 +++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c   |  7 +++----
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 420d89ba7f83..4aff0b58fa5d 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,11 +25,20 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 	if (copy_to_user(buf, (void *)region + pos, count))
 		ret = -EFAULT;
 	else
 		ret = count;
+
+out_unlock:
 	mutex_unlock(&private->io_mutex);
 	return ret;
 }
@@ -48,6 +58,12 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
 	if (!mutex_trylock(&private->io_mutex))
 		return -EAGAIN;
 
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	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..f3015132d4b5 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,13 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (cio_update_schib(sch)) {
@@ -97,6 +105,12 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		list_del(&crw->next);
 
 	mutex_lock(&private->io_mutex);
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (crw)
@@ -109,6 +123,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 
 	region->crw = 0;
 
+out:
 	mutex_unlock(&private->io_mutex);
 
 	kfree(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 v8 06/10] s390/vfio_ccw: ensure index for read/write regions are within range

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent access to the `private->crw` list without locking can lead to double-free and list corruption.
- [High] `private->io_work` and `private->crw_work` are not explicitly canceled before the `private` structure is freed, leading to a Use-After-Free.
--

--- Patch [6]: [PATCH v8 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit dd567bdeb9066b3fa10790bef4429b4c38a7df1f
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure index for read/write regions are within range
    
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.

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 38c176cf62957..f3015132d4b5f 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c

[ ... ]

> @@ -97,6 +105,12 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		list_del(&crw->next);

[Severity: High]
This is a pre-existing issue, but does concurrent access to the private->crw
list without locking lead to double-free and list corruption here?

The private->crw list is modified by list_add_tail() in vfio_ccw_queue_crw()
(invoked by channel path hardware event contexts):

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
	...
	list_add_tail(&crw->next, &private->crw);
	queue_work(vfio_ccw_work_q, &private->crw_work);
	...
}

If a hardware event fires while userspace is concurrently removing an element
locklessly via list_del() in vfio_ccw_crw_region_read(), will the overlapping
list modifications corrupt the list?

>  
>  	mutex_lock(&private->io_mutex);
> +	if (i >= private->num_regions) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	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 45ec722d25ea6..032a1cdf4df70 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[Severity: High]
This is a pre-existing issue, but are private->io_work and private->crw_work
explicitly canceled before the private structure is freed, or can this lead
to a use-after-free?

During device teardown, vfio_ccw_mdev_release_dev() frees the private structure
without explicitly canceling io_work and crw_work using cancel_work_sync():

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
	struct vfio_ccw_private *private =
		container_of(vdev, struct vfio_ccw_private, vdev);
	...
	mutex_destroy(&private->io_mutex);
}

Although vfio_ccw_sch_quiesce() calls flush_workqueue(), it does so before
calling cio_disable_subchannel(). If an interrupt arrives after the flush but
before the hardware is disabled, it could re-queue the work. When private is
later freed, will the pending work execute on freed memory?

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

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

[PATCH v8 07/10] s390/vfio_ccw: cancel existing workqueues

[Eric Farman]

The initialization of the io_work and crw_work workqueues begs the
question of whether they should be un-initialized. Add the corresponding
cleanup tags in _release_dev to ensure work isn't dispatched after
the private struct is free'd.

Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
Fixes: e5f84dbaea59 ("vfio: ccw: return I/O results asynchronously")
Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 032a1cdf4df7..bc8eb485d03f 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -131,6 +131,13 @@ 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;
 
+	/*
+	 * Ensure these work items are fully drained, so none can
+	 * fire after being released.
+	 */
+	cancel_work_sync(&private->io_work);
+	cancel_work_sync(&private->crw_work);
+
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
 		kfree(crw);
@@ -202,6 +209,14 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+
+	/*
+	 * Ensure these work items are drained, in the event the
+	 * device is re-opened instead of released
+	 */
+	cancel_work_sync(&private->io_work);
+	cancel_work_sync(&private->crw_work);
+
 	vfio_ccw_unregister_dev_regions(private);
 }
 
-- 
2.53.0

Re: [PATCH v8 07/10] s390/vfio_ccw: cancel existing workqueues

[sashiko-bot]

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

Pre-existing issues:
- [High] Race condition between mdev_remove and CSS callbacks allows work to be queued after cancel_work_sync(), leading to a Use-After-Free.
- [High] Concurrent lockless modifications to the private->crw list cause list corruption.
- [High] Eventfd contexts for IRQ triggers are leaked upon device close and release.
- [High] Unsynchronized clearing of `private->completion` can lead to stack use-after-free in the interrupt handler.
--

--- Patch [7]: [PATCH v8 07/10] s390/vfio_ccw: cancel existing workqueues ---
commit 713bc2489172cb8218b7c8bfb484ab3caaed51a1
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: cancel existing workqueues

The initialization of the io_work and crw_work workqueues begs the
question of whether they should be un-initialized. Add the corresponding
cleanup tags in _release_dev to ensure work isn't dispatched after
the private struct is free'd.

Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
Fixes: e5f84dbaea59 ("vfio: ccw: return I/O results asynchronously")
Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..bc8eb485d03fd 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -131,6 +131,13 @@ 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;
>  
> +	/*
> +	 * Ensure these work items are fully drained, so none can
> +	 * fire after being released.
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);

[Severity: High]
This is a pre-existing issue, but does this fully prevent the work from
being dispatched after the private struct is freed?

If vfio_ccw_mdev_remove() clears drvdata without holding sch->lock:

vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    ...
}

And a concurrent hardware channel path event triggers vfio_ccw_chp_event()
which fetches the private pointer locklessly:

vfio_ccw_chp_event() {
    ...
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
}

Could vfio_ccw_chp_event() queue private->crw_work after this
cancel_work_sync() call has already finished, leading to a use-after-free
when private is subsequently freed?

> +
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate and
delete items from private->crw without holding a lock here?

In vfio_ccw_queue_crw(), elements are added to the list while holding
sch->lock:

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

However, vfio_ccw_crw_region_read() removes items without holding sch->lock
or any other synchronization:

vfio_ccw_crw_region_read() {
    ...
    if (crw)
        list_del(&crw->next);
    ...
}

Can concurrent asynchronous hardware events and userspace reads corrupt this
doubly-linked list?

>  		list_del(&crw->next);
>  		kfree(crw);

[ ... ]

> @@ -202,6 +209,14 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> +
> +	/*
> +	 * Ensure these work items are drained, in the event the
> +	 * device is re-opened instead of released
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this code leak the
eventfd contexts for the IRQ triggers?

Userspace sets IRQ triggers via vfio_ccw_mdev_set_irqs(), which takes a
reference to the eventfd_ctx.

If userspace exits or crashes without explicitly disabling the IRQs, neither
vfio_ccw_mdev_close_device() nor vfio_ccw_mdev_release_dev() appear to call
eventfd_ctx_put() for io_trigger, crw_trigger, or req_trigger.

[Severity: High]
This is a pre-existing issue, but can the unsynchronized clearing of
private->completion lead to a stack use-after-free in the interrupt handler?

In vfio_ccw_sch_quiesce(), if wait_for_completion_timeout() times out, it
clears the pointer without holding sch->lock:

vfio_ccw_sch_quiesce() {
    ...
    private->completion = NULL;
    ...
}

Concurrently, fsm_irq() might read the non-NULL pointer:

fsm_irq() {
    ...
    if (private->completion)
        complete(private->completion);
    ...
}

If quiesce returns and destroys the on-stack completion frame before
complete() executes, could the interrupt handler corrupt the stack?

>  	vfio_ccw_unregister_dev_regions(private);
>  }

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

Re: [PATCH v8 07/10] s390/vfio_ccw: cancel existing workqueues

[Matthew Rosato]

On 7/27/26 9:35 PM, Eric Farman wrote:
> The initialization of the io_work and crw_work workqueues begs the
> question of whether they should be un-initialized. Add the corresponding
> cleanup tags in _release_dev to ensure work isn't dispatched after
> the private struct is free'd.
> 
> Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Fixes: e5f84dbaea59 ("vfio: ccw: return I/O results asynchronously")
> Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
> 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_ops.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df7..bc8eb485d03f 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -131,6 +131,13 @@ 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;
>  
> +	/*
> +	 * Ensure these work items are fully drained, so none can
> +	 * fire after being released.
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
> @@ -202,6 +209,14 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> +
> +	/*
> +	 * Ensure these work items are drained, in the event the
> +	 * device is re-opened instead of released
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +
>  	vfio_ccw_unregister_dev_regions(private);
>  }
>  

[PATCH v8 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[Eric Farman]

The fsm_notoper() routine is called when the device has been
lost, and is (by definition) no longer operational. Since this
can happen asynchronously from the normal behavior of the
driver, the cleanup may happen when holding other locks
in the calling sequence (notably, the cio subchannel lock).

Push the cleanup of the private->cp resources to a workqueue,
where it can be done out from under that lock sequence and
safely under a common locking mechanism.

Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 9 +++++++++
 drivers/s390/cio/vfio_ccw_fsm.c     | 3 +--
 drivers/s390/cio/vfio_ccw_ops.c     | 6 +++++-
 drivers/s390/cio/vfio_ccw_private.h | 3 +++
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1a095085bc72..c197ad5ab580 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
 		eventfd_signal(private->crw_trigger);
 }
 
+void vfio_ccw_notoper_todo(struct work_struct *work)
+{
+	struct vfio_ccw_private *private;
+
+	private = container_of(work, struct vfio_ccw_private, notoper_work);
+
+	cp_free(&private->cp);
+}
+
 /*
  * Css driver callbacks
  */
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d7988ea47ef..4d47a3c7b9a0 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -170,8 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	css_sched_sch_todo(sch, SCH_TODO_UNREG);
 	private->state = VFIO_CCW_STATE_NOT_OPER;
 
-	/* This is usually handled during CLOSE event */
-	cp_free(&private->cp);
+	queue_work(vfio_ccw_work_q, &private->notoper_work);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index bc8eb485d03f..1cca3ecdae45 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -54,6 +54,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);
+	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -133,7 +134,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 
 	/*
 	 * Ensure these work items are fully drained, so none can
-	 * fire after being released.
+	 * fire after being released. The notoper_work struct is
+	 * only meaningful if the device had been opened, which
+	 * means it would have been cleaned in an earlier close.
 	 */
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
@@ -216,6 +219,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 	 */
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
+	flush_work(&private->notoper_work);
 
 	vfio_ccw_unregister_dev_regions(private);
 }
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0501d4bbcdbd..e2256402b089 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -102,6 +102,7 @@ struct vfio_ccw_parent {
  * @req_trigger: eventfd ctx for signaling userspace to return device
  * @io_work: work for deferral process of I/O handling
  * @crw_work: work for deferral process of CRW handling
+ * @notoper_work: work for deferred processing in not-operational state
  */
 struct vfio_ccw_private {
 	struct vfio_device vdev;
@@ -125,11 +126,13 @@ struct vfio_ccw_private {
 	struct eventfd_ctx	*req_trigger;
 	struct work_struct	io_work;
 	struct work_struct	crw_work;
+	struct work_struct	notoper_work;
 } __aligned(8);
 
 int vfio_ccw_sch_quiesce(struct subchannel *sch);
 void vfio_ccw_sch_io_todo(struct work_struct *work);
 void vfio_ccw_crw_todo(struct work_struct *work);
+void vfio_ccw_notoper_todo(struct work_struct *work);
 
 extern struct mdev_driver vfio_ccw_mdev_driver;
 
-- 
2.53.0

Re: [PATCH v8 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failure to cancel pending `notoper_work` during device release causes a workqueue leak and use-after-free on the `private` structure.
- [High] Missing `io_mutex` locking in `vfio_ccw_notoper_todo` leads to a data race with concurrent channel program operations.
--

--- Patch [8]: [PATCH v8 08/10] s390/vfio_ccw: move cp cleanup out of not operational ---
commit bf6c617fa515fe63ff2d80efc36831d26b7cdedc
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: move cp cleanup out of not operational

The fsm_notoper() routine is called when the device has been lost, and is
no longer operational. Push the cleanup of the private->cp resources to a
workqueue, where it can be done safely under a common locking mechanism.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  		eventfd_signal(private->crw_trigger);
>  }
>  
> +void vfio_ccw_notoper_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +
> +	private = container_of(work, struct vfio_ccw_private, notoper_work);
> +
> +	cp_free(&private->cp);
> +}
> +

[Severity: High]
The commit message mentions performing the cleanup safely under a common
locking mechanism. Could this lockless call to cp_free() lead to a regression
with concurrent channel program operations?

If userspace initiates an I/O request, vfio_ccw_mdev_write_io_region()
holds io_mutex and processes the request via fsm_io_request() and
cp_init() / cp_prefetch().

Concurrently, if a hardware event triggers fsm_notoper() and queues
notoper_work, this worker executes vfio_ccw_notoper_todo() and locklessly
calls cp_free(&private->cp).

Should this function acquire io_mutex to prevent a data race with the
allocation and initialization of ccwchain structures in the I/O path?

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -133,7 +134,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  
>  	/*
>  	 * Ensure these work items are fully drained, so none can
> -	 * fire after being released.
> +	 * fire after being released. The notoper_work struct is
> +	 * only meaningful if the device had been opened, which
> +	 * means it would have been cleaned in an earlier close.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);

[Severity: High]
Is the assumption that notoper_work is only meaningful if the device had
been opened always correct?

If a machine check or path loss occurs before the mediated device is opened,
vfio_ccw_sch_event() transitions the state from STANDBY to NOT_OPER via
fsm_notoper(), queuing notoper_work unconditionally.

Since the device was never opened, vfio_ccw_mdev_close_device() (which
flushes the work) is never called.

When the device is subsequently removed, this release function omits canceling
notoper_work. This could allow the workqueue to execute vfio_ccw_notoper_todo()
after the VFIO core frees the private structure.

Should cancel_work_sync(&private->notoper_work) be added here to prevent a
potential use-after-free on the private structure?

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

Re: [PATCH v8 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[Matthew Rosato]

On 7/27/26 9:35 PM, Eric Farman wrote:
> The fsm_notoper() routine is called when the device has been
> lost, and is (by definition) no longer operational. Since this
> can happen asynchronously from the normal behavior of the
> driver, the cleanup may happen when holding other locks
> in the calling sequence (notably, the cio subchannel lock).
> 
> Push the cleanup of the private->cp resources to a workqueue,
> where it can be done out from under that lock sequence and
> safely under a common locking mechanism.

Nit: you mention 'safely under a common locking mechanism' but that
doesn't actually happen until next patch.

Maybe add something like 'so a subsequent patch can...'

> 
> Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 9 +++++++++
>  drivers/s390/cio/vfio_ccw_fsm.c     | 3 +--
>  drivers/s390/cio/vfio_ccw_ops.c     | 6 +++++-
>  drivers/s390/cio/vfio_ccw_private.h | 3 +++
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc72..c197ad5ab580 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  		eventfd_signal(private->crw_trigger);
>  }
>  
> +void vfio_ccw_notoper_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +
> +	private = container_of(work, struct vfio_ccw_private, notoper_work);
> +
> +	cp_free(&private->cp);
> +}
> +
>  /*
>   * Css driver callbacks
>   */
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef..4d47a3c7b9a0 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,8 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
>  
> -	/* This is usually handled during CLOSE event */
> -	cp_free(&private->cp);
> +	queue_work(vfio_ccw_work_q, &private->notoper_work);
>  }
>  
>  /*
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index bc8eb485d03f..1cca3ecdae45 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -54,6 +54,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);
> +	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
>  
>  	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
>  	if (!private->cp.guest_cp)
> @@ -133,7 +134,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  
>  	/*
>  	 * Ensure these work items are fully drained, so none can
> -	 * fire after being released.
> +	 * fire after being released. The notoper_work struct is
> +	 * only meaningful if the device had been opened, which
> +	 * means it would have been cleaned in an earlier close.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> @@ -216,6 +219,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	flush_work(&private->notoper_work);

Sashiko found a path where you might never open/close, so I guess we
need to flush in both close and release after all?

I still think flush vs cancel to ensure that we process the cp_free()
work if it's pending else we risk leaking the cp resources.

I think this is the last issue of note, so this series is very close as
far as I'm concerned.

Thanks,
Matt

>  
>  	vfio_ccw_unregister_dev_regions(private);
>  }
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 0501d4bbcdbd..e2256402b089 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -102,6 +102,7 @@ struct vfio_ccw_parent {
>   * @req_trigger: eventfd ctx for signaling userspace to return device
>   * @io_work: work for deferral process of I/O handling
>   * @crw_work: work for deferral process of CRW handling
> + * @notoper_work: work for deferred processing in not-operational state
>   */
>  struct vfio_ccw_private {
>  	struct vfio_device vdev;
> @@ -125,11 +126,13 @@ struct vfio_ccw_private {
>  	struct eventfd_ctx	*req_trigger;
>  	struct work_struct	io_work;
>  	struct work_struct	crw_work;
> +	struct work_struct	notoper_work;
>  } __aligned(8);
>  
>  int vfio_ccw_sch_quiesce(struct subchannel *sch);
>  void vfio_ccw_sch_io_todo(struct work_struct *work);
>  void vfio_ccw_crw_todo(struct work_struct *work);
> +void vfio_ccw_notoper_todo(struct work_struct *work);
>  
>  extern struct mdev_driver vfio_ccw_mdev_driver;
>  

Re: [PATCH v8 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[Eric Farman]

On 7/27/26 10:08 PM, Matthew Rosato wrote:
> On 7/27/26 9:35 PM, Eric Farman wrote:
>> The fsm_notoper() routine is called when the device has been
>> lost, and is (by definition) no longer operational. Since this
>> can happen asynchronously from the normal behavior of the
>> driver, the cleanup may happen when holding other locks
>> in the calling sequence (notably, the cio subchannel lock).
>>
>> Push the cleanup of the private->cp resources to a workqueue,
>> where it can be done out from under that lock sequence and
>> safely under a common locking mechanism.
> 
> Nit: you mention 'safely under a common locking mechanism' but that
> doesn't actually happen until next patch.
> 
> Maybe add something like 'so a subsequent patch can...'
> 
>>
>> Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_drv.c     | 9 +++++++++
>>   drivers/s390/cio/vfio_ccw_fsm.c     | 3 +--
>>   drivers/s390/cio/vfio_ccw_ops.c     | 6 +++++-
>>   drivers/s390/cio/vfio_ccw_private.h | 3 +++
>>   4 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>> index 1a095085bc72..c197ad5ab580 100644
>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>> @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>>   		eventfd_signal(private->crw_trigger);
>>   }
>>   
>> +void vfio_ccw_notoper_todo(struct work_struct *work)
>> +{
>> +	struct vfio_ccw_private *private;
>> +
>> +	private = container_of(work, struct vfio_ccw_private, notoper_work);
>> +
>> +	cp_free(&private->cp);
>> +}
>> +
>>   /*
>>    * Css driver callbacks
>>    */
>> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
>> index 4d7988ea47ef..4d47a3c7b9a0 100644
>> --- a/drivers/s390/cio/vfio_ccw_fsm.c
>> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
>> @@ -170,8 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>>   	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>>   	private->state = VFIO_CCW_STATE_NOT_OPER;
>>   
>> -	/* This is usually handled during CLOSE event */
>> -	cp_free(&private->cp);
>> +	queue_work(vfio_ccw_work_q, &private->notoper_work);
>>   }
>>   
>>   /*
>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>> index bc8eb485d03f..1cca3ecdae45 100644
>> --- a/drivers/s390/cio/vfio_ccw_ops.c
>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> @@ -54,6 +54,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);
>> +	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
>>   
>>   	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
>>   	if (!private->cp.guest_cp)
>> @@ -133,7 +134,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>>   
>>   	/*
>>   	 * Ensure these work items are fully drained, so none can
>> -	 * fire after being released.
>> +	 * fire after being released. The notoper_work struct is
>> +	 * only meaningful if the device had been opened, which
>> +	 * means it would have been cleaned in an earlier close.
>>   	 */
>>   	cancel_work_sync(&private->io_work);
>>   	cancel_work_sync(&private->crw_work);
>> @@ -216,6 +219,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>>   	 */
>>   	cancel_work_sync(&private->io_work);
>>   	cancel_work_sync(&private->crw_work);
>> +	flush_work(&private->notoper_work);
> 
> Sashiko found a path where you might never open/close, so I guess we
> need to flush in both close and release after all?

Argh.

The distinction I overlooked is that while the device might not be 
opened, meaning there's no cp stuff to free, the fsm_notoper call will 
enqueue the workqueue itself anyway. It makes no distinction of whether 
there will be anything for the cp_free logic to actually do.

> 
> I still think flush vs cancel to ensure that we process the cp_free()
> work if it's pending else we risk leaking the cp resources.

I have been going back and forth, and agree flush is probably better. 
Seems wrong to just blindly cancel it, even if there -shouldn't- be 
anything to release.

> 
> I think this is the last issue of note, so this series is very close as
> far as I'm concerned.
> 
> Thanks,
> Matt
> 
>>   
>>   	vfio_ccw_unregister_dev_regions(private);
>>   }
>> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
>> index 0501d4bbcdbd..e2256402b089 100644
>> --- a/drivers/s390/cio/vfio_ccw_private.h
>> +++ b/drivers/s390/cio/vfio_ccw_private.h
>> @@ -102,6 +102,7 @@ struct vfio_ccw_parent {
>>    * @req_trigger: eventfd ctx for signaling userspace to return device
>>    * @io_work: work for deferral process of I/O handling
>>    * @crw_work: work for deferral process of CRW handling
>> + * @notoper_work: work for deferred processing in not-operational state
>>    */
>>   struct vfio_ccw_private {
>>   	struct vfio_device vdev;
>> @@ -125,11 +126,13 @@ struct vfio_ccw_private {
>>   	struct eventfd_ctx	*req_trigger;
>>   	struct work_struct	io_work;
>>   	struct work_struct	crw_work;
>> +	struct work_struct	notoper_work;
>>   } __aligned(8);
>>   
>>   int vfio_ccw_sch_quiesce(struct subchannel *sch);
>>   void vfio_ccw_sch_io_todo(struct work_struct *work);
>>   void vfio_ccw_crw_todo(struct work_struct *work);
>> +void vfio_ccw_notoper_todo(struct work_struct *work);
>>   
>>   extern struct mdev_driver vfio_ccw_mdev_driver;
>>   
> 

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

[Eric Farman]

The io_mutex was defined to serialize the io_regions, but then has
also sort of been associated with the I/O themselves because of
the close relationship they share.

With the handful of races that are possible, the choices are either to:
 A) expand the scope of io_mutex to close these remaining windows, or
 B) reduce the scope of io_mutex to just io_region, and introduce a new
    lock mechanism for the remaining I/O resources

This patch implements A, since B brings with it a lot more interactions
that would need to be tracked and kept in a correct hierarchy.

The biggest functional change is the introduction of a workqueue
element for the cp_free() called out of fsm_notoper(), which is could
be run in an interrupt context and thus cannot be acquiring mutexes.

Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 2 +-
 drivers/s390/cio/vfio_ccw_cp.c      | 8 +++++++-
 drivers/s390/cio/vfio_ccw_drv.c     | 6 ++++--
 drivers/s390/cio/vfio_ccw_fsm.c     | 5 +++++
 drivers/s390/cio/vfio_ccw_private.h | 3 ++-
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index f3015132d4b5..9269b54f5cfd 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);
 
-	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 5ef082b8289a..58722c4baa25 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;
 
+	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;
 }
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index c197ad5ab580..757ff5b2556e 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -91,6 +91,7 @@ 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->io_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,9 +99,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 			cp_is_finished = true;
 		}
 	}
-	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
-	mutex_unlock(&private->io_mutex);
 
 	/*
 	 * Reset to IDLE only if processing of a channel program
@@ -110,6 +109,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 	 */
 	if (cp_is_finished)
 		private->state = VFIO_CCW_STATE_IDLE;
+	mutex_unlock(&private->io_mutex);
 
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger);
@@ -131,7 +131,9 @@ void vfio_ccw_notoper_todo(struct work_struct *work)
 
 	private = container_of(work, struct vfio_ccw_private, notoper_work);
 
+	mutex_lock(&private->io_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->io_mutex);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d47a3c7b9a0..5fd94e9d5c61 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -170,6 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	css_sched_sch_todo(sch, SCH_TODO_UNREG);
 	private->state = VFIO_CCW_STATE_NOT_OPER;
 
+	/* This routine could be called from IRQ context, so defer */
 	queue_work(vfio_ccw_work_q, &private->notoper_work);
 }
 
@@ -409,7 +410,11 @@ static void fsm_close(struct vfio_ccw_private *private,
 
 	private->state = VFIO_CCW_STATE_STANDBY;
 	spin_unlock_irq(&sch->lock);
+
+	mutex_lock(&private->io_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->io_mutex);
+
 	return;
 
 err_unlock:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index e2256402b089..739121116ab6 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -88,7 +88,8 @@ struct vfio_ccw_parent {
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
  * @io_region: MMIO region to input/output I/O arguments/results
- * @io_mutex: protect against concurrent update of I/O regions
+ * @io_mutex: protect against concurrent update of I/O resources
+ *            and @cp lifecycle
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
  * @schib_region: MMIO region for SCHIB information
-- 
2.53.0

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

[sashiko-bot]

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

New issues:
- [High] The `notoper_work` work item is not canceled when the mediated device is released, leading to a Use-After-Free of the `vfio_ccw_private` structure and `io_mutex` corruption.

Pre-existing issues:
- [High] Incomplete synchronization of the `private->crw` list leads to a data race and potential list corruption between atomic writers and mutex-holding readers.
- [High] The device state (`private->state`) is vulnerable to corruption because the newly protected `io_mutex` critical sections race with lockless atomic context writers.
--

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

The io_mutex was defined to serialize the io_regions, but has also been
associated with the I/O themselves. This patch expands the scope of io_mutex
to close remaining race windows, and introduces a workqueue element for
cp_free() 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);
>  
> -	mutex_lock(&private->io_mutex);
>  	if (i >= private->num_regions) {
>  		ret = -EINVAL;
>  		goto out;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but does
this lock movement fully resolve the race on the private->crw list?

While this patch protects list deletion in vfio_ccw_crw_region_read() with
io_mutex, it appears the list can still be modified concurrently without
locks. For example, vfio_ccw_queue_crw() adds items to this list locklessly:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
	...
	crw->crw.rsid = rsid;

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

Because vfio_ccw_queue_crw() can be called from atomic contexts (like
vfio_ccw_chp_event()), it cannot acquire the sleepable io_mutex. Can this
result in list corruption between the lockless atomic producer and the
mutex-holding reader?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index c197ad5ab5805..757ff5b2556e3 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -91,6 +91,7 @@ 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->io_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,9 +99,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  			cp_is_finished = true;
>  		}
>  	}
> -	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
> -	mutex_unlock(&private->io_mutex);
>  
>  	/*
>  	 * Reset to IDLE only if processing of a channel program
> @@ -110,6 +109,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  	 */
>  	if (cp_is_finished)
>  		private->state = VFIO_CCW_STATE_IDLE;
> +	mutex_unlock(&private->io_mutex);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but could
this state update be overwritten by concurrent lockless modifications?

With the newly expanded io_mutex, vfio_ccw_sch_io_todo() reads and modifies
private->state under the lock. However, fsm_notoper() updates the state
without acquiring io_mutex:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_notoper() {
	...
	css_sched_sch_todo(sch, SCH_TODO_UNREG);
	private->state = VFIO_CCW_STATE_NOT_OPER;
	...
}

If a device goes non-operational concurrently with the completion of a
channel program, could vfio_ccw_sch_io_todo() overwrite the NOT_OPER state
with IDLE, discarding the transition?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d47a3c7b9a06..5fd94e9d5c618 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,6 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
>  
> +	/* This routine could be called from IRQ context, so defer */
>  	queue_work(vfio_ccw_work_q, &private->notoper_work);
>  }

[Severity: High]
Is it possible for private->notoper_work to outlive the private structure
and cause a use-after-free?

If a VFIO_CCW_EVENT_NOT_OPER event occurs while the device is in the STANDBY
state (for instance, from an offline channel path before the device is
opened), fsm_notoper() unconditionally queues notoper_work.

However, if the mediated device is then removed without ever being opened,
vfio_ccw_mdev_release_dev() appears to skip canceling this work:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
	...
	/*
	 * Ensure these work items are fully drained, so none can
	 * fire after being released. The notoper_work struct is
	 * only meaningful if the device had been opened, which
	 * means it would have been cleaned in an earlier close.
	 */
	cancel_work_sync(&private->io_work);
	cancel_work_sync(&private->crw_work);
	...
}

Because the notoper_work is never canceled here, could it execute after the
private structure is freed, leading to memory corruption when
vfio_ccw_notoper_todo() attempts to acquire io_mutex?

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

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

[Matthew Rosato]

On 7/27/26 9:35 PM, Eric Farman wrote:
> The io_mutex was defined to serialize the io_regions, but then has
> also sort of been associated with the I/O themselves because of
> the close relationship they share.
> 
> With the handful of races that are possible, the choices are either to:
>  A) expand the scope of io_mutex to close these remaining windows, or
>  B) reduce the scope of io_mutex to just io_region, and introduce a new
>     lock mechanism for the remaining I/O resources
> 
> This patch implements A, since B brings with it a lot more interactions
> that would need to be tracked and kept in a correct hierarchy.
> 
> The biggest functional change is the introduction of a workqueue
> element for the cp_free() called out of fsm_notoper(), which is could
> be run in an interrupt context and thus cannot be acquiring mutexes.

Nit: re-word "which is could be run"

Also the wording here kind of implies you added fsm_notoper() in this
patch, but that was last patch - here you just made it use the io_mutex.

Otherwise LGTM.

> 
> Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_chp.c     | 2 +-
>  drivers/s390/cio/vfio_ccw_cp.c      | 8 +++++++-
>  drivers/s390/cio/vfio_ccw_drv.c     | 6 ++++--
>  drivers/s390/cio/vfio_ccw_fsm.c     | 5 +++++
>  drivers/s390/cio/vfio_ccw_private.h | 3 ++-
>  5 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index f3015132d4b5..9269b54f5cfd 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);
>  
> -	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 5ef082b8289a..58722c4baa25 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;
>  
> +	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;
>  }
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index c197ad5ab580..757ff5b2556e 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -91,6 +91,7 @@ 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->io_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,9 +99,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  			cp_is_finished = true;
>  		}
>  	}
> -	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
> -	mutex_unlock(&private->io_mutex);
>  
>  	/*
>  	 * Reset to IDLE only if processing of a channel program
> @@ -110,6 +109,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  	 */
>  	if (cp_is_finished)
>  		private->state = VFIO_CCW_STATE_IDLE;
> +	mutex_unlock(&private->io_mutex);
>  
>  	if (private->io_trigger)
>  		eventfd_signal(private->io_trigger);
> @@ -131,7 +131,9 @@ void vfio_ccw_notoper_todo(struct work_struct *work)
>  
>  	private = container_of(work, struct vfio_ccw_private, notoper_work);
>  
> +	mutex_lock(&private->io_mutex);
>  	cp_free(&private->cp);
> +	mutex_unlock(&private->io_mutex);
>  }
>  
>  /*
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d47a3c7b9a0..5fd94e9d5c61 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,6 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
>  
> +	/* This routine could be called from IRQ context, so defer */
>  	queue_work(vfio_ccw_work_q, &private->notoper_work);
>  }
>  
> @@ -409,7 +410,11 @@ static void fsm_close(struct vfio_ccw_private *private,
>  
>  	private->state = VFIO_CCW_STATE_STANDBY;
>  	spin_unlock_irq(&sch->lock);
> +
> +	mutex_lock(&private->io_mutex);
>  	cp_free(&private->cp);
> +	mutex_unlock(&private->io_mutex);
> +
>  	return;
>  
>  err_unlock:
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index e2256402b089..739121116ab6 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -88,7 +88,8 @@ struct vfio_ccw_parent {
>   * @state: internal state of the device
>   * @completion: synchronization helper of the I/O completion
>   * @io_region: MMIO region to input/output I/O arguments/results
> - * @io_mutex: protect against concurrent update of I/O regions
> + * @io_mutex: protect against concurrent update of I/O resources
> + *            and @cp lifecycle
>   * @region: additional regions for other subchannel operations
>   * @cmd_region: MMIO region for asynchronous I/O commands other than START
>   * @schib_region: MMIO region for SCHIB information

[PATCH v8 10/10] 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 26 ++++++++++++++++----------
 drivers/s390/cio/vfio_ccw_drv.c     |  6 ++++++
 drivers/s390/cio/vfio_ccw_ops.c     |  4 ++++
 drivers/s390/cio/vfio_ccw_private.h |  4 ++++
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 9269b54f5cfd..7708eb4d6de0 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -93,18 +93,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
 	struct ccw_crw_region *region;
 	struct vfio_ccw_crw *crw;
+	unsigned long flags;
 	int ret;
 
 	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);
-
 	if (i >= private->num_regions) {
 		ret = -EINVAL;
 		goto out;
@@ -113,6 +108,16 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
+	spin_lock_irqsave(&private->crw_lock, flags);
+	crw = list_first_entry_or_null(&private->crw,
+				       struct vfio_ccw_crw, next);
+
+	if (crw)
+		list_del(&crw->next);
+
+	/* Drop CRW lock while copying to userspace */
+	spin_unlock_irqrestore(&private->crw_lock, flags);
+
 	if (crw)
 		memcpy(&region->crw, &crw->crw, sizeof(region->crw));
 
@@ -122,15 +127,16 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		ret = count;
 
 	region->crw = 0;
-
-out:
-	mutex_unlock(&private->io_mutex);
-
 	kfree(crw);
 
 	/* Notify the guest if more CRWs are on our queue */
+	spin_lock_irqsave(&private->crw_lock, flags);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock_irqrestore(&private->crw_lock, flags);
+
+out:
+	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 757ff5b2556e..ab6b518cc353 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -118,11 +118,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 void vfio_ccw_crw_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
+	unsigned long flags;
 
 	private = container_of(work, struct vfio_ccw_private, crw_work);
 
+	spin_lock_irqsave(&private->crw_lock, flags);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock_irqrestore(&private->crw_lock, flags);
 }
 
 void vfio_ccw_notoper_todo(struct work_struct *work)
@@ -286,6 +289,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 			       unsigned int rsid)
 {
 	struct vfio_ccw_crw *crw;
+	unsigned long flags;
 
 	/*
 	 * If unable to allocate a CRW, just drop the event and
@@ -303,7 +307,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 	crw->crw.erc = erc;
 	crw->crw.rsid = rsid;
 
+	spin_lock_irqsave(&private->crw_lock, flags);
 	list_add_tail(&crw->next, &private->crw);
+	spin_unlock_irqrestore(&private->crw_lock, flags);
 	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 1cca3ecdae45..4a0fe2f8fdb4 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -55,6 +55,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
+	spin_lock_init(&private->crw_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -131,6 +132,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 	struct vfio_ccw_crw *crw, *temp;
+	unsigned long flags;
 
 	/*
 	 * Ensure these work items are fully drained, so none can
@@ -141,10 +143,12 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
 
+	spin_lock_irqsave(&private->crw_lock, flags);
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
 		kfree(crw);
 	}
+	spin_unlock_irqrestore(&private->crw_lock, flags);
 
 	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 739121116ab6..3bd0171d38d0 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -98,6 +98,8 @@ struct vfio_ccw_parent {
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info
+ * @crw_lock: serialization of CRW list information
+ * @crw: list of Channel Report Word elements
  * @io_trigger: eventfd ctx for signaling userspace I/O results
  * @crw_trigger: eventfd ctx for signaling userspace CRW information
  * @req_trigger: eventfd ctx for signaling userspace to return device
@@ -120,6 +122,8 @@ struct vfio_ccw_private {
 	struct channel_program	cp;
 	struct irb		irb;
 	union scsw		scsw;
+
+	spinlock_t		crw_lock;
 	struct list_head	crw;
 
 	struct eventfd_ctx	*io_trigger;
-- 
2.53.0

Re: [PATCH v8 10/10] s390/vfio_ccw: implement a crw lock

[sashiko-bot]

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

New issues:
- [High] Missing cancellation of `notoper_work` during device release leads to Use-After-Free.

Pre-existing issues:
- [Critical] Lockless modification of eventfd triggers in `vfio_ccw_mdev_set_irqs` leads to Use-After-Free of `eventfd_ctx` in asynchronous workers.
- [High] Race condition between asynchronous CSS callbacks and mdev removal leads to Use-After-Free of the `private` structure.
--

--- Patch [10]: [PATCH v8 10/10] s390/vfio_ccw: implement a crw lock ---
commit e956337911477e5c5887e514827c14569954a32d
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a crw lock

This patch adds a lock to the vfio_ccw driver to manage the list of
asynchronous CRW events. It serializes access to the CRW payload list
to ensure cohesive reads between hardware events and userspace.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 757ff5b2556e3..ab6b518cc353f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> [ ... ]
> @@ -118,11 +118,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
> +	unsigned long flags;
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem with how
crw_trigger is accessed here.

Does private->crw_lock actually protect private->crw_trigger from
concurrent modification?

Looking at vfio_ccw_mdev_set_irqs(), it replaces and frees eventfd contexts
without holding any locks:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
    ...
    if (*ctx)
        eventfd_ctx_put(*ctx);

    *ctx = efdctx;
}

If the VFIO_DEVICE_SET_IRQS ioctl swaps and frees the eventfd_ctx while
this asynchronous worker is preparing to signal it, won't eventfd_signal()
dereference a freed pointer?

> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  }
> [ ... ]
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 1cca3ecdae45b..4a0fe2f8fdb44 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> [ ... ]
> @@ -131,6 +132,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	struct vfio_ccw_private *private =
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  	struct vfio_ccw_crw *crw, *temp;
> +	unsigned long flags;
>  
>  	/*
>  	 * Ensure these work items are fully drained, so none can
>  	 * fire after being released. The notoper_work struct is
>  	 * only meaningful if the device had been opened, which
>  	 * means it would have been cleaned in an earlier close.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);

[Severity: High]
The comment above suggests notoper_work is only meaningful if the device
had been opened, but could a race occur here?

If the device was never opened (state is VFIO_CCW_STATE_STANDBY), can an
asynchronous CSS callback still trigger a VFIO_CCW_EVENT_NOT_OPER event
(e.g., if the subchannel is unregistered)?

If this happens and queues notoper_work, and the device is subsequently
released, won't release_dev free the private structure while notoper_work
is still queued?

When the workqueue executes:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_notoper_todo() {
    ...
    mutex_lock(&private->io_mutex);
    cp_free(&private->cp);
    mutex_unlock(&private->io_mutex);
}

Won't this dereference the freed private pointer? Should we add
cancel_work_sync(&private->notoper_work) here?


[Severity: High]
This is a pre-existing issue, but since we are looking at the lifecycle of
the private structure, is there a race condition with asynchronous CSS
callbacks during mdev removal?

In vfio_ccw_sch_event(), the private pointer is fetched locklessly:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
    struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    unsigned long flags;
    ...
    spin_lock_irqsave(&sch->lock, flags);
}

If vfio_ccw_mdev_remove() runs concurrently, it clears the drvdata
locklessly and drops the device reference:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_remove() {
    vfio_unregister_group_dev(&private->vdev);
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
}

If the CSS callback is preempted after fetching the pointer but before
acquiring sch->lock, and resumes after the removal path completes, won't
it use the freed private structure?

> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
>  	}
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
> [ ... ]

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