[PATCH v2 0/7] s390/vfio_ccw fixes

[PATCH v2 0/7] s390/vfio_ccw fixes

[Eric Farman]

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

v1: https://lore.kernel.org/r/20260714232208.1683788-1-farman@linux.ibm.com/
v1->v2:
 - [sashiko] Reorder patches to put the cp_init and channel program segment
   pieces at the head of the series
 - [sashiko] Fix the out of bounds array check to break the loop correctly
 - [sashiko] Move the nospec clamp to the default case, rather than the
   entirety of the switch statement (to keep it in the default leg of the
   switch where it's used, rather than outside the entire switch)
 - [EF] Return -EINVAL if ccwchain count is exhausted, instead of -ENOMEM
 - [EF] Drop calc_max_idal_len() logic in place of a comparison against
   idaw[0] before/after the pair of vfio_dma_rw() calls 
 - [EF] Implement two new spin locks, one for struct channel_program in
   struct vfio_ccw_private, and one for the list of CRWs in struct
   vfio_ccw_private.

Eric Farman (7):
  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: ensure index for read/write regions are within range
  s390/vfio_ccw: implement a channel program lock
  s390/vfio_ccw: implement a crw lock

 drivers/s390/cio/vfio_ccw_chp.c     | 15 ++++---
 drivers/s390/cio/vfio_ccw_cp.c      | 67 +++++++++++++++++++++++++----
 drivers/s390/cio/vfio_ccw_cp.h      |  8 ++++
 drivers/s390/cio/vfio_ccw_drv.c     | 10 ++++-
 drivers/s390/cio/vfio_ccw_ops.c     | 13 +++++-
 drivers/s390/cio/vfio_ccw_private.h |  4 ++
 6 files changed, 101 insertions(+), 16 deletions(-)

-- 
2.53.0

[PATCH v2 1/7] 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
Cc: 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

[PATCH v2 2/7] 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..7abdebd8f7c3 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 ERR_PTR(-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

[PATCH v2 3/7] 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 7abdebd8f7c3..ce61858c82eb 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

[PATCH v2 4/7] 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 ce61858c82eb..c6f0264fb5d3 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

[PATCH v2 5/7] 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_ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 45ec722d25ea..e695cd985369 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);
 	}
-- 
2.53.0

[PATCH v2 6/7] s390/vfio_ccw: implement a channel program lock

[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 spin lock, which can be
acquired with or without the mutex guarding the I/O regions that
feed into the population of the channel program.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c      | 13 ++++++++++++-
 drivers/s390/cio/vfio_ccw_cp.h      |  1 +
 drivers/s390/cio/vfio_ccw_drv.c     |  6 +++++-
 drivers/s390/cio/vfio_ccw_ops.c     |  8 +++++++-
 drivers/s390/cio/vfio_ccw_private.h |  2 ++
 5 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index c6f0264fb5d3..7bfb3a952817 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -790,7 +790,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
  * @cp, which must have been returned by a previous call to cp_init().
  * Otherwise, undefined behavior occurs.
  */
-void cp_free(struct channel_program *cp)
+void cp_free_locked(struct channel_program *cp)
 {
 	struct vfio_device *vdev =
 		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
@@ -810,6 +810,17 @@ void cp_free(struct channel_program *cp)
 	}
 }
 
+void cp_free(struct channel_program *cp)
+{
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
+	unsigned long flags;
+
+	spin_lock_irqsave(&private->cp_lock, flags);
+	cp_free_locked(cp);
+	spin_unlock_irqrestore(&private->cp_lock, flags);
+}
+
 /**
  * cp_prefetch() - translate a guest physical address channel program to
  *                 a real-device runnable channel program.
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index d547dbc969cc..edfb47a292ca 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -48,6 +48,7 @@ struct channel_program {
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
+void cp_free_locked(struct channel_program *cp);
 void cp_free(struct channel_program *cp);
 int cp_prefetch(struct channel_program *cp);
 union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch);
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1a095085bc72..bfb68de6e52c 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -83,6 +83,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
 	struct irb *irb;
+	unsigned long flags;
 	bool is_final;
 	bool cp_is_finished = false;
 
@@ -91,13 +92,16 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 
 	is_final = !(scsw_actl(&irb->scsw) &
 		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
+	spin_lock_irqsave(&private->cp_lock, flags);
 	if (scsw_is_solicited(&irb->scsw)) {
 		cp_update_scsw(&private->cp, &irb->scsw);
 		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
-			cp_free(&private->cp);
+			cp_free_locked(&private->cp);
 			cp_is_finished = true;
 		}
 	}
+	spin_unlock_irqrestore(&private->cp_lock, flags);
+
 	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 	mutex_unlock(&private->io_mutex);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index e695cd985369..d2475b3158cf 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -36,10 +36,15 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
 {
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
+	unsigned long flags;
 
 	/* Drivers MUST unpin pages in response to an invalidation. */
-	if (!cp_iova_pinned(&private->cp, iova, length))
+	spin_lock_irqsave(&private->cp_lock, flags);
+	if (!cp_iova_pinned(&private->cp, iova, length)) {
+		spin_unlock_irqrestore(&private->cp_lock, flags);
 		return;
+	}
+	spin_unlock_irqrestore(&private->cp_lock, flags);
 
 	vfio_ccw_mdev_reset(private);
 }
@@ -54,6 +59,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->cp_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0501d4bbcdbd..061b594fde36 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -115,7 +115,9 @@ struct vfio_ccw_private {
 	struct ccw_crw_region	*crw_region;
 	int num_regions;
 
+	spinlock_t		cp_lock;
 	struct channel_program	cp;
+
 	struct irb		irb;
 	union scsw		scsw;
 	struct list_head	crw;
-- 
2.53.0

[PATCH v2 7/7] 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.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 15 ++++++++++-----
 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, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 38c176cf6295..bbc445efd67f 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -90,14 +90,18 @@ 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);
+	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);
-	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));
@@ -108,14 +112,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 bfb68de6e52c..12edccb62cbf 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);
 }
 
 /*
@@ -296,7 +298,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 d2475b3158cf..b3699ffd2f1d 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_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 	spin_lock_init(&private->cp_lock);
+	spin_lock_init(&private->crw_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -137,10 +138,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 061b594fde36..4a43609d6422 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -120,6 +120,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 v2 1/7] s390/vfio_ccw: free all memory if cp_init() fails

[Farhan Ali]

On 7/20/2026 1:19 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
> Cc: 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;

The fix looks correct to me and I don't think Sashiko found any issues 
with this patch (issues identified as pre-existing and i think something 
this series is trying to fix).

Reviewed-by: Farhan Ali<alifm@linux.ibm.com>

Thanks

Farhan

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

[Eric Farman]

On 7/20/26 5:48 PM, Farhan Ali wrote:
> 
> On 7/20/2026 1:19 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
>> Cc: 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;
> 
> The fix looks correct to me and I don't think Sashiko found any issues 
> with this patch (issues identified as pre-existing and i think something 
> this series is trying to fix).

Correct, the two pre-existing findings reported against this patch 
should be addressed by patches 2 and 3, respectively, in this series.

> 
> Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
> 

Thank you!

> Thanks
> 
> Farhan
> 
> 
>