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