[PATCH v2 0/9] KVM: s390: And then... even more fixes again

[PATCH v2 0/9] KVM: s390: And then... even more fixes again

[Claudio Imbrenda]

Another round of mostly unrelated misc fixes.

Mostly small and/or unlikely issues, but they needed to be addressed.

v1->v2:
* Use the srcu only around kvm_s390_inject_vcpu()
* Wildly simplify the third patch: introduce gisa_test_ipm_gisc() and
  use it in get_all_floating_irqs() instead of gisa_tac_ipm_gisc()
* Use set_page_dirty() instead of set_page_dirty_lock(). In all
  scenarios we are holding an extra reference to the page, we don't
  need to also lock. Also move things around a little to avoid races.
* Fix return value of kvm_arch_prepare_memory_region()
* Fix return value being overwritten in patch 8
* Drop patches 6 and 7, they need more thought

Claudio Imbrenda (9):
  KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key()
  KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl()
  KVM: s390: Fix get_all_floating_irqs()
  KVM: s390: Fix dirty marking in adapter_indicators_set*()
  KVM: s390: Fix pgste_get_trylock_multiple()
  KVM: s390: Fix IRQ injection with SIGP Stop and Store Status
  KVM: s390: Fix kvm_s390_clear_pv_state()
  KVM: s390: Fix potential tiny kernel stack leak
  KVM: s390: Fix _gaccess_shadow_fault()

 arch/s390/kvm/dat.c       |  13 ++--
 arch/s390/kvm/gaccess.c   |  10 +++
 arch/s390/kvm/interrupt.c | 125 ++++++++++++++++++++++----------------
 arch/s390/kvm/kvm-s390.c  |   6 +-
 arch/s390/kvm/pv.c        |   4 ++
 5 files changed, 96 insertions(+), 62 deletions(-)

-- 
2.55.0

[PATCH v2 1/9] KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key()

[Claudio Imbrenda]

Some callers pass NULL as oldkey. Calling page_cond_set_storage_key()
will cause that NULL pointer to get dereferenced.

Fix by checking for NULL and assigning the pointer to a dummy local
variable to avoid crashes.

Fixes: 8e03e8316eb2 ("KVM: s390: KVM page table management functions: storage keys")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/dat.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 3f2d6e8902d7..165c704fcf29 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -722,9 +722,12 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
 	if (rc)
 		return rc;
 
-	if (!ptep)
+	if (!ptep) {
+		if (!oldkey)
+			oldkey = &prev;
 		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
 						 nq, mr, mc);
+	}
 
 	old = pgste_get_lock(ptep);
 	pgste = old;
-- 
2.55.0

[PATCH v2 2/9] KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl()

[Claudio Imbrenda]

kvm_arch_vcpu_unlocked_ioctl() is called without further locks held, but
kvm_s390_inject_vcpu(), which is called from there, needs either the
kvm->srcu or the slots lock.

Fix by taking the kvm->srcu in kvm_arch_vcpu_unlocked_ioctl().

Fixes: ba5c1e9b6cee ("KVM: s390: interrupt subsystem, cpu timer, waitpsw")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 518a69c55e85..b35340642c3e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5450,7 +5450,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
 
 		if (copy_from_user(&s390irq, argp, sizeof(s390irq)))
 			return -EFAULT;
-		rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
+		scoped_guard(srcu, &vcpu->kvm->srcu)
+			rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
 		break;
 	}
 	case KVM_S390_INTERRUPT: {
@@ -5463,7 +5464,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
 			return -EFAULT;
 		if (s390int_to_s390irq(&s390int, &s390irq))
 			return -EINVAL;
-		rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
+		scoped_guard(srcu, &vcpu->kvm->srcu)
+			rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
 		break;
 	}
 	default:
-- 
2.55.0

[PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs()

[Claudio Imbrenda]

When attempting to report all pending floating interrupt to userspace,
the GISA IPM bits are atomically tested and cleared, and the
corresponding interrupt description is written in the output buffer. If
the output buffer is too small, an error is returned to userspace, but
the GISA IPM bits are now lost.

Moreover, the contract of KVM_DEV_FLIC_GET_ALL_IRQS, which is the only
path to get_all_floating_irqs(), states that:

> All interrupts remain pending, i.e. are not deleted from the list of
> currently pending interrupts.

Fix by non-destructively testing for the GISA IPM bits.

Fixes: 24160af6cb28 ("KVM: s390: add GISA interrupts to FLIC ioctl interface")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 6b3f97a7513b..61e75d10110e 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -273,6 +273,11 @@ static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
 }
 
+static inline int gisa_test_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
+{
+	return test_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *)gisa);
+}
+
 static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
 {
 	unsigned long pending = vcpu->kvm->arch.float_int.pending_irqs |
@@ -2242,7 +2247,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
 				ret = -ENOMEM;
 				goto out_nolock;
 			}
-			if (gisa_tac_ipm_gisc(gi->origin, i)) {
+			if (gisa_test_ipm_gisc(gi->origin, i)) {
 				irq = (struct kvm_s390_irq *) &buf[n];
 				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
 				irq->u.io.io_int_word = isc_to_int_word(i);
-- 
2.55.0

[PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*()

[Claudio Imbrenda]

When the indicator and/or summary bits are set in the guest, the
accessed page was only marked dirty if the access was performed using
the slow path; accesses through the new kvm_arch_set_irq_inatomic fast
inject path would not mark the page as dirty.

Fix by adding/moving the missing calls to set_page_dirty() and
mark_page_dirty().

Opportunistically reorder the local variables to be in reverse
Christmas tree order.

Fixes: 1e95e3bc6b05 ("KVM: s390: Enable adapter_indicators_set to use mapped pages")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 48 ++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 61e75d10110e..8e4b88bce31f 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2953,12 +2953,14 @@ static int adapter_indicators_set(struct kvm *kvm,
 				  struct s390_io_adapter *adapter,
 				  struct kvm_s390_adapter_int *adapter_int)
 {
-	unsigned long bit;
-	int summary_set, idx;
 	struct s390_map_info *ind_info, *summary_info;
-	void *map;
 	struct page *ind_page, *summary_page;
 	unsigned long flags;
+	unsigned long bit;
+	int summary_set;
+	void *map;
+
+	guard(srcu)(&kvm->srcu);
 
 	ind_page = NULL;
 
@@ -2969,21 +2971,20 @@ static int adapter_indicators_set(struct kvm *kvm,
 		ind_page = pin_map_page(kvm, adapter_int->ind_addr, 0);
 		if (!ind_page)
 			return -1;
-		idx = srcu_read_lock(&kvm->srcu);
 		map = page_address(ind_page);
 		bit = get_ind_bit(adapter_int->ind_addr,
 				  adapter_int->ind_offset, adapter->swap);
 		set_bit(bit, map);
-		mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
-		set_page_dirty_lock(ind_page);
-		srcu_read_unlock(&kvm->srcu, idx);
+		set_page_dirty(ind_page);
 		unpin_user_page(ind_page);
 	} else {
 		map = page_address(ind_info->page);
 		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
 		set_bit(bit, map);
+		set_page_dirty(ind_info->page);
 		spin_unlock_irqrestore(&adapter->maps_lock, flags);
 	}
+	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
 
 	spin_lock_irqsave(&adapter->maps_lock, flags);
 	summary_info = get_map_info(adapter, adapter_int->summary_addr);
@@ -2992,22 +2993,21 @@ static int adapter_indicators_set(struct kvm *kvm,
 		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
 		if (WARN_ON_ONCE(!summary_page))
 			return -1;
-		idx = srcu_read_lock(&kvm->srcu);
 		map = page_address(summary_page);
 		bit = get_ind_bit(adapter_int->summary_addr,
 				  adapter_int->summary_offset, adapter->swap);
 		summary_set = test_and_set_bit(bit, map);
-		mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
-		set_page_dirty_lock(summary_page);
-		srcu_read_unlock(&kvm->srcu, idx);
+		set_page_dirty(summary_page);
 		unpin_user_page(summary_page);
 	} else {
 		map = page_address(summary_info->page);
 		bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
 				  adapter->swap);
 		summary_set = test_and_set_bit(bit, map);
+		set_page_dirty(summary_info->page);
 		spin_unlock_irqrestore(&adapter->maps_lock, flags);
 	}
+	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
 
 	return summary_set ? 0 : 1;
 }
@@ -3017,26 +3017,30 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
 				       struct kvm_s390_adapter_int *adapter_int,
 				       int setbit)
 {
+	struct s390_map_info *ind_info, *summary_info;
 	unsigned long bit;
 	int summary_set;
-	struct s390_map_info *ind_info, *summary_info;
 	void *map;
 
-	spin_lock(&adapter->maps_lock);
+	guard(srcu)(&kvm->srcu);
+	guard(spinlock)(&adapter->maps_lock);
+
 	ind_info = get_map_info(adapter, adapter_int->ind_addr);
-	if (!ind_info) {
-		spin_unlock(&adapter->maps_lock);
+	if (!ind_info)
 		return -EWOULDBLOCK;
-	}
+
 	map = page_address(ind_info->page);
 	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
-	if (setbit)
+	if (setbit) {
+		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
+		set_page_dirty(ind_info->page);
 		set_bit(bit, map);
+	}
+
 	summary_info = get_map_info(adapter, adapter_int->summary_addr);
-	if (!summary_info) {
-		spin_unlock(&adapter->maps_lock);
+	if (!summary_info)
 		return -EWOULDBLOCK;
-	}
+
 	map = page_address(summary_info->page);
 	bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
 			  adapter->swap);
@@ -3046,7 +3050,9 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
 		summary_set = test_and_set_bit(bit, map);
 	else
 		summary_set = test_and_clear_bit(bit, map);
-	spin_unlock(&adapter->maps_lock);
+	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
+	set_page_dirty(summary_info->page);
+
 	return summary_set ? 0 : 1;
 }
 
-- 
2.55.0

[PATCH v2 5/9] KVM: s390: Fix pgste_get_trylock_multiple()

[Claudio Imbrenda]

In case of failure, pgste_get_trylock_multiple() will attempt to unlock
the locked PGSTEs based on whether the PCL is set. In some
circumstances this can lead to unlocking PGSTEs that were locked by
other threads.

Fix by unlocking the amount of PGSTEs that were actually locked,
ignoring the PCL bit in the array.

Fixes: 94fd9b16cc67 ("KVM: s390: KVM page table management functions: lifecycle management")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/dat.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 165c704fcf29..f4dd6f783417 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -923,11 +923,8 @@ static void pgste_set_unlock_multiple(union pte *first, int n, union pgste *pgst
 {
 	int i;
 
-	for (i = 0; i < n; i++) {
-		if (!pgstes[i].pcl)
-			break;
+	for (i = 0; i < n; i++)
 		pgste_set_unlock(first + i, pgstes[i]);
-	}
 }
 
 static bool pgste_get_trylock_multiple(union pte *first, int n, union pgste *pgstes)
@@ -940,7 +937,7 @@ static bool pgste_get_trylock_multiple(union pte *first, int n, union pgste *pgs
 	}
 	if (i == n)
 		return true;
-	pgste_set_unlock_multiple(first, n, pgstes);
+	pgste_set_unlock_multiple(first, i, pgstes);
 	return false;
 }
 
-- 
2.55.0

[PATCH v2 6/9] KVM: s390: Fix IRQ injection with SIGP Stop and Store Status

[Claudio Imbrenda]

When __inject_sigp_stop() is called for a Stop and Store Status
operation, if the vCPU is running, the interrupt is marked as pending
and the status is stored by the thread performing the KVM_RUN IOCTL.

If the vCPU is already stopped, the status is stored immediately.

Storing the status means writing into userspace, which might fault, and
__inject_sigp_stop() is called from do_inject_vcpu() which in turn is
always called holding a spinlock, which is obviously an issue.

Fix this by returning -EWOULDBLOCK from __inject_sigp_stop(), and
adding a bool flag to indicate whether a store status is needed. The
callers of do_inject_vcpu() are modified to pass the pointer to the
bool flag; whenever a Store Status operation is needed, the callers can
now perform it outside the spinlock.

Opportunistically refactor kvm_s390_set_irq_state() to use
scoped_guard() and __free().

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 70 +++++++++++++++++++++------------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 8e4b88bce31f..6940f4d354e5 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1555,23 +1555,21 @@ static int __inject_set_prefix(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
 }
 
 #define KVM_S390_STOP_SUPP_FLAGS (KVM_S390_STOP_FLAG_STORE_STATUS)
-static int __inject_sigp_stop(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
+static int __inject_sigp_stop(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq, bool *storestatus)
 {
 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
 	struct kvm_s390_stop_info *stop = &li->irq.stop;
-	int rc = 0;
 
 	vcpu->stat.inject_stop_signal++;
 	trace_kvm_s390_inject_vcpu(vcpu->vcpu_id, KVM_S390_SIGP_STOP, 0, 0);
 
 	if (irq->u.stop.flags & ~KVM_S390_STOP_SUPP_FLAGS)
 		return -EINVAL;
-
 	if (is_vcpu_stopped(vcpu)) {
-		if (irq->u.stop.flags & KVM_S390_STOP_FLAG_STORE_STATUS)
-			rc = kvm_s390_store_status_unloaded(vcpu,
-						KVM_S390_STORE_STATUS_NOADDR);
-		return rc;
+		if (!(irq->u.stop.flags & KVM_S390_STOP_FLAG_STORE_STATUS))
+			return 0;
+		*storestatus = true;
+		return -EWOULDBLOCK;
 	}
 
 	if (test_and_set_bit(IRQ_PEND_SIGP_STOP, &li->pending_irqs))
@@ -2107,7 +2105,7 @@ void kvm_s390_clear_stop_irq(struct kvm_vcpu *vcpu)
 	spin_unlock(&li->lock);
 }
 
-static int do_inject_vcpu(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
+static int do_inject_vcpu(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq, bool *storestatus)
 {
 	int rc;
 
@@ -2119,7 +2117,7 @@ static int do_inject_vcpu(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
 		rc = __inject_set_prefix(vcpu, irq);
 		break;
 	case KVM_S390_SIGP_STOP:
-		rc = __inject_sigp_stop(vcpu, irq);
+		rc = __inject_sigp_stop(vcpu, irq, storestatus);
 		break;
 	case KVM_S390_RESTART:
 		rc = __inject_sigp_restart(vcpu);
@@ -2155,11 +2153,16 @@ static int do_inject_vcpu(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
 int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu, struct kvm_s390_irq *irq)
 {
 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
+	bool storestatus = false;
 	int rc;
 
 	spin_lock(&li->lock);
-	rc = do_inject_vcpu(vcpu, irq);
+	rc = do_inject_vcpu(vcpu, irq, &storestatus);
 	spin_unlock(&li->lock);
+
+	if (rc == -EWOULDBLOCK && storestatus)
+		rc = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);
+
 	if (!rc)
 		kvm_s390_vcpu_wakeup(vcpu);
 	return rc;
@@ -3180,7 +3183,8 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, struct kvm *kvm,
 int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len)
 {
 	struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
-	struct kvm_s390_irq *buf;
+	struct kvm_s390_irq *buf __free(kvfree) = NULL;
+	bool tmp, storestatus = false;
 	int r = 0;
 	int n;
 
@@ -3188,31 +3192,33 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
 	if (!buf)
 		return -ENOMEM;
 
-	if (copy_from_user((void *) buf, irqstate, len)) {
-		r = -EFAULT;
-		goto out_free;
-	}
+	if (copy_from_user((void *)buf, irqstate, len))
+		return -EFAULT;
 
-	/*
-	 * Don't allow setting the interrupt state
-	 * when there are already interrupts pending
-	 */
-	spin_lock(&li->lock);
-	if (li->pending_irqs) {
-		r = -EBUSY;
-		goto out_unlock;
-	}
+	scoped_guard(spinlock, &li->lock) {
+		/*
+		 * Don't allow setting the interrupt state
+		 * when there are already interrupts pending
+		 */
+		if (li->pending_irqs)
+			return -EBUSY;
 
-	for (n = 0; n < len / sizeof(*buf); n++) {
-		r = do_inject_vcpu(vcpu, &buf[n]);
-		if (r)
-			break;
+		for (n = 0; n < len / sizeof(*buf); n++) {
+			tmp = false;
+			r = do_inject_vcpu(vcpu, &buf[n], &tmp);
+			if (r == -EWOULDBLOCK && tmp) {
+				storestatus = true;
+				r = 0;
+			}
+			if (r)
+				break;
+		}
 	}
 
-out_unlock:
-	spin_unlock(&li->lock);
-out_free:
-	vfree(buf);
+	if (storestatus) {
+		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);
+		return r ? r : n;
+	}
 
 	return r;
 }
-- 
2.55.0

[PATCH v2 7/9] KVM: s390: Fix kvm_s390_clear_pv_state()

[Claudio Imbrenda]

kvm_s390_clear_pv_state() needs to also clear the dumping flag, to
allow the protected VM to be started again (as non-protected, with all
protected state safely destroyed) after a forced reboot while a
protected dump was ongoing and not completed.

Fixes: e40df9efd68a ("KVM: s390: pv: clear the state without memset")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/pv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index b02e0159d3cd..98a9a57f71b9 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -242,6 +242,10 @@ static void kvm_s390_clear_pv_state(struct kvm *kvm)
 	kvm->arch.pv.guest_len = 0;
 	kvm->arch.pv.stor_base = 0;
 	kvm->arch.pv.stor_var = NULL;
+	if (kvm->arch.pv.dumping) {
+		kvm_s390_vcpu_unblock_all(kvm);
+		kvm->arch.pv.dumping = false;
+	}
 }
 
 static void kvm_s390_pv_dispose_cpu(struct kvm_vcpu *vcpu, bool free_stor_base)
-- 
2.55.0

[PATCH v2 8/9] KVM: s390: Fix potential tiny kernel stack leak

[Claudio Imbrenda]

In some circumstances, one bit of kernel stack could have been leaked
from dat_cond_set_storage_key().

Fix by clearing prev before use.

Fixes: 8e03e8316eb2 ("KVM: s390: KVM page table management functions: storage keys")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 arch/s390/kvm/dat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index f4dd6f783417..f2ea013cb33e 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -737,6 +737,7 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
 	pgste.fp = skey.fp;
 	pgste.gc = skey.c;
 	pgste.gr = skey.r;
+	prev.skey = 0;
 
 	if (!ptep->h.i) {
 		rc = page_cond_set_storage_key(pte_origin(*ptep), skey, &prev, nq, mr, mc);
-- 
2.55.0

[PATCH v2 9/9] KVM: s390: Fix _gaccess_shadow_fault()

[Claudio Imbrenda]

In some circumstances, it is possible that the page of nested guest
memory that is being shadowed is not present at all in the parent guest
gmap. dat_entry_walk() will not find any leaf entry and return with
-ENOENT, which will erroneously be propagated all the way to userspace.

Fix by manually calling gmap_link() on the memory of the nested guest
that is being shadowed if the mapping was not already present.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/gaccess.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 36102b2727fb..f3a889988cdb 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -1507,6 +1507,7 @@ static int _do_shadow_crste(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gpa_
 static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
 			      unsigned long saddr, struct pgtwalk *w)
 {
+	struct kvm_memory_slot *slot;
 	struct guest_fault *entries;
 	int flags, i, hl, gl, l, rc;
 	union crste *table, *host;
@@ -1551,8 +1552,17 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
 			return -EAGAIN;
 	}
 
+retry:
 	rc = dat_entry_walk(NULL, entries[LEVEL_MEM].gfn, sg->parent->asce, DAT_WALK_LEAF,
 			    TABLE_TYPE_PAGE_TABLE, &host, &ptep_h);
+	if (rc == -ENOENT) {
+		slot = gfn_to_memslot(sg->kvm, entries[LEVEL_MEM].gfn);
+		if (!slot)
+			return PGM_ADDRESSING;
+		rc = gmap_link(mc, sg->parent, entries + LEVEL_MEM, slot);
+		if (!rc)
+			goto retry;
+	}
 	if (rc)
 		return rc;
 
-- 
2.55.0

Re: [PATCH v2 5/9] KVM: s390: Fix pgste_get_trylock_multiple()

[sashiko-bot]

> In case of failure, pgste_get_trylock_multiple() will attempt to unlock
> the locked PGSTEs based on whether the PCL is set. In some
> circumstances this can lead to unlocking PGSTEs that were locked by
> other threads.
> 
> Fix by unlocking the amount of PGSTEs that were actually locked,
> ignoring the PCL bit in the array.
> 
> Fixes: 94fd9b16cc67 ("KVM: s390: KVM page table management functions: lifecycle management")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

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

Re: [PATCH v2 8/9] KVM: s390: Fix potential tiny kernel stack leak

[sashiko-bot]

> In some circumstances, one bit of kernel stack could have been leaked
> from dat_cond_set_storage_key().
> 
> Fix by clearing prev before use.
> 
> Fixes: 8e03e8316eb2 ("KVM: s390: KVM page table management functions: storage keys")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

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

Re: [PATCH v2 7/9] KVM: s390: Fix kvm_s390_clear_pv_state()

[sashiko-bot]

> kvm_s390_clear_pv_state() needs to also clear the dumping flag, to
> allow the protected VM to be started again (as non-protected, with all
> protected state safely destroyed) after a forced reboot while a
> protected dump was ongoing and not completed.
> 
> Fixes: e40df9efd68a ("KVM: s390: pv: clear the state without memset")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

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

Re: [PATCH v2 6/9] KVM: s390: Fix IRQ injection with SIGP Stop and Store Status

[sashiko-bot]

> When __inject_sigp_stop() is called for a Stop and Store Status
> operation, if the vCPU is running, the interrupt is marked as pending
> and the status is stored by the thread performing the KVM_RUN IOCTL.
> 
> If the vCPU is already stopped, the status is stored immediately.
> 
> Storing the status means writing into userspace, which might fault, and
> __inject_sigp_stop() is called from do_inject_vcpu() which in turn is
> always called holding a spinlock, which is obviously an issue.
> 
> Fix this by returning -EWOULDBLOCK from __inject_sigp_stop(), and
> adding a bool flag to indicate whether a store status is needed. The
> callers of do_inject_vcpu() are modified to pass the pointer to the
> bool flag; whenever a Store Status operation is needed, the callers can
> now perform it outside the spinlock.
> 
> Opportunistically refactor kvm_s390_set_irq_state() to use
> scoped_guard() and __free().
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

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

Re: [PATCH v2 1/9] KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key()

[sashiko-bot]

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

Pre-existing issues:
- [High] Storage key reference and change bits are lost when executing SSKE with MR (Merge Reference) or MC (Merge Change) flags.
- [High] Race condition when updating hardware storage keys on huge pages (lockless read-modify-write).
--

--- Patch [1]: [PATCH v2 1/9] KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key() ---
commit 4e6ac296606202ca3143d3677ec8bf9361312ec5
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Properly handle NULL pointer in dat_cond_set_storage_key()

Some callers pass NULL as oldkey. This commit checks for NULL and assigns 
the pointer to a dummy local variable before calling 
page_cond_set_storage_key(), preventing a NULL pointer dereference.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 3f2d6e8902d76..165c704fcf299 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -722,9 +722,12 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
>  	if (rc)
>  		return rc;
>  
> -	if (!ptep)
> +	if (!ptep) {
> +		if (!oldkey)
> +			oldkey = &prev;
>  		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
>  						 nq, mr, mc);
> +	}

[Severity: High]
This is a pre-existing issue, but does calling page_cond_set_storage_key()
for huge pages lack synchronization?

For regular pages, this function acquires pgste_get_lock() below to
serialize updates. However, for huge pages, page_cond_set_storage_key() is
called directly without a per-page lock. 

Since page_cond_set_storage_key() performs a non-atomic read-modify-write by
executing page_get_storage_key() and later conditionally updating it with
page_set_storage_key(), can concurrent executions interleave and cause lost
storage key updates or lost change bits?

[Severity: High]
This is also a pre-existing issue, but do we lose the guest's reference and
change bits when the merge reference (mr) or merge change (mc) flags are
true?

In page_cond_set_storage_key(), if the new key differs from the old key, it
calls:

    page_set_storage_key(paddr, skey.skey, !nq);

The skey parameter does not logically OR the old reference and change bits
from oldkey when mr or mc are true. Since this writes directly to hardware,
won't the old hardware bits be permanently overwritten and lost?

Additionally, lower down in dat_cond_set_storage_key(), the software
tracking bits are unconditionally overwritten:

    pgste.gc = skey.c;
    pgste.gr = skey.r;

Could these assignments also drop the old.gc and old.gr bits when the merge
flags are set, potentially leading to a situation where a guest OS discards
a dirty page believing it to be clean?

>  
>  	old = pgste_get_lock(ptep);
>  	pgste = old;

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

Re: [PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs()

[Christian Borntraeger]

Am 12.08.26 um 12:44 schrieb Claudio Imbrenda:
> When attempting to report all pending floating interrupt to userspace,
> the GISA IPM bits are atomically tested and cleared, and the
> corresponding interrupt description is written in the output buffer. If
> the output buffer is too small, an error is returned to userspace, but
> the GISA IPM bits are now lost.
> 
> Moreover, the contract of KVM_DEV_FLIC_GET_ALL_IRQS, which is the only
> path to get_all_floating_irqs(), states that:
> 
>> All interrupts remain pending, i.e. are not deleted from the list of
>> currently pending interrupts.
> 
> Fix by non-destructively testing for the GISA IPM bits.
> 
> Fixes: 24160af6cb28 ("KVM: s390: add GISA interrupts to FLIC ioctl interface")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

much better.

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   arch/s390/kvm/interrupt.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 6b3f97a7513b..61e75d10110e 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -273,6 +273,11 @@ static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>   	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>   }
>   
> +static inline int gisa_test_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +{
> +	return test_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *)gisa);
> +}
> +
>   static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
>   {
>   	unsigned long pending = vcpu->kvm->arch.float_int.pending_irqs |
> @@ -2242,7 +2247,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
>   				ret = -ENOMEM;
>   				goto out_nolock;
>   			}
> -			if (gisa_tac_ipm_gisc(gi->origin, i)) {
> +			if (gisa_test_ipm_gisc(gi->origin, i)) {
>   				irq = (struct kvm_s390_irq *) &buf[n];
>   				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
>   				irq->u.io.io_int_word = isc_to_int_word(i);

Re: [PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs()

[sashiko-bot]

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

Pre-existing issues:
- [High] Potential NULL pointer dereference due to a TOCTOU data race on `gi->origin`.
--

--- Patch [3]: [PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs() ---
commit b1b05a4cfd3db133f1eddf2ecf286b6b12e10c50
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix get_all_floating_irqs()

This commit modifies the reporting of floating interrupts to userspace 
to use a non-destructive test. It ensures that the GISA IPM bits are 
not inadvertently cleared and lost if the userspace output buffer is 
too small.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c

[ ... ]

> @@ -2242,7 +2247,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
>  				ret = -ENOMEM;
>  				goto out_nolock;
>  			}
> -			if (gisa_tac_ipm_gisc(gi->origin, i)) {
> +			if (gisa_test_ipm_gisc(gi->origin, i)) {

[Severity: High]
This is a pre-existing issue, but is it possible for gi->origin to be
dereferenced as NULL here?

Looking at get_all_floating_irqs(), gi->origin is accessed multiple times
without lock protection or READ_ONCE(). The pointer is initially checked
earlier in the function:

    if (gi->origin && gisa_get_ipm(gi->origin)) {

and then accessed repeatedly inside the loop for each ISC.

Could the compiler reload the gi->origin pointer from memory between the
initial check and these loop iterations?

If a concurrent KVM_PV_ENABLE ioctl is invoked, it can execute
kvm_s390_pv_init_vm() -> kvm_s390_cpus_to_pv() -> kvm_s390_gisa_disable() ->
kvm_s390_gisa_destroy(), which sets gi->origin = NULL.

Since the KVM_DEV_FLIC_GET_ALL_IRQS ioctl does not hold kvm->lock, could
this concurrent clearing lead to a NULL pointer dereference if the compiler
reloads gi->origin inside the loop?

>  				irq = (struct kvm_s390_irq *) &buf[n];
>  				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
>  				irq->u.io.io_int_word = isc_to_int_word(i);

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

Re: [PATCH v2 2/9] KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl()

[sashiko-bot]

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

Pre-existing issues:
- [High] kvm_s390_inject_vcpu() can sleep while holding a spinlock when handling a KVM_S390_SIGP_STOP interrupt, leading to a kernel panic.
--

--- Patch [2]: [PATCH v2 2/9] KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl() ---
commit 9a55f0a8064e7c3f8972e52233d190eda6c12400
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl()

kvm_arch_vcpu_unlocked_ioctl() is called without further locks held, but
kvm_s390_inject_vcpu(), which is called from there, needs either the
kvm->srcu or the slots lock. Fix by taking the kvm->srcu.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5463,7 +5464,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
>  			return -EFAULT;
>  		if (s390int_to_s390irq(&s390int, &s390irq))
>  			return -EINVAL;
> -		rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> +		scoped_guard(srcu, &vcpu->kvm->srcu)
> +			rc = kvm_s390_inject_vcpu(vcpu, &s390irq);

[Severity: High]
This is a pre-existing issue, but can this call path sleep while holding
a spinlock?

If userspace calls the KVM_S390_INTERRUPT ioctl with type KVM_S390_SIGP_STOP
and the KVM_S390_STOP_FLAG_STORE_STATUS flag, kvm_s390_inject_vcpu()
acquires a spinlock and disables preemption:

arch/s390/kvm/interrupt.c:kvm_s390_inject_vcpu()
    spin_lock(&li->lock);
    rc = do_inject_vcpu(vcpu, irq);
    spin_unlock(&li->lock);

While under the spinlock, do_inject_vcpu() triggers a guest memory write:

arch/s390/kvm/interrupt.c:__inject_sigp_stop()
    if (irq->u.stop.flags & KVM_S390_STOP_FLAG_STORE_STATUS)
        rc = kvm_s390_store_status_unloaded(vcpu,
                    KVM_S390_STORE_STATUS_NOADDR);

This initiates a write to guest memory that can fault and sleep:

arch/s390/kvm/kvm-s390.c:kvm_s390_store_status_unloaded()
    if (gpa == KVM_S390_STORE_STATUS_NOADDR) {
        if (write_guest_abs(vcpu, 163, &archmode, 1))

Could this underlying __copy_to_user() call (via write_guest_abs) cause a
'scheduling while atomic' kernel panic by sleeping while li->lock is held?

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

Re: [PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Replacing set_page_dirty_lock() with set_page_dirty() on GUP-pinned pages introduces a Use-After-Free vulnerability due to a race with file truncation.
- [High] Ordering mark_page_dirty() before memory modification creates a race condition with dirty log harvesting, risking lost updates during live migration.
--

--- Patch [4]: [PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*() ---
commit 8bb6adeda8416869b198540515723990915f8390
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix dirty marking in adapter_indicators_set*()

This commit updates dirty page marking for adapter indicators in KVM s390.
It adds missing calls to set_page_dirty() and mark_page_dirty() in the fast
path and reorganizes the slow path, replacing set_page_dirty_lock() with
set_page_dirty() for pinned pages.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 608ff95d53bac..e79f636c7544d 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
[ ... ]
> @@ -2997,21 +2999,20 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		ind_page = pin_map_page(kvm, adapter_int->ind_addr, 0);
>  		if (!ind_page)
>  			return -1;
> -		idx = srcu_read_lock(&kvm->srcu);
>  		map = page_address(ind_page);
>  		bit = get_ind_bit(adapter_int->ind_addr,
>  				  adapter_int->ind_offset, adapter->swap);
>  		set_bit(bit, map);
> -		mark_page_dirty(kvm, adapter_int->ind_gaddr >> PAGE_SHIFT);
> -		set_page_dirty_lock(ind_page);
> -		srcu_read_unlock(&kvm->srcu, idx);
> +		set_page_dirty(ind_page);

[Severity: High]
Is it safe to replace set_page_dirty_lock() with set_page_dirty() here?

For short-term GUP-pinned memory like this ind_page, the memory can be backed
by any filesystem. Without the folio lock, a concurrent file truncation could
unmap the folio and free the mapping.

If set_page_dirty() reads folio->mapping concurrently, could it dereference
freed memory when calling mapping->a_ops->dirty_folio()?

>  		unpin_user_page(ind_page);
>  	} else {
>  		map = page_address(ind_info->page);
>  		bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
>  		set_bit(bit, map);
> +		set_page_dirty(ind_info->page);
>  		spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
>  
>  	spin_lock_irqsave(&adapter->maps_lock, flags);
>  	summary_info = get_map_info(adapter, adapter_int->summary_addr);
> @@ -3020,22 +3021,21 @@ static int adapter_indicators_set(struct kvm *kvm,
>  		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
>  		if (WARN_ON_ONCE(!summary_page))
>  			return -1;
> -		idx = srcu_read_lock(&kvm->srcu);
>  		map = page_address(summary_page);
>  		bit = get_ind_bit(adapter_int->summary_addr,
>  				  adapter_int->summary_offset, adapter->swap);
>  		summary_set = test_and_set_bit(bit, map);
> -		mark_page_dirty(kvm, adapter_int->summary_gaddr >> PAGE_SHIFT);
> -		set_page_dirty_lock(summary_page);
> -		srcu_read_unlock(&kvm->srcu, idx);
> +		set_page_dirty(summary_page);

[Severity: High]
Similarly, could calling set_page_dirty() instead of set_page_dirty_lock() on
the GUP-pinned summary_page lead to a use-after-free if the file is truncated
concurrently?

>  		unpin_user_page(summary_page);
>  	} else {
>  		map = page_address(summary_info->page);
>  		bit = get_ind_bit(summary_info->addr, adapter_int->summary_offset,
>  				  adapter->swap);
>  		summary_set = test_and_set_bit(bit, map);
> +		set_page_dirty(summary_info->page);
>  		spin_unlock_irqrestore(&adapter->maps_lock, flags);
>  	}
> +	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
>  
>  	return summary_set ? 0 : 1;
>  }
> @@ -3045,26 +3045,30 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
>  				       struct kvm_s390_adapter_int *adapter_int,
>  				       int setbit)
>  {
[ ... ]
>  	map = page_address(ind_info->page);
>  	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> -	if (setbit)
> +	if (setbit) {
> +		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
> +		set_page_dirty(ind_info->page);
>  		set_bit(bit, map);
> +	}

[Severity: High]
Does calling mark_page_dirty() before actually modifying the memory with
set_bit() create a race condition with dirty log harvesting?

If userspace harvests the KVM dirty log between mark_page_dirty() and
set_bit(), it might clear the dirty bit in KVM's tracking and read the old
unmodified page.

When set_bit() then executes, the page would no longer be marked dirty, and
the change wouldn't be migrated to the destination.

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

Re: [PATCH v2 2/9] KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl()

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> kvm_arch_vcpu_unlocked_ioctl() is called without further locks held, but
> kvm_s390_inject_vcpu(), which is called from there, needs either the
> kvm->srcu or the slots lock.
>
> Fix by taking the kvm->srcu in kvm_arch_vcpu_unlocked_ioctl().
>
> Fixes: ba5c1e9b6cee ("KVM: s390: interrupt subsystem, cpu timer, waitpsw")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/kvm/kvm-s390.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 518a69c55e85..b35340642c3e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5450,7 +5450,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
>  
>  		if (copy_from_user(&s390irq, argp, sizeof(s390irq)))
>  			return -EFAULT;
> -		rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> +		scoped_guard(srcu, &vcpu->kvm->srcu)
> +			rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
>  		break;
>  	}
>  	case KVM_S390_INTERRUPT: {
> @@ -5463,7 +5464,8 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
>  			return -EFAULT;
>  		if (s390int_to_s390irq(&s390int, &s390irq))
>  			return -EINVAL;
> -		rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
> +		scoped_guard(srcu, &vcpu->kvm->srcu)
> +			rc = kvm_s390_inject_vcpu(vcpu, &s390irq);
>  		break;
>  	}
>  	default:

Re: [PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*()

[Christian Borntraeger]

Am 12.08.26 um 12:44 schrieb Claudio Imbrenda:

> @@ -3017,26 +3017,30 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
>   				       struct kvm_s390_adapter_int *adapter_int,
>   				       int setbit)
>   {
> +	struct s390_map_info *ind_info, *summary_info;
>   	unsigned long bit;
>   	int summary_set;
> -	struct s390_map_info *ind_info, *summary_info;
>   	void *map;
>   
> -	spin_lock(&adapter->maps_lock);
> +	guard(srcu)(&kvm->srcu);
> +	guard(spinlock)(&adapter->maps_lock);
> +
>   	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> -	if (!ind_info) {
> -		spin_unlock(&adapter->maps_lock);
> +	if (!ind_info)
>   		return -EWOULDBLOCK;
> -	}
> +
>   	map = page_address(ind_info->page);
>   	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> -	if (setbit)
> +	if (setbit) {
> +		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
> +		set_page_dirty(ind_info->page);
>   		set_bit(bit, map);
> +	}

This marks the indicator page dirty before writing it.
The summary twenty lines below does it the other way round and so do both
slow-path branches.
The order matters because kvm_get_dirty_log_protect() harvests destructively.

Re: [PATCH v2 5/9] KVM: s390: Fix pgste_get_trylock_multiple()

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> In case of failure, pgste_get_trylock_multiple() will attempt to unlock
> the locked PGSTEs based on whether the PCL is set. In some
> circumstances this can lead to unlocking PGSTEs that were locked by
> other threads.
>
> Fix by unlocking the amount of PGSTEs that were actually locked,
> ignoring the PCL bit in the array.
>
> Fixes: 94fd9b16cc67 ("KVM: s390: KVM page table management functions: lifecycle management")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/kvm/dat.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 165c704fcf29..f4dd6f783417 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -923,11 +923,8 @@ static void pgste_set_unlock_multiple(union pte *first, int n, union pgste *pgst
>  {
>  	int i;
>  
> -	for (i = 0; i < n; i++) {
> -		if (!pgstes[i].pcl)
> -			break;
> +	for (i = 0; i < n; i++)
>  		pgste_set_unlock(first + i, pgstes[i]);
> -	}
>  }
>  
>  static bool pgste_get_trylock_multiple(union pte *first, int n, union pgste *pgstes)
> @@ -940,7 +937,7 @@ static bool pgste_get_trylock_multiple(union pte *first, int n, union pgste *pgs
>  	}
>  	if (i == n)
>  		return true;
> -	pgste_set_unlock_multiple(first, n, pgstes);
> +	pgste_set_unlock_multiple(first, i, pgstes);
>  	return false;
>  }
>  

Re: [PATCH v2 4/9] KVM: s390: Fix dirty marking in adapter_indicators_set*()

[Claudio Imbrenda]

On Wed, 12 Aug 2026 13:03:39 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 12.08.26 um 12:44 schrieb Claudio Imbrenda:
> 
> > @@ -3017,26 +3017,30 @@ static int adapter_indicators_set_fast(struct kvm *kvm,
> >   				       struct kvm_s390_adapter_int *adapter_int,
> >   				       int setbit)
> >   {
> > +	struct s390_map_info *ind_info, *summary_info;
> >   	unsigned long bit;
> >   	int summary_set;
> > -	struct s390_map_info *ind_info, *summary_info;
> >   	void *map;
> >   
> > -	spin_lock(&adapter->maps_lock);
> > +	guard(srcu)(&kvm->srcu);
> > +	guard(spinlock)(&adapter->maps_lock);
> > +
> >   	ind_info = get_map_info(adapter, adapter_int->ind_addr);
> > -	if (!ind_info) {
> > -		spin_unlock(&adapter->maps_lock);
> > +	if (!ind_info)
> >   		return -EWOULDBLOCK;
> > -	}
> > +
> >   	map = page_address(ind_info->page);
> >   	bit = get_ind_bit(ind_info->addr, adapter_int->ind_offset, adapter->swap);
> > -	if (setbit)
> > +	if (setbit) {
> > +		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
> > +		set_page_dirty(ind_info->page);
> >   		set_bit(bit, map);
> > +	}  
> 
> This marks the indicator page dirty before writing it.
> The summary twenty lines below does it the other way round and so do both
> slow-path branches.
> The order matters because kvm_get_dirty_log_protect() harvests destructively.

yes, that was a typo, but the patch also has other issues, so I'm
dropping it for now.

Re: [PATCH v2 9/9] KVM: s390: Fix _gaccess_shadow_fault()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Calling `gmap_link()` inside `_gaccess_do_shadow()` while holding `sg->parent->children_lock` causes a guaranteed lockdep splat and a recursive spinlock deadlock.
--

--- Patch [9]: [PATCH v2 9/9] KVM: s390: Fix _gaccess_shadow_fault() ---
commit 2773a2ded997afc2d9abf44ed48fd84a08d773f8
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix _gaccess_shadow_fault()

This commit addresses an issue where shadowed nested guest memory is not
present in the parent guest gmap, causing -ENOENT to be incorrectly
propagated to userspace. It fixes this by manually calling gmap_link()
on the missing memory of the nested guest.

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1551,8 +1552,17 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
>  			return -EAGAIN;
>  	}
>  
> +retry:
>  	rc = dat_entry_walk(NULL, entries[LEVEL_MEM].gfn, sg->parent->asce, DAT_WALK_LEAF,
>  			    TABLE_TYPE_PAGE_TABLE, &host, &ptep_h);
> +	if (rc == -ENOENT) {
> +		slot = gfn_to_memslot(sg->kvm, entries[LEVEL_MEM].gfn);
> +		if (!slot)
> +			return PGM_ADDRESSING;
> +		rc = gmap_link(mc, sg->parent, entries + LEVEL_MEM, slot);

[Severity: Critical]
Does this introduce a recursive spinlock deadlock?

Earlier in _gaccess_do_shadow(), we enforce that the parent's children_lock
is already held:

	lockdep_assert_held(&sg->parent->children_lock);

However, the new call to gmap_link() passes sg->parent as the gmap argument.
If gmap_link() eventually triggers an unshadow event, the call chain reaches
gmap_handle_vsie_unshadow_event(), which attempts to unconditionally acquire
the same lock:

	scoped_guard(spinlock, &parent->children_lock)
		_gmap_handle_vsie_unshadow_event(parent, gfn);

Additionally, gmap_link() can lead to _gmap_ptep_xchg() which explicitly
asserts that the children_lock is not held:

	else
		lockdep_assert_not_held(&gmap->children_lock);

Could a nested guest trigger this missing mapping path and cause a host
kernel hang?

> +		if (!rc)
> +			goto retry;
> +	}
>  	if (rc)
>  		return rc;
>

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

Re: [PATCH v2 6/9] KVM: s390: Fix IRQ injection with SIGP Stop and Store Status

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> When __inject_sigp_stop() is called for a Stop and Store Status
> operation, if the vCPU is running, the interrupt is marked as pending
> and the status is stored by the thread performing the KVM_RUN IOCTL.
>
> If the vCPU is already stopped, the status is stored immediately.
>
> Storing the status means writing into userspace, which might fault, and
> __inject_sigp_stop() is called from do_inject_vcpu() which in turn is
> always called holding a spinlock, which is obviously an issue.
>
> Fix this by returning -EWOULDBLOCK from __inject_sigp_stop(), and
> adding a bool flag to indicate whether a store status is needed. The
> callers of do_inject_vcpu() are modified to pass the pointer to the
> bool flag; whenever a Store Status operation is needed, the callers can
> now perform it outside the spinlock.
>
> Opportunistically refactor kvm_s390_set_irq_state() to use
> scoped_guard() and __free().
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 70 +++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 32 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 8e4b88bce31f..6940f4d354e5 100644

[...]

> @@ -3188,31 +3192,33 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	if (copy_from_user((void *) buf, irqstate, len)) {
> -		r = -EFAULT;
> -		goto out_free;
> -	}
> +	if (copy_from_user((void *)buf, irqstate, len))
> +		return -EFAULT;
>  
> -	/*
> -	 * Don't allow setting the interrupt state
> -	 * when there are already interrupts pending
> -	 */
> -	spin_lock(&li->lock);
> -	if (li->pending_irqs) {
> -		r = -EBUSY;
> -		goto out_unlock;
> -	}
> +	scoped_guard(spinlock, &li->lock) {
> +		/*
> +		 * Don't allow setting the interrupt state
> +		 * when there are already interrupts pending
> +		 */
> +		if (li->pending_irqs)
> +			return -EBUSY;
>  
> -	for (n = 0; n < len / sizeof(*buf); n++) {
> -		r = do_inject_vcpu(vcpu, &buf[n]);
> -		if (r)
> -			break;
> +		for (n = 0; n < len / sizeof(*buf); n++) {
> +			tmp = false;
> +			r = do_inject_vcpu(vcpu, &buf[n], &tmp);
> +			if (r == -EWOULDBLOCK && tmp) {
> +				storestatus = true;
> +				r = 0;
> +			}
> +			if (r)
> +				break;
> +		}
>  	}
>  
> -out_unlock:
> -	spin_unlock(&li->lock);
> -out_free:
> -	vfree(buf);
> +	if (storestatus) {
> +		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);

I assume we do not care about loosing n = -EFAULT when we are already on the
error path here with r != 0. But are there cases in which we would not want to
call kvm_s390_store_status_unloaded() at all here when one of the later
do_inject_vcpu() calls failed with a specific error?

> +		return r ? r : n;
> +	}
>  
>  	return r;
>  }

Re: [PATCH v2 7/9] KVM: s390: Fix kvm_s390_clear_pv_state()

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> kvm_s390_clear_pv_state() needs to also clear the dumping flag, to
> allow the protected VM to be started again (as non-protected, with all
> protected state safely destroyed) after a forced reboot while a
> protected dump was ongoing and not completed.
>
> Fixes: e40df9efd68a ("KVM: s390: pv: clear the state without memset")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Acked-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/kvm/pv.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
> index b02e0159d3cd..98a9a57f71b9 100644
> --- a/arch/s390/kvm/pv.c
> +++ b/arch/s390/kvm/pv.c
> @@ -242,6 +242,10 @@ static void kvm_s390_clear_pv_state(struct kvm *kvm)
>  	kvm->arch.pv.guest_len = 0;
>  	kvm->arch.pv.stor_base = 0;
>  	kvm->arch.pv.stor_var = NULL;
> +	if (kvm->arch.pv.dumping) {
> +		kvm_s390_vcpu_unblock_all(kvm);
> +		kvm->arch.pv.dumping = false;
> +	}
>  }
>  
>  static void kvm_s390_pv_dispose_cpu(struct kvm_vcpu *vcpu, bool free_stor_base)

Re: [PATCH v2 6/9] KVM: s390: Fix IRQ injection with SIGP Stop and Store Status

[Claudio Imbrenda]

On Wed, 12 Aug 2026 14:59:36 +0200
"Christoph Schlameuss" <schlameuss@linux.ibm.com> wrote:

> On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> > When __inject_sigp_stop() is called for a Stop and Store Status
> > operation, if the vCPU is running, the interrupt is marked as pending
> > and the status is stored by the thread performing the KVM_RUN IOCTL.
> >
> > If the vCPU is already stopped, the status is stored immediately.
> >
> > Storing the status means writing into userspace, which might fault, and
> > __inject_sigp_stop() is called from do_inject_vcpu() which in turn is
> > always called holding a spinlock, which is obviously an issue.
> >
> > Fix this by returning -EWOULDBLOCK from __inject_sigp_stop(), and
> > adding a bool flag to indicate whether a store status is needed. The
> > callers of do_inject_vcpu() are modified to pass the pointer to the
> > bool flag; whenever a Store Status operation is needed, the callers can
> > now perform it outside the spinlock.
> >
> > Opportunistically refactor kvm_s390_set_irq_state() to use
> > scoped_guard() and __free().
> >
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >  arch/s390/kvm/interrupt.c | 70 +++++++++++++++++++++------------------
> >  1 file changed, 38 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > index 8e4b88bce31f..6940f4d354e5 100644  
> 
> [...]
> 
> > @@ -3188,31 +3192,33 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
> >  	if (!buf)
> >  		return -ENOMEM;
> >  
> > -	if (copy_from_user((void *) buf, irqstate, len)) {
> > -		r = -EFAULT;
> > -		goto out_free;
> > -	}
> > +	if (copy_from_user((void *)buf, irqstate, len))
> > +		return -EFAULT;
> >  
> > -	/*
> > -	 * Don't allow setting the interrupt state
> > -	 * when there are already interrupts pending
> > -	 */
> > -	spin_lock(&li->lock);
> > -	if (li->pending_irqs) {
> > -		r = -EBUSY;
> > -		goto out_unlock;
> > -	}
> > +	scoped_guard(spinlock, &li->lock) {
> > +		/*
> > +		 * Don't allow setting the interrupt state
> > +		 * when there are already interrupts pending
> > +		 */
> > +		if (li->pending_irqs)
> > +			return -EBUSY;
> >  
> > -	for (n = 0; n < len / sizeof(*buf); n++) {
> > -		r = do_inject_vcpu(vcpu, &buf[n]);
> > -		if (r)
> > -			break;
> > +		for (n = 0; n < len / sizeof(*buf); n++) {
> > +			tmp = false;
> > +			r = do_inject_vcpu(vcpu, &buf[n], &tmp);
> > +			if (r == -EWOULDBLOCK && tmp) {
> > +				storestatus = true;
> > +				r = 0;
> > +			}
> > +			if (r)
> > +				break;
> > +		}
> >  	}
> >  
> > -out_unlock:
> > -	spin_unlock(&li->lock);
> > -out_free:
> > -	vfree(buf);
> > +	if (storestatus) {
> > +		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);  
> 
> I assume we do not care about loosing n = -EFAULT when we are already on the
> error path here with r != 0. But are there cases in which we would not want to
> call kvm_s390_store_status_unloaded() at all here when one of the later
> do_inject_vcpu() calls failed with a specific error?

no, because the store status should have happened before the failed
interrupt injections.

The only tricky part is if store status fails, but the other injections
don't. In that case we should have aborted without injecting the other
interupts. On the other hand, when the caller receives a -EFAULT it
will probably just give up on the whole VM.

> 
> > +		return r ? r : n;
> > +	}
> >  
> >  	return r;
> >  }  
> 
> 

Re: [PATCH v2 8/9] KVM: s390: Fix potential tiny kernel stack leak

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> In some circumstances, one bit of kernel stack could have been leaked
> from dat_cond_set_storage_key().
>
> Fix by clearing prev before use.
>
> Fixes: 8e03e8316eb2 ("KVM: s390: KVM page table management functions: storage keys")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/kvm/dat.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index f4dd6f783417..f2ea013cb33e 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
> @@ -737,6 +737,7 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
>  	pgste.fp = skey.fp;
>  	pgste.gc = skey.c;
>  	pgste.gr = skey.r;
> +	prev.skey = 0;
>  
>  	if (!ptep->h.i) {
>  		rc = page_cond_set_storage_key(pte_origin(*ptep), skey, &prev, nq, mr, mc);

Re: [PATCH v2 6/9] KVM: s390: Fix IRQ injection with SIGP Stop and Store Status

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 3:11 PM CEST, Claudio Imbrenda wrote:
> On Wed, 12 Aug 2026 14:59:36 +0200
> "Christoph Schlameuss" <schlameuss@linux.ibm.com> wrote:
>
>> On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
>> > When __inject_sigp_stop() is called for a Stop and Store Status
>> > operation, if the vCPU is running, the interrupt is marked as pending
>> > and the status is stored by the thread performing the KVM_RUN IOCTL.
>> >
>> > If the vCPU is already stopped, the status is stored immediately.
>> >
>> > Storing the status means writing into userspace, which might fault, and
>> > __inject_sigp_stop() is called from do_inject_vcpu() which in turn is
>> > always called holding a spinlock, which is obviously an issue.
>> >
>> > Fix this by returning -EWOULDBLOCK from __inject_sigp_stop(), and
>> > adding a bool flag to indicate whether a store status is needed. The
>> > callers of do_inject_vcpu() are modified to pass the pointer to the
>> > bool flag; whenever a Store Status operation is needed, the callers can
>> > now perform it outside the spinlock.
>> >
>> > Opportunistically refactor kvm_s390_set_irq_state() to use
>> > scoped_guard() and __free().
>> >
>> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>> > ---
>> >  arch/s390/kvm/interrupt.c | 70 +++++++++++++++++++++------------------
>> >  1 file changed, 38 insertions(+), 32 deletions(-)
>> >
>> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> > index 8e4b88bce31f..6940f4d354e5 100644  
>> 
>> [...]
>> 
>> > @@ -3188,31 +3192,33 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
>> >  	if (!buf)
>> >  		return -ENOMEM;
>> >  
>> > -	if (copy_from_user((void *) buf, irqstate, len)) {
>> > -		r = -EFAULT;
>> > -		goto out_free;
>> > -	}
>> > +	if (copy_from_user((void *)buf, irqstate, len))
>> > +		return -EFAULT;
>> >  
>> > -	/*
>> > -	 * Don't allow setting the interrupt state
>> > -	 * when there are already interrupts pending
>> > -	 */
>> > -	spin_lock(&li->lock);
>> > -	if (li->pending_irqs) {
>> > -		r = -EBUSY;
>> > -		goto out_unlock;
>> > -	}
>> > +	scoped_guard(spinlock, &li->lock) {
>> > +		/*
>> > +		 * Don't allow setting the interrupt state
>> > +		 * when there are already interrupts pending
>> > +		 */
>> > +		if (li->pending_irqs)
>> > +			return -EBUSY;
>> >  
>> > -	for (n = 0; n < len / sizeof(*buf); n++) {
>> > -		r = do_inject_vcpu(vcpu, &buf[n]);
>> > -		if (r)
>> > -			break;
>> > +		for (n = 0; n < len / sizeof(*buf); n++) {
>> > +			tmp = false;
>> > +			r = do_inject_vcpu(vcpu, &buf[n], &tmp);
>> > +			if (r == -EWOULDBLOCK && tmp) {
>> > +				storestatus = true;
>> > +				r = 0;
>> > +			}
>> > +			if (r)
>> > +				break;
>> > +		}
>> >  	}
>> >  
>> > -out_unlock:
>> > -	spin_unlock(&li->lock);
>> > -out_free:
>> > -	vfree(buf);
>> > +	if (storestatus) {
>> > +		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);  
>> 
>> I assume we do not care about loosing n = -EFAULT when we are already on the
>> error path here with r != 0. But are there cases in which we would not want to
>> call kvm_s390_store_status_unloaded() at all here when one of the later
>> do_inject_vcpu() calls failed with a specific error?
>
> no, because the store status should have happened before the failed
> interrupt injections.
>
> The only tricky part is if store status fails, but the other injections
> don't. In that case we should have aborted without injecting the other
> interupts. On the other hand, when the caller receives a -EFAULT it
> will probably just give up on the whole VM.
>

Thanks, that would have been my feeling, just wanted to validate.

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>


>> > +		return r ? r : n;
>> > +	}
>> >  
>> >  	return r;
>> >  }  
>> 
>> 

Re: [PATCH v2 3/9] KVM: s390: Fix get_all_floating_irqs()

[Christoph Schlameuss]

On Wed Aug 12, 2026 at 12:44 PM CEST, Claudio Imbrenda wrote:
> When attempting to report all pending floating interrupt to userspace,
> the GISA IPM bits are atomically tested and cleared, and the
> corresponding interrupt description is written in the output buffer. If
> the output buffer is too small, an error is returned to userspace, but
> the GISA IPM bits are now lost.
>
> Moreover, the contract of KVM_DEV_FLIC_GET_ALL_IRQS, which is the only
> path to get_all_floating_irqs(), states that:
>
>> All interrupts remain pending, i.e. are not deleted from the list of
>> currently pending interrupts.
>
> Fix by non-destructively testing for the GISA IPM bits.
>
> Fixes: 24160af6cb28 ("KVM: s390: add GISA interrupts to FLIC ioctl interface")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  arch/s390/kvm/interrupt.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 6b3f97a7513b..61e75d10110e 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -273,6 +273,11 @@ static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>  }
>  
> +static inline int gisa_test_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> +{
> +	return test_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *)gisa);
> +}
> +
>  static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
>  {
>  	unsigned long pending = vcpu->kvm->arch.float_int.pending_irqs |
> @@ -2242,7 +2247,7 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
>  				ret = -ENOMEM;
>  				goto out_nolock;
>  			}
> -			if (gisa_tac_ipm_gisc(gi->origin, i)) {
> +			if (gisa_test_ipm_gisc(gi->origin, i)) {
>  				irq = (struct kvm_s390_irq *) &buf[n];
>  				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
>  				irq->u.io.io_int_word = isc_to_int_word(i);