[PATCH v4 00/13] KVM: s390: Misc fixes

[PATCH v4 00/13] KVM: s390: Misc fixes

[Claudio Imbrenda]

Fix a bunch of small issues that came up during the previous round of fixes.

They are mostly extremely unlikely races, but they should be fixed
nonetheless.

v3->v4
* Improve patch descriptions, add comments
* Use smp_store_release and smp_load_acquire in the first patch
* Multiple fixes in patch 4: potential NULL pointer dereference,
  incorrect behaviour in low memory condition
* Rework patch 8 to use scope-based cleanup instead of gotos.
* Three new patches:
  - KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory
  - KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()
  - KVM: s390: Fix sca_clear_ext_call() for UCONTROL

v2->v3
* Use READ_ONCE to pair with WRITE_ONCE in the first patch
* Fix leaking PGM_ADDRESSING also in kvm_s390_keyop() and related functions
* Fix and improve commit messages
* Use slots_arch_lock instead of slots_lock for ESSA operations
* Use normal spin_{,un}lock() functions instead of scoped_guard to avoid
  mixing the two styles
* Use the newly introduced vcpu->arch.initialized to determine whether the
  SCA entry needs to be cleared
* Improve handling of -EINTR; handle_mvpg_pei() needed some refactoring to
  deal with it properly
* Three new patches:
  - Free the mmu cache when kvm_arch_vcpu_create() fails
  - Fix ordering when adding to SCA
  - Fix cleanup in kvm_s390_pv_create_cpu()

v1->v2
* Drop some patches that have been picked upstream in the meantime.
* Drop patch 3, as it was trying to fix a bug that does not exist
* Avoid the NULL gmap dereference by using a flag
* Fix the return value of kvm_s390_[gp]et_skeys too
* Use kvm->slots_arch_lock instead of kvm->slots_lock for CMMA and ESSA
  handling, to avoid potential deadlocks with the RCU.
* Three new patches to fix other issues that came out while fixing the
  other issues

Claudio Imbrenda (13):
  KVM: s390: Fix unlikely NULL gmap dereference
  KVM: s390: Fix leaking of PGM_ADDRESSING to userspace
  KVM: s390: Fix race in __do_essa()
  KVM: s390: cmma: Fix dirty tracking when removing memslot
  KVM: s390: ucontrol: Add missing locking around gmap_remove_child()
  KVM: s390: Fix overclearing ESCA in case of error
  KVM: s390: Return -EINTR if a signal was pending while faulting-in
  KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails
  KVM: s390: Fix ordering when adding to SCA
  KVM: s390: Fix cleanup in kvm_s390_pv_create_cpu()
  KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory
  KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()
  KVM: s390: Fix sca_clear_ext_call() for UCONTROL

 arch/s390/include/asm/kvm_host.h |   1 +
 arch/s390/kvm/dat.c              |  23 ++++--
 arch/s390/kvm/dat.h              |   2 +-
 arch/s390/kvm/faultin.c          |   4 +-
 arch/s390/kvm/intercept.c        |  23 +++---
 arch/s390/kvm/interrupt.c        |   7 +-
 arch/s390/kvm/kvm-s390.c         | 121 +++++++++++++++++++------------
 arch/s390/kvm/priv.c             |  10 ++-
 arch/s390/kvm/pv.c               |  43 +++++------
 9 files changed, 141 insertions(+), 93 deletions(-)

-- 
2.55.0

[PATCH v4 01/13] KVM: s390: Fix unlikely NULL gmap dereference

[Claudio Imbrenda]

When creating a new vCPU, kvm_vm_ioctl_create_vcpu() will call
kvm_arch_vcpu_postcreate() after the file descriptor for the new vCPU
has been created. The new file descriptor has not been returned yet,
but a malicious userspace program could try to guess it.

If a malicious userspace program manages to start the newly created vCPU
before kvm_arch_vcpu_postcreate() is called, __vcpu_run() will try to
dereference vcpu->arch.gmap and trigger a NULL pointer dereference.

Fix this by adding a new field to struct kvm_vcpu_arch to keep track of
the initialization status of the vCPU. Refuse to run a vCPU that is not
fully initialized.

Fixes: dafd032a15f8 ("KVM: s390: move vcpu specific initalization to a later point")
Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/kvm-s390.c         | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index eaa34c5bd3c1..edf75b6ad20c 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -440,6 +440,7 @@ struct kvm_vcpu_arch {
 	bool skey_enabled;
 	/* Indicator if the access registers have been loaded from guest */
 	bool acrs_loaded;
+	bool initialized;
 	struct kvm_s390_pv_vcpu pv;
 	union diag318_info diag318_info;
 	struct kvm_s390_mmu_cache *mc;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 150b5dd2170e..f86b4b0b356f 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3613,6 +3613,9 @@ void kvm_arch_vcpu_postcreate(struct kvm_vcpu *vcpu)
 	if (test_kvm_facility(vcpu->kvm, 74) || vcpu->kvm->arch.user_instr0 ||
 	    vcpu->kvm->arch.user_operexec)
 		vcpu->arch.sie_block->ictl |= ICTL_OPEREXC;
+
+	/* Pairs with smp_load_acquire() in kvm_arch_vcpu_ioctl_run() and kvm_arch_vcpu_ioctl() */
+	smp_store_release(&vcpu->arch.initialized, true);
 }
 
 static bool kvm_has_pckmo_subfunc(struct kvm *kvm, unsigned long nr)
@@ -5039,6 +5042,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 	    kvm_run->kvm_dirty_regs & ~KVM_SYNC_S390_VALID_FIELDS)
 		return -EINVAL;
 
+	/* Pairs with smp_store_release() in kvm_arch_vcpu_postcreate() */
+	if (!smp_load_acquire(&vcpu->arch.initialized))
+		return -EINVAL;
+
 	vcpu_load(vcpu);
 
 	if (guestdbg_exit_pending(vcpu)) {
@@ -5523,6 +5530,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	long r;
 	u16 rc, rrc;
 
+	/* Pairs with smp_store_release() in kvm_arch_vcpu_postcreate() */
+	if (!smp_load_acquire(&vcpu->arch.initialized))
+		return -EINVAL;
+
 	vcpu_load(vcpu);
 
 	switch (ioctl) {
-- 
2.55.0

[PATCH v4 02/13] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

[Claudio Imbrenda]

If kvm_s390_set_cmma_bits() is asked to set CMMA values outside of a
memslot, PGM_ADDRESSING (5) is returned, instead of a negative error
value.

Same issue with kvm_s390_{g,s}et_skeys(), kvm_s390_keyop(), and
dat_reset_reference_bit().

Fix by returning -EFAULT whenever the return value would be > 0, which
is consistent with the behaviour before the gmap rewrite.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/dat.c      | 16 ++++++++++------
 arch/s390/kvm/dat.h      |  2 +-
 arch/s390/kvm/kvm-s390.c | 16 ++++++++--------
 arch/s390/kvm/priv.c     |  5 +++--
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index ed4259d17629..171b61959908 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -755,13 +755,15 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
 	return rc;
 }
 
-int dat_reset_reference_bit(union asce asce, gfn_t gfn)
+int dat_reset_reference_bit(union asce asce, gfn_t gfn, union skey *skey)
 {
 	union pgste pgste, old;
 	union crste *crstep;
 	union pte *ptep;
 	int rc;
 
+	skey->skey = 0;
+
 	rc = dat_entry_walk(NULL, gfn, asce, DAT_WALK_ANY, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
 	if (rc)
 		return rc;
@@ -771,21 +773,23 @@ int dat_reset_reference_bit(union asce asce, gfn_t gfn)
 
 		if (!crste.h.fc || !crste.s.fc1.pr)
 			return 0;
-		return page_reset_referenced(large_crste_to_phys(*crstep, gfn));
+		skey->skey = page_reset_referenced(large_crste_to_phys(*crstep, gfn)) << 1;
+		return 0;
 	}
 	old = pgste_get_lock(ptep);
 	pgste = old;
 
 	if (!ptep->h.i) {
-		rc = page_reset_referenced(pte_origin(*ptep));
-		pgste.hr = rc >> 1;
+		skey->skey = page_reset_referenced(pte_origin(*ptep)) << 1;
+		pgste.hr = skey->r;
 	}
-	rc |= (pgste.gr << 1) | pgste.gc;
+	skey->r |= pgste.gr;
+	skey->c |= pgste.gc;
 	pgste.gr = 0;
 
 	dat_update_ptep_sd(old, pgste, ptep);
 	pgste_set_unlock(ptep, pgste);
-	return rc;
+	return 0;
 }
 
 static long dat_reset_skeys_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
diff --git a/arch/s390/kvm/dat.h b/arch/s390/kvm/dat.h
index fad605305e05..141ee7b9f019 100644
--- a/arch/s390/kvm/dat.h
+++ b/arch/s390/kvm/dat.h
@@ -537,7 +537,7 @@ int dat_set_storage_key(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t gf
 			union skey skey, bool nq);
 int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gfn_t gfn,
 			     union skey skey, union skey *oldkey, bool nq, bool mr, bool mc);
-int dat_reset_reference_bit(union asce asce, gfn_t gfn);
+int dat_reset_reference_bit(union asce asce, gfn_t gfn, union skey *skey);
 long dat_reset_skeys(union asce asce, gfn_t start);
 
 unsigned long dat_get_ptval(struct page_table *table, struct ptval_param param);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f86b4b0b356f..7bc6426f5f4d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -571,7 +571,7 @@ static int kvm_s390_keyop(struct kvm_s390_mmu_cache *mc, struct kvm *kvm, int op
 	switch (op) {
 	case KVM_S390_KEYOP_SSKE:
 		r = dat_cond_set_storage_key(mc, asce, gfn, skey, &skey, 0, 0, 0);
-		if (r >= 0)
+		if (r == 0 || r == 1)
 			return skey.skey;
 		break;
 	case KVM_S390_KEYOP_ISKE:
@@ -580,14 +580,14 @@ static int kvm_s390_keyop(struct kvm_s390_mmu_cache *mc, struct kvm *kvm, int op
 			return skey.skey;
 		break;
 	case KVM_S390_KEYOP_RRBE:
-		r = dat_reset_reference_bit(asce, gfn);
-		if (r > 0)
-			return r << 1;
+		r = dat_reset_reference_bit(asce, gfn, &skey);
+		if (!r)
+			return skey.skey;
 		break;
 	default:
 		return -EINVAL;
 	}
-	return r;
+	return r > 0 ? -EFAULT : r;
 }
 
 /* Section: device related */
@@ -2214,7 +2214,7 @@ static int kvm_s390_get_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
 	}
 
 	kvfree(keys);
-	return r;
+	return r <= 0 ? r : -EFAULT;
 }
 
 static int kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
@@ -2276,7 +2276,7 @@ static int kvm_s390_set_skeys(struct kvm *kvm, struct kvm_s390_skeys *args)
 	kvm_s390_free_mmu_cache(mc);
 out:
 	kvfree(keys);
-	return r;
+	return r <= 0 ? r : -EFAULT;
 }
 
 /*
@@ -2386,7 +2386,7 @@ static int kvm_s390_set_cmma_bits(struct kvm *kvm,
 
 	set_bit(GMAP_FLAG_USES_CMM, &kvm->arch.gmap->flags);
 
-	return r;
+	return r <= 0 ? r : -EFAULT;
 }
 
 /**
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index ad0ddc433a73..ea5a99537346 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -289,6 +289,7 @@ static int handle_iske(struct kvm_vcpu *vcpu)
 static int handle_rrbe(struct kvm_vcpu *vcpu)
 {
 	unsigned long gaddr;
+	union skey skey;
 	int reg1, reg2;
 	int rc;
 
@@ -307,12 +308,12 @@ static int handle_rrbe(struct kvm_vcpu *vcpu)
 	gaddr = kvm_s390_logical_to_effective(vcpu, gaddr);
 	gaddr = kvm_s390_real_to_abs(vcpu, gaddr);
 	scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
-		rc = dat_reset_reference_bit(vcpu->arch.gmap->asce, gpa_to_gfn(gaddr));
+		rc = dat_reset_reference_bit(vcpu->arch.gmap->asce, gpa_to_gfn(gaddr), &skey);
 	if (rc > 0)
 		return kvm_s390_inject_program_int(vcpu, rc);
 	if (rc < 0)
 		return rc;
-	kvm_s390_set_psw_cc(vcpu, rc);
+	kvm_s390_set_psw_cc(vcpu, (skey.skey >> 1) & 3);
 	return 0;
 }
 
-- 
2.55.0

[PATCH v4 03/13] KVM: s390: Fix race in __do_essa()

[Claudio Imbrenda]

An unlikely race between __do_essa() and kvm_s390_vm_start_migration(),
kvm_s390_vm_stop_migration(), or dat_get_cmma() was possible.

Fix by locking kvm->slots_lock. Since this is not a hot path, the
overhead of an additional mutex is negligible.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 14 +++++++-------
 arch/s390/kvm/priv.c     |  5 +++--
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7bc6426f5f4d..100911ee793a 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1300,7 +1300,9 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
 {
 	int res = -ENXIO;
 
-	mutex_lock(&kvm->slots_lock);
+	guard(srcu)(&kvm->srcu);
+	guard(mutex)(&kvm->slots_arch_lock);
+
 	switch (attr->attr) {
 	case KVM_S390_VM_MIGRATION_START:
 		res = kvm_s390_vm_start_migration(kvm);
@@ -1311,7 +1313,6 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
 	default:
 		break;
 	}
-	mutex_unlock(&kvm->slots_lock);
 
 	return res;
 }
@@ -2998,9 +2999,8 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		r = -EFAULT;
 		if (copy_from_user(&args, argp, sizeof(args)))
 			break;
-		mutex_lock(&kvm->slots_lock);
-		r = kvm_s390_get_cmma_bits(kvm, &args);
-		mutex_unlock(&kvm->slots_lock);
+		scoped_guard(mutex, &kvm->slots_arch_lock)
+			r = kvm_s390_get_cmma_bits(kvm, &args);
 		if (!r) {
 			r = copy_to_user(argp, &args, sizeof(args));
 			if (r)
@@ -3014,9 +3014,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		r = -EFAULT;
 		if (copy_from_user(&args, argp, sizeof(args)))
 			break;
-		mutex_lock(&kvm->slots_lock);
+		mutex_lock(&kvm->slots_arch_lock);
 		r = kvm_s390_set_cmma_bits(kvm, &args);
-		mutex_unlock(&kvm->slots_lock);
+		mutex_unlock(&kvm->slots_arch_lock);
 		break;
 	}
 	case KVM_S390_PV_COMMAND: {
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index ea5a99537346..b1ba24c346ef 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -1261,8 +1261,9 @@ static int handle_essa(struct kvm_vcpu *vcpu)
 		/* Retry the ESSA instruction */
 		kvm_s390_retry_instr(vcpu);
 	} else {
-		scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
-			i = __do_essa(vcpu, orc);
+		scoped_guard(mutex, &vcpu->kvm->slots_arch_lock)
+			scoped_guard(read_lock, &vcpu->kvm->mmu_lock)
+				i = __do_essa(vcpu, orc);
 		if (i < 0)
 			return i;
 		/* Account for the possible extra cbrl entry */
-- 
2.55.0

[PATCH v4 04/13] KVM: s390: cmma: Fix dirty tracking when removing memslot

[Claudio Imbrenda]

When a memslot is removed, all ptes that mapped the slot are cleared or
even deallocated. If this happens while the system is in migration
mode, and if cmma-dirty pages are removed, the cmma-dirty counter will
not reflect reality.

Fix by appropriately decrementing the cmma-dirty counter when removing
a memslot.

Opportunistically improve kvm_arch_commit_memory_region() to use
__free() for the struct kvm_s390_mmu_cache.

Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/dat.c      |  7 ++++++-
 arch/s390/kvm/kvm-s390.c | 19 +++++++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 171b61959908..3f2d6e8902d7 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -850,6 +850,7 @@ static long _dat_slot_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_wal
 	struct slot_priv *p = walk->priv;
 	union crste dummy = { .val = p->token };
 	union pte new_pte, pte = READ_ONCE(*ptep);
+	union pgste pgste;
 
 	new_pte = _PTE_TOK(dummy.tok.type, dummy.tok.par);
 
@@ -857,7 +858,11 @@ static long _dat_slot_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_wal
 	if (pte.val == new_pte.val)
 		return 0;
 
-	dat_ptep_xchg(ptep, new_pte, gfn, walk->asce, false);
+	pgste = pgste_get_lock(ptep);
+	pgste = __dat_ptep_xchg(ptep, pgste, new_pte, gfn, walk->asce, false);
+	pgste.cmma_d = 0;
+	pgste_set_unlock(ptep, pgste);
+
 	return 0;
 }
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 100911ee793a..7b37e410d416 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5805,14 +5805,24 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 	return 0;
 }
 
+static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
+{
+	if (pgste_of(ptep)->cmma_d)
+		atomic64_dec(walk->priv);
+	return 0;
+}
+
 void kvm_arch_commit_memory_region(struct kvm *kvm,
 				struct kvm_memory_slot *old,
 				const struct kvm_memory_slot *new,
 				enum kvm_mr_change change)
 {
-	struct kvm_s390_mmu_cache *mc = NULL;
+	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
+	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
 	int rc = 0;
 
+	guard(mutex)(&kvm->slots_arch_lock);
+
 	if (change == KVM_MR_FLAGS_ONLY)
 		return;
 
@@ -5823,6 +5833,12 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 	}
 
 	scoped_guard(write_lock, &kvm->mmu_lock) {
+		if (kvm->arch.migration_mode && kvm->arch.use_cmma && old) {
+			_dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
+					    kvm->arch.gmap->asce, &ops, DAT_WALK_IGN_HOLES,
+					    &kvm->arch.cmma_dirty_pages);
+		}
+
 		switch (change) {
 		case KVM_MR_DELETE:
 			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
@@ -5844,7 +5860,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 out:
 	if (rc)
 		pr_warn("failed to commit memory region\n");
-	kvm_s390_free_mmu_cache(mc);
 	return;
 }
 
-- 
2.55.0

[PATCH v4 05/13] KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

[Claudio Imbrenda]

gmap_remove_child() needs to be called while holding the children_lock
of the parent gmap. This was not the case in the error handling path of
kvm_arch_vcpu_create() for UCONTROL guests.

Fix by adding the missing lock.

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

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7b37e410d416..82c6c8632e1b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3870,7 +3870,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 
 out_ucontrol_uninit:
 	if (kvm_is_ucontrol(vcpu->kvm)) {
+		spin_lock(&vcpu->kvm->arch.gmap->children_lock);
 		gmap_remove_child(vcpu->arch.gmap);
+		spin_unlock(&vcpu->kvm->arch.gmap->children_lock);
 		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
 	}
 out_free_sie_block:
-- 
2.55.0

[PATCH v4 06/13] KVM: s390: Fix overclearing ESCA in case of error

[Claudio Imbrenda]

If an attempt is made to create a vCPU with an already existing ID,
the duplicated vCPU will be destroyed. When destroying a vCPU, its
ESCA entry will be cleared. In the above scenario, the spurious
duplicate vCPU is destroyed, but the ESCA entry corresponding to the
original vCPU is cleared.

Fix by skipping clearing the ESCA entry if the vCPU creation was not
successful, i.e. if the pointer to the ESCA in the state description is
not set.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 2 +-
 arch/s390/kvm/kvm-s390.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 9e3e6b0d72ad..2acdff130fa6 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -86,7 +86,7 @@ static void sca_clear_ext_call(struct kvm_vcpu *vcpu)
 	struct esca_block *sca = vcpu->kvm->arch.sca;
 	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
 
-	if (!kvm_s390_use_sca_entries())
+	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
 		return;
 	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);
 
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 82c6c8632e1b..47eba1664bec 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3461,7 +3461,7 @@ static void sca_del_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct esca_block *sca = vcpu->kvm->arch.sca;
 
-	if (!kvm_s390_use_sca_entries())
+	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
 		return;
 
 	clear_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);
-- 
2.55.0

[PATCH v4 07/13] KVM: s390: Return -EINTR if a signal was pending while faulting-in

[Claudio Imbrenda]

If a signal is pending while trying to fault-in a page, return -EINTR
instead of -EAGAIN.

Also fix unpack_one() and handle_mvpg_pei() so that they can handle
-EINTR properly.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: e907ae530133 ("KVM: s390: Add helper functions for fault handling")
---
 arch/s390/kvm/faultin.c   |  4 ++--
 arch/s390/kvm/intercept.c | 23 +++++++++++++----------
 arch/s390/kvm/pv.c        |  2 +-
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/arch/s390/kvm/faultin.c b/arch/s390/kvm/faultin.c
index fee80047bd94..4ff4c5970956 100644
--- a/arch/s390/kvm/faultin.c
+++ b/arch/s390/kvm/faultin.c
@@ -92,8 +92,8 @@ int kvm_s390_faultin_gfn(struct kvm_vcpu *vcpu, struct kvm *kvm, struct guest_fa
 		if (is_noslot_pfn(f->pfn))
 			return PGM_ADDRESSING;
 		/* Signal pending: try again. */
-		if (f->pfn == KVM_PFN_ERR_SIGPENDING)
-			return -EAGAIN;
+		if (is_sigpending_pfn(f->pfn))
+			return -EINTR;
 		/* Check if it's read-only memory; don't try to actually handle that case. */
 		if (f->pfn == KVM_PFN_ERR_RO_FAULT)
 			return -EOPNOTSUPP;
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 1980df61ef30..a40fe2a10cbf 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -369,27 +369,30 @@ static int handle_mvpg_pei(struct kvm_vcpu *vcpu)
 	if (rc)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
 
-	do {
-		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(srcaddr), false);
-	} while (rc == -EAGAIN);
-	if (rc)
-		return rc;
-
-	/* Ensure that the source is paged-in, no actual access -> no key checking */
+	/* Ensure that the destination is paged-in, no actual access -> no key checking */
 	rc = guest_translate_address_with_key(vcpu, vcpu->run->s.regs.gprs[reg1],
 					      reg1, &dstaddr, GACC_STORE, 0);
 	if (rc)
 		return kvm_s390_inject_prog_cond(vcpu, rc);
 
+	/*
+	 * If an error happens below, the instruction needs to be retried.
+	 * Positive error codes are impossible; addressing exceptions would
+	 * have been discovered and reported above.
+	 */
+	kvm_s390_retry_instr(vcpu);
+
 	do {
-		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(dstaddr), true);
+		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(srcaddr), false);
 	} while (rc == -EAGAIN);
 	if (rc)
 		return rc;
 
-	kvm_s390_retry_instr(vcpu);
+	do {
+		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(dstaddr), true);
+	} while (rc == -EAGAIN);
 
-	return 0;
+	return rc;
 }
 
 static int handle_partial_execution(struct kvm_vcpu *vcpu)
diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index 1beacc841ca8..dc204b521052 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -809,7 +809,7 @@ static int unpack_one(struct kvm *kvm, unsigned long addr, u64 tweak,
 			return -EAGAIN;
 	}
 
-	if (ret && ret != -EAGAIN)
+	if (ret && ret != -EAGAIN && ret != -EINTR)
 		KVM_UV_EVENT(kvm, 3, "PROTVIRT VM UNPACK: failed addr %llx with rc %x rrc %x",
 			     uvcb.gaddr, *rc, *rrc);
 	return ret;
-- 
2.55.0

[PATCH v4 08/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

[Claudio Imbrenda]

The mmu cache is the first thing that is allocated in
kvm_arch_vcpu_create(), but in case of failure it was not freed.

Fix by freeing the mmu cache in case of failure.

Refactor kvm_arch_vcpu_create() to use scope-based cleanup instead of
gotos.

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

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 47eba1664bec..776dfa0ef970 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3795,21 +3795,21 @@ int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
 	return 0;
 }
 
+DEFINE_FREE(sie_page, struct sie_page *, if (_T) free_page((unsigned long)(_T)))
+
 int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 {
-	struct sie_page *sie_page;
+	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
+	struct sie_page *sie_page __free(sie_page) = NULL;
 	int rc;
 
 	BUILD_BUG_ON(sizeof(struct sie_page) != 4096);
-	vcpu->arch.mc = kvm_s390_new_mmu_cache();
-	if (!vcpu->arch.mc)
+	mc = kvm_s390_new_mmu_cache();
+	if (!mc)
 		return -ENOMEM;
 	sie_page = (struct sie_page *) get_zeroed_page(GFP_KERNEL_ACCOUNT);
-	if (!sie_page) {
-		kvm_s390_free_mmu_cache(vcpu->arch.mc);
-		vcpu->arch.mc = NULL;
+	if (!sie_page)
 		return -ENOMEM;
-	}
 
 	vcpu->arch.sie_block = &sie_page->sie_block;
 	vcpu->arch.sie_block->itdba = virt_to_phys(&sie_page->itdb);
@@ -3851,10 +3851,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 		vcpu->run->kvm_valid_regs |= KVM_SYNC_FPRS;
 
 	if (kvm_is_ucontrol(vcpu->kvm)) {
-		rc = -ENOMEM;
 		vcpu->arch.gmap = gmap_new_child(vcpu->kvm->arch.gmap, -1UL);
 		if (!vcpu->arch.gmap)
-			goto out_free_sie_block;
+			return -ENOMEM;
 	}
 
 	VM_EVENT(vcpu->kvm, 3, "create cpu %d at 0x%p, sie block at 0x%p",
@@ -3862,22 +3861,19 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
 	trace_kvm_s390_create_vcpu(vcpu->vcpu_id, vcpu, vcpu->arch.sie_block);
 
 	rc = kvm_s390_vcpu_setup(vcpu);
-	if (rc)
-		goto out_ucontrol_uninit;
+	if (rc) {
+		if (kvm_is_ucontrol(vcpu->kvm)) {
+			scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
+				gmap_remove_child(vcpu->arch.gmap);
+			vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
+		}
+		return rc;
+	}
 
+	vcpu->arch.mc = no_free_ptr(mc);
+	sie_page = NULL;
 	kvm_s390_update_topology_change_report(vcpu->kvm, 1);
 	return 0;
-
-out_ucontrol_uninit:
-	if (kvm_is_ucontrol(vcpu->kvm)) {
-		spin_lock(&vcpu->kvm->arch.gmap->children_lock);
-		gmap_remove_child(vcpu->arch.gmap);
-		spin_unlock(&vcpu->kvm->arch.gmap->children_lock);
-		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
-	}
-out_free_sie_block:
-	free_page((unsigned long)(vcpu->arch.sie_block));
-	return rc;
 }
 
 int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
-- 
2.55.0

[PATCH v4 09/13] KVM: s390: Fix ordering when adding to SCA

[Claudio Imbrenda]

When adding a new vCPU to the SCA area, the validity bit in the MCN was
set before the pointer to the state description, potentially allowing
for a race.

Fix by setting the pointer before setting the bit.

Fixes: 14542a0a54c5 ("KVM: S390: Remove sca_lock")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 776dfa0ef970..59f784f8c01b 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3481,8 +3481,8 @@ static void sca_add_vcpu(struct kvm_vcpu *vcpu)
 	if (!kvm_s390_use_sca_entries())
 		return;
 
+	WRITE_ONCE(sca->cpu[vcpu->vcpu_id].sda, virt_to_phys(vcpu->arch.sie_block));
 	set_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);
-	sca->cpu[vcpu->vcpu_id].sda = virt_to_phys(vcpu->arch.sie_block);
 }
 
 static int sca_can_add_vcpu(struct kvm *kvm, unsigned int id)
-- 
2.55.0

[PATCH v4 10/13] KVM: s390: Fix cleanup in kvm_s390_pv_create_cpu()

[Claudio Imbrenda]

If creating a protected vCPU in kvm_s390_pv_create_cpu() fails,
kvm_s390_pv_destroy_cpu() was called, which checks whether the vCPU has
a PV handle and exits doing nothing otherwise. At that point, due to
not having created the protected vCPU, the PV handle will not be set,
and kvm_s390_pv_destroy_cpu() will do nothing, thus leaking the
allocated memory.

Fix by factoring out the code to free and reset a PV vCPU; call it from
kvm_s390_pv_destroy_cpu() and kvm_s390_pv_create_cpu().

Opportunistically fix the return value of kvm_s390_pv_destroy_cpu() in
case of errors: return -EIO instead if EIO.

Fixes: d4074324b07a ("KVM: s390: pv: avoid double free of sida page")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
---
 arch/s390/kvm/pv.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index dc204b521052..b02e0159d3cd 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -244,6 +244,24 @@ static void kvm_s390_clear_pv_state(struct kvm *kvm)
 	kvm->arch.pv.stor_var = NULL;
 }
 
+static void kvm_s390_pv_dispose_cpu(struct kvm_vcpu *vcpu, bool free_stor_base)
+{
+	if (free_stor_base)
+		free_pages(vcpu->arch.pv.stor_base, get_order(uv_info.guest_cpu_stor_len));
+	free_page((unsigned long)sida_addr(vcpu->arch.sie_block));
+	vcpu->arch.sie_block->pv_handle_cpu = 0;
+	vcpu->arch.sie_block->pv_handle_config = 0;
+	memset(&vcpu->arch.pv, 0, sizeof(vcpu->arch.pv));
+	vcpu->arch.sie_block->sdf = 0;
+	/*
+	 * The sidad field (for sdf == 2) is now the gbea field (for sdf == 0).
+	 * Use the reset value of gbea to avoid leaking the kernel pointer of
+	 * the just freed sida.
+	 */
+	vcpu->arch.sie_block->gbea = 1;
+	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+}
+
 int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
 {
 	int cc;
@@ -258,24 +276,9 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
 	WARN_ONCE(cc, "protvirt destroy cpu failed rc %x rrc %x", *rc, *rrc);
 
 	/* Intended memory leak for something that should never happen. */
-	if (!cc)
-		free_pages(vcpu->arch.pv.stor_base,
-			   get_order(uv_info.guest_cpu_stor_len));
-
-	free_page((unsigned long)sida_addr(vcpu->arch.sie_block));
-	vcpu->arch.sie_block->pv_handle_cpu = 0;
-	vcpu->arch.sie_block->pv_handle_config = 0;
-	memset(&vcpu->arch.pv, 0, sizeof(vcpu->arch.pv));
-	vcpu->arch.sie_block->sdf = 0;
-	/*
-	 * The sidad field (for sdf == 2) is now the gbea field (for sdf == 0).
-	 * Use the reset value of gbea to avoid leaking the kernel pointer of
-	 * the just freed sida.
-	 */
-	vcpu->arch.sie_block->gbea = 1;
-	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+	kvm_s390_pv_dispose_cpu(vcpu, !cc);
 
-	return cc ? EIO : 0;
+	return cc ? -EIO : 0;
 }
 
 int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
@@ -319,9 +322,7 @@ int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu, u16 *rc, u16 *rrc)
 		     uvcb.header.rrc);
 
 	if (cc) {
-		u16 dummy;
-
-		kvm_s390_pv_destroy_cpu(vcpu, &dummy, &dummy);
+		kvm_s390_pv_dispose_cpu(vcpu, true);
 		return -EIO;
 	}
 
-- 
2.55.0

[PATCH v4 11/13] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory

[Claudio Imbrenda]

When low on memory, kvm_arch_commit_memory_region() might not perform
the required action completely, and instead just print a warning and
return. This allows the VM to continue running with an inconsistent
state.

Fix by refilling the struct kvm_s390_mmu_cache and trying again. A true
failure only happens if the refill fails, or if the cache cannot be
allocated at all; in such case print a warning and kill the VM. Such a
failure cannot currently happen.

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

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 59f784f8c01b..b151d25a4a4c 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5817,7 +5817,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 {
 	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
 	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;
-	int rc = 0;
+	int rc = -ENOMEM;
 
 	guard(mutex)(&kvm->slots_arch_lock);
 
@@ -5825,11 +5825,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 		return;
 
 	mc = kvm_s390_new_mmu_cache();
-	if (!mc) {
-		rc = -ENOMEM;
+	if (!mc)
 		goto out;
-	}
-
+retry:
 	scoped_guard(write_lock, &kvm->mmu_lock) {
 		if (kvm->arch.migration_mode && kvm->arch.use_cmma && old) {
 			_dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
@@ -5852,11 +5850,17 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 		case KVM_MR_FLAGS_ONLY:
 			break;
 		default:
+			rc = 0;
 			WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
 		}
 	}
+	if (rc == -ENOMEM) {
+		rc = kvm_s390_mmu_cache_topup(mc);
+		if (!rc)
+			goto retry;
+	}
 out:
-	if (rc)
+	if (KVM_BUG_ON(rc, kvm))
 		pr_warn("failed to commit memory region\n");
 	return;
 }
-- 
2.55.0

[PATCH v4 12/13] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

[Claudio Imbrenda]

In some cases kvm_s390_vcpu_unsetup_cmma() can be called with a 0
cbrlo; in such cases, if running with V != R, free_page() will attempt
to free physical page 0.

Fix by freeing cbrlo only if it's non-zero.

Fixes: b31605c12f4e ("KVM: s390: make cmma usage conditionally")
Fixes: 29b40f105ec8 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b151d25a4a4c..84c6f77e9fc9 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -3677,7 +3677,8 @@ static void kvm_s390_vcpu_crypto_setup(struct kvm_vcpu *vcpu)
 
 void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu)
 {
-	free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
+	if (vcpu->arch.sie_block->cbrlo)
+		free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
 	vcpu->arch.sie_block->cbrlo = 0;
 }
 
-- 
2.55.0

[PATCH v4 13/13] KVM: s390: Fix sca_clear_ext_call() for UCONTROL

[Claudio Imbrenda]

When cleaning up a UCONTROL VM, sca_clear_ext_call() will touch memory
outside of the allocated ESCA block, and UCONTROL VMs don't even use
ESCA.

Fix by not touching ESCA for UCONTROL VMs.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Fixes: 7d43bafcff17 ("KVM: s390: Make provisions for ESCA utilization")
---
 arch/s390/kvm/interrupt.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 2acdff130fa6..8d146a0681dc 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -84,10 +84,13 @@ static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
 static void sca_clear_ext_call(struct kvm_vcpu *vcpu)
 {
 	struct esca_block *sca = vcpu->kvm->arch.sca;
-	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
+	union esca_sigp_ctrl *sigp_ctrl;
 
-	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
+	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized || kvm_is_ucontrol(vcpu->kvm))
 		return;
+
+	/* Initialize after the above check, to prevent going out of bounds */
+	sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
 	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);
 
 	WRITE_ONCE(sigp_ctrl->value, 0);
-- 
2.55.0

Re: [PATCH v4 10/13] KVM: s390: Fix cleanup in kvm_s390_pv_create_cpu()

[sashiko-bot]

> If creating a protected vCPU in kvm_s390_pv_create_cpu() fails,
> kvm_s390_pv_destroy_cpu() was called, which checks whether the vCPU has
> a PV handle and exits doing nothing otherwise. At that point, due to
> not having created the protected vCPU, the PV handle will not be set,
> and kvm_s390_pv_destroy_cpu() will do nothing, thus leaking the
> allocated memory.
> 
> Fix by factoring out the code to free and reset a PV vCPU; call it from
> kvm_s390_pv_destroy_cpu() and kvm_s390_pv_create_cpu().
> 
> Opportunistically fix the return value of kvm_s390_pv_destroy_cpu() in
> case of errors: return -EIO instead if EIO.
> 
> Fixes: d4074324b07a ("KVM: s390: pv: avoid double free of sida page")
> 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/20260728170708.294400-1-imbrenda@linux.ibm.com?part=10

Re: [PATCH v4 09/13] KVM: s390: Fix ordering when adding to SCA

[sashiko-bot]

> When adding a new vCPU to the SCA area, the validity bit in the MCN was
> set before the pointer to the state description, potentially allowing
> for a race.
> 
> Fix by setting the pointer before setting the bit.
> 
> Fixes: 14542a0a54c5 ("KVM: S390: Remove sca_lock")
> 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/20260728170708.294400-1-imbrenda@linux.ibm.com?part=9

Re: [PATCH v4 02/13] KVM: s390: Fix leaking of PGM_ADDRESSING to userspace

[sashiko-bot]

> If kvm_s390_set_cmma_bits() is asked to set CMMA values outside of a
> memslot, PGM_ADDRESSING (5) is returned, instead of a negative error
> value.
> 
> Same issue with kvm_s390_{g,s}et_skeys(), kvm_s390_keyop(), and
> dat_reset_reference_bit().
> 
> Fix by returning -EFAULT whenever the return value would be > 0, which
> is consistent with the behaviour before the gmap rewrite.
> 
> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> 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/20260728170708.294400-1-imbrenda@linux.ibm.com?part=2

Re: [PATCH v4 05/13] KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

[sashiko-bot]

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

Pre-existing issues:
- [High] The struct kvm_s390_mmu_cache (vcpu->arch.mc) is permanently leaked in the error cleanup paths of kvm_arch_vcpu_create().
--

--- Patch [5]: [PATCH v4 05/13] KVM: s390: ucontrol: Add missing locking around gmap_remove_child() ---
commit 8377be474458b5d68906db9024e50f7b0c2263fe
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

gmap_remove_child() needs to be called while holding the children_lock
of the parent gmap. This was not the case in the error handling path of
kvm_arch_vcpu_create() for UCONTROL guests.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 5046de39d739e..fa6a0bacd9675 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3868,7 +3868,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  out_ucontrol_uninit:
>  	if (kvm_is_ucontrol(vcpu->kvm)) {
> +		spin_lock(&vcpu->kvm->arch.gmap->children_lock);
>  		gmap_remove_child(vcpu->arch.gmap);
> +		spin_unlock(&vcpu->kvm->arch.gmap->children_lock);
>  		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
>  	}
>  out_free_sie_block:
>  	free_page((unsigned long)(vcpu->arch.sie_block));
>  	return rc;

[Severity: High]
This is a pre-existing issue, but does this error path leak vcpu->arch.mc?

In kvm_arch_vcpu_create(), vcpu->arch.mc is dynamically allocated via
kvm_s390_new_mmu_cache(). If a subsequent step like gmap_new_child() or
kvm_s390_vcpu_setup() fails, the code jumps here to out_ucontrol_uninit
or out_free_sie_block.

Since these paths return an error without calling
kvm_s390_free_mmu_cache(vcpu->arch.mc), and the core KVM code in
kvm_vm_ioctl_create_vcpu() does not invoke kvm_arch_vcpu_destroy() when
creation fails, it appears this memory could be permanently leaked.

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

Re: [PATCH v4 03/13] KVM: s390: Fix race in __do_essa()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Meaningful code, documentation, and commit message mismatch regarding which lock is used.
--

--- Patch [3]: [PATCH v4 03/13] KVM: s390: Fix race in __do_essa() ---
commit 44561620d155e5433777595f8bc3b2d06087934d
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix race in __do_essa()

This patch fixes a potential race condition between __do_essa() and the 
migration and CMMA state handling functions. It transitions the affected paths 
to use the architecture-specific slots lock via scope-based guards.

> KVM: s390: Fix race in __do_essa()
>
> An unlikely race between __do_essa() and kvm_s390_vm_start_migration(),
> kvm_s390_vm_stop_migration(), or dat_get_cmma() was possible.
>
> Fix by locking kvm->slots_lock. Since this is not a hot path, the
> overhead of an additional mutex is negligible.

[Severity: Medium]
This isn't a bug, but the commit message states the fix uses kvm->slots_lock,
while the code changes actually use kvm->slots_arch_lock. Should the commit
message be updated to reflect the use of the architecture-specific lock?

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 89de085f39670..1952f592a80a1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1298,7 +1298,9 @@ static int kvm_s390_vm_set_migration(struct kvm *kvm,
>  {
>  	int res = -ENXIO;
>  
> -	mutex_lock(&kvm->slots_lock);
> +	guard(srcu)(&kvm->srcu);
> +	guard(mutex)(&kvm->slots_arch_lock);

[Severity: Medium]
This isn't a bug, but the function documentation for kvm_s390_vm_start_migration()
and kvm_s390_vm_stop_migration() (which are called from here) still states they
must be called with kvm->slots_lock held:

arch/s390/kvm/kvm-s390.c:kvm_s390_vm_start_migration():
/*
 * Must be called with kvm->srcu held to avoid races on memslots, and with
 * kvm->slots_lock to avoid races with ourselves, kvm_s390_vm_stop_migration(),
 * and kvm_s390_get_cmma_bits().
 */
static int kvm_s390_vm_start_migration(struct kvm *kvm)

Since these paths now use kvm->slots_arch_lock instead, would it be helpful
to update those comments to match the new locking behavior?

[ ... ]

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

Re: [PATCH v4 04/13] KVM: s390: cmma: Fix dirty tracking when removing memslot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double-decrement of `cmma_dirty_pages` counter on `-ENOMEM` retry paths
- [Medium] Mixing goto-based error handling and split definition-initialization with scope-based cleanup helpers (`__free` and `guard`)
--

--- Patch [4]: [PATCH v4 04/13] KVM: s390: cmma: Fix dirty tracking when removing memslot ---
commit 335967b9cb5dc29aafc1953486ce5288ed03df4d
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: cmma: Fix dirty tracking when removing memslot
    
This patch fixes cmma-dirty tracking by appropriately decrementing the
cmma-dirty counter when a memslot is removed. It also opportunistically
switches kvm_arch_commit_memory_region() to use __free() for the MMU
cache.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1952f592a80a1..5046de39d739e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -5803,14 +5803,24 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>  	return 0;
>  }
>  
> +static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
> +{
> +	if (pgste_of(ptep)->cmma_d)
> +		atomic64_dec(walk->priv);
> +	return 0;
> +}
> +
>  void kvm_arch_commit_memory_region(struct kvm *kvm,
>  				struct kvm_memory_slot *old,
>  				const struct kvm_memory_slot *new,
>  				enum kvm_mr_change change)
>  {
> -	struct kvm_s390_mmu_cache *mc = NULL;
> +	const struct dat_walk_ops ops = { .pte_entry = cmma_d_count_pte, };
> +	struct kvm_s390_mmu_cache *mc __free(kvm_s390_mmu_cache) = NULL;

[Severity: Medium]
This isn't a bug, but does mixing goto-based error handling and split
definition-initialization with scope-based cleanup helpers go against the
cleanup subsystem guidelines?

In kvm_arch_commit_memory_region(), mc is initialized to NULL at the
function start. Later, the code continues to handle errors with a goto:

    mc = kvm_s390_new_mmu_cache();
    if (!mc)
        goto out;

Could the goto out; be replaced with a direct return to avoid mixing
cleanup patterns, and could the variable be declared at the point of
allocation?

>  	int rc = 0;
>  
> +	guard(mutex)(&kvm->slots_arch_lock);
> +
>  	if (change == KVM_MR_FLAGS_ONLY)
>  		return;
[ ... ]
> @@ -5821,6 +5831,12 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  	}
>  
>  	scoped_guard(write_lock, &kvm->mmu_lock) {
> +		if (kvm->arch.migration_mode && kvm->arch.use_cmma && old) {
> +			_dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
> +					    kvm->arch.gmap->asce, &ops, DAT_WALK_IGN_HOLES,
> +					    &kvm->arch.cmma_dirty_pages);
> +		}
> +
>  		switch (change) {
>  		case KVM_MR_DELETE:
>  			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);

[Severity: High]
Can this result in a double-decrement of the cmma_dirty_pages counter?

Looking at kvm_arch_commit_memory_region(), this block executes inside a
retry loop:

    retry:
        scoped_guard(write_lock, &kvm->mmu_lock) {
            if (kvm->arch.migration_mode && kvm->arch.use_cmma && old) {
                _dat_walk_gfn_range(..., &ops, ...);
            }
            
            switch (change) {
            case KVM_MR_DELETE:
                rc = dat_delete_slot(...);
                ...
        }
        if (rc == -ENOMEM) {
            rc = kvm_s390_mmu_cache_topup(mc);
            if (!rc)
                goto retry;
        }

If dat_delete_slot() clears cmma_d on some PTEs and then fails with
-ENOMEM (e.g. running out of MMU cache while splitting a page table), we 
top up the cache and jump back to the retry label. The pre-pass
_dat_walk_gfn_range() runs again, but cmma_d_count_pte() doesn't clear the
cmma_d bit on the unhandled PTEs during its traversal.

Would the second pass decrement the counter again for the PTEs that weren't
processed before the -ENOMEM failure, potentially causing the counter to
underflow?

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

Re: [PATCH v4 12/13] KVM: s390: Fix kvm_s390_vcpu_unsetup_cmma()

[sashiko-bot]

> In some cases kvm_s390_vcpu_unsetup_cmma() can be called with a 0
> cbrlo; in such cases, if running with V != R, free_page() will attempt
> to free physical page 0.
> 
> Fix by freeing cbrlo only if it's non-zero.
> 
> Fixes: b31605c12f4e ("KVM: s390: make cmma usage conditionally")
> Fixes: 29b40f105ec8 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling")
> 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/20260728170708.294400-1-imbrenda@linux.ibm.com?part=12

Re: [PATCH v4 11/13] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] User-triggerable KVM_BUG_ON (WARN_ON_ONCE) on memcg OOM.
--

--- Patch [11]: [PATCH v4 11/13] KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory ---
commit 9bd2f3fe880640681d24faea9af3bd26f006d538
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix kvm_arch_commit_memory_region() when low on memory
    
When low on memory, kvm_arch_commit_memory_region() attempts to refill
the MMU cache and retry. If the refill fails, it kills the VM using
KVM_BUG_ON.

> 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
[ ... ]
> @@ -5850,11 +5848,17 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  		case KVM_MR_FLAGS_ONLY:
>  			break;
>  		default:
> +			rc = 0;
>  			WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
>  		}
>  	}
> +	if (rc == -ENOMEM) {
> +		rc = kvm_s390_mmu_cache_topup(mc);
> +		if (!rc)
> +			goto retry;
> +	}
>  out:
> -	if (rc)
> +	if (KVM_BUG_ON(rc, kvm))

[Severity: High]
Could this KVM_BUG_ON() be triggered by a host-userspace process facing
memory pressure?

If an unprivileged userspace process creates a VM within a restricted memory
cgroup (memcg), and then calls KVM_SET_USER_MEMORY_REGION to modify a
memslot, kvm_s390_mmu_cache_topup() could return -ENOMEM due to the memcg
limit.

Since KVM_BUG_ON expands to WARN_ON_ONCE, wouldn't this allow a malicious
guest or unprivileged host process to cause a host-side denial of service
if panic_on_warn is enabled?

>  		pr_warn("failed to commit memory region\n");
>  	return;
>  }

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

Re: [PATCH v4 01/13] KVM: s390: Fix unlikely NULL gmap dereference

[sashiko-bot]

> When creating a new vCPU, kvm_vm_ioctl_create_vcpu() will call
> kvm_arch_vcpu_postcreate() after the file descriptor for the new vCPU
> has been created. The new file descriptor has not been returned yet,
> but a malicious userspace program could try to guess it.
> 
> If a malicious userspace program manages to start the newly created vCPU
> before kvm_arch_vcpu_postcreate() is called, __vcpu_run() will try to
> dereference vcpu->arch.gmap and trigger a NULL pointer dereference.
> 
> Fix this by adding a new field to struct kvm_vcpu_arch to keep track of
> the initialization status of the vCPU. Refuse to run a vCPU that is not
> fully initialized.
> 
> Fixes: dafd032a15f8 ("KVM: s390: move vcpu specific initalization to a later point")
> Fixes: e38c884df921 ("KVM: s390: Switch to new gmap")
> 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/20260728170708.294400-1-imbrenda@linux.ibm.com?part=1

Re: [PATCH v4 06/13] KVM: s390: Fix overclearing ESCA in case of error

[sashiko-bot]

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

Pre-existing issues:
- [High] Memory leak of MMU cache on kvm_arch_vcpu_create() error path.
- [High] Out-of-bounds array access and kernel memory corruption in ESCA interrupt handlers for UCONTROL VMs due to unconstrained vcpu_id.
--

--- Patch [6]: [PATCH v4 06/13] KVM: s390: Fix overclearing ESCA in case of error ---
commit a40c7240ce30546ef77f030cf591a1bc94d9f75b
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix overclearing ESCA in case of error

If an attempt is made to create a vCPU with an already existing ID, the
duplicated vCPU will be destroyed. This patch avoids clearing the ESCA
entry if the vCPU creation was not successful.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 9e3e6b0d72ad6..2acdff130fa6c 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -86,7 +86,7 @@ static void sca_clear_ext_call(struct kvm_vcpu *vcpu)
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
>  	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>  
> -	if (!kvm_s390_use_sca_entries())
> +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
>  		return;

[Severity: High]
This is a pre-existing issue, but does this pointer calculation allow an
out-of-bounds access for UCONTROL VMs?

UCONTROL VMs bypass the sca_can_add_vcpu() bounds check during
kvm_arch_vcpu_precreate(), which allows vcpu_id to reach up to 254 instead
of the array limit of 247.

When a successfully initialized UCONTROL vCPU is destroyed,
kvm_arch_vcpu_destroy() unconditionally calls kvm_s390_clear_local_irqs(),
which invokes sca_clear_ext_call().

Because the sigp_ctrl pointer is computed without verifying if the VM is a
UCONTROL VM, a vcpu_id of 254 resolves to an offset 132 bytes beyond the
allocated esca_block.

Could the subsequent WRITE_ONCE(sigp_ctrl->value, 0) further down in the
function then overwrite adjacent kernel memory?

>  	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);
>  
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index fa6a0bacd9675..24b48dc5655b1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -3459,7 +3459,7 @@ static void sca_del_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
>  
> -	if (!kvm_s390_use_sca_entries())
> +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
>  		return;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path for vCPU 
creation leak the MMU cache?

In kvm_arch_vcpu_create(), vcpu->arch.mc is dynamically allocated:

arch/s390/kvm/kvm-s390.c:kvm_arch_vcpu_create() {
    ...
    vcpu->arch.mc = kvm_s390_new_mmu_cache();
    ...
}

If a subsequent initialization step fails (such as gmap_new_child() failing
for UCONTROL VMs, or kvm_s390_vcpu_setup() returning an error), the function
jumps to the out_free_sie_block label.

While this error path frees vcpu->arch.sie_block, it completely neglects to
call kvm_s390_free_mmu_cache(vcpu->arch.mc).

Since the caller (kvm_vm_ioctl_create_vcpu()) then directly frees the vcpu
struct without calling kvm_arch_vcpu_destroy(), does this permanently leak
the kvm_s390_mmu_cache and its associated page tables?

>  
>  	clear_bit_inv(vcpu->vcpu_id, (unsigned long *)sca->mcn);

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

Re: [PATCH v4 08/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unconditional call to `kvm_s390_vcpu_unsetup_cmma()` when PV VCPU creation fails causes freeing of physical page 0 if CMMA is disabled, leading to a host panic.
--

--- Patch [8]: [PATCH v4 08/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails ---
commit 30d38fd7067b631b7997fead40df5a9ae47fcfb8
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

This commit fixes a memory leak where the mmu cache is not freed if
vcpu creation fails. It also refactors kvm_arch_vcpu_create() to use
scope-based cleanup instead of goto-based error handling.

> 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

[ ... ]

> @@ -3849,10 +3849,9 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	VM_EVENT(vcpu->kvm, 3, "create cpu %d at 0x%p, sie block at 0x%p",
>  		 vcpu->vcpu_id, vcpu, vcpu->arch.sie_block);
>  	trace_kvm_s390_create_vcpu(vcpu->vcpu_id, vcpu, vcpu->arch.sie_block);
>  
>  	rc = kvm_s390_vcpu_setup(vcpu);

[Severity: Critical]
This is a pre-existing issue, but could this lead to a host panic if a
Protected VM fails to create a VCPU and CMMA is disabled?

If kvm_s390_pv_create_cpu() fails (e.g., hitting a cgroup memory limit)
inside kvm_s390_vcpu_setup(), it unconditionally unsets CMMA:

kvm_s390_vcpu_setup() {
    ...
	if (kvm_s390_pv_is_protected(vcpu->kvm)) {
		rc = kvm_s390_pv_create_cpu(vcpu, &uvrc, &uvrrc);
		if (rc)
			kvm_s390_vcpu_unsetup_cmma(vcpu);
	}
    ...
}

If use_cmma is false, vcpu->arch.sie_block->cbrlo is 0. 
kvm_s390_vcpu_unsetup_cmma() then executes:

kvm_s390_vcpu_unsetup_cmma() {
    ...
	free_page((unsigned long)phys_to_virt(vcpu->arch.sie_block->cbrlo));
    ...
}

With KASLR enabled, phys_to_virt(0) evaluates to __identity_base 
(a non-zero address), which bypasses the safety check in free_pages(). 
Does this erroneously free physical page 0 (the lowcore page), leading 
to memory corruption or a fatal VM_BUG_ON_PAGE panic on the host?

> -	if (rc)
> -		goto out_ucontrol_uninit;
> +	if (rc) {
> +		if (kvm_is_ucontrol(vcpu->kvm)) {
> +			scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
> +				gmap_remove_child(vcpu->arch.gmap);
> +			vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
> +		}
> +		return rc;
> +	}
>  
> +	vcpu->arch.mc = no_free_ptr(mc);
> +	sie_page = NULL;
>  	kvm_s390_update_topology_change_report(vcpu->kvm, 1);
>  	return 0;

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

Re: [PATCH v4 07/13] KVM: s390: Return -EINTR if a signal was pending while faulting-in

[sashiko-bot]

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

Pre-existing issues:
- [High] Nested virtualization (vSIE) skips the SIE instruction and corrupts guest state when a signal interrupts the shadow page fault handler.
- [High] Instruction handlers using read_guest/write_guest (e.g., STSI, LPSW) silently skip execution if a signal is pending during page fault-in.
--

--- Patch [7]: [PATCH v4 07/13] KVM: s390: Return -EINTR if a signal was pending while faulting-in ---
commit 1e3636aa03c57bfbcde97da61dcda6802d420d61
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Return -EINTR if a signal was pending while faulting-in

If a signal is pending while trying to fault-in a page, return -EINTR
instead of -EAGAIN. Also fix unpack_one() and handle_mvpg_pei() so
that they can handle -EINTR properly.

> diff --git a/arch/s390/kvm/faultin.c b/arch/s390/kvm/faultin.c
> index fee80047bd94f..4ff4c59709566 100644
> --- a/arch/s390/kvm/faultin.c
> +++ b/arch/s390/kvm/faultin.c
> @@ -92,8 +92,8 @@ int kvm_s390_faultin_gfn(struct kvm_vcpu *vcpu, struct kvm *kvm, struct guest_fa
>  		if (is_noslot_pfn(f->pfn))
>  			return PGM_ADDRESSING;
>  		/* Signal pending: try again. */
> -		if (f->pfn == KVM_PFN_ERR_SIGPENDING)
> -			return -EAGAIN;
> +		if (is_sigpending_pfn(f->pfn))
> +			return -EINTR;

[Severity: High]
This is a pre-existing issue, but does propagating this error cause
nested virtualization (vSIE) to skip the SIE instruction when a signal
interrupts the shadow page fault handler?

If a signal is sent to the vCPU thread while handling a nested fault,
gaccess_shadow_fault() calls kvm_s390_get_guest_page(), which returns -EINTR.

This error propagates without rewinding the L2 guest PSW via
retry_vsie_icpt() or kvm_s390_rewind_psw():

arch/s390/kvm/vsie.c:vsie_handle_mvpg() {
    ...
    if (rc_dest < 0)
        return rc_dest;
    ...
}

arch/s390/kvm/vsie.c:vsie_run() {
    ...
    if (rc || scb_s->icptcode)
        break;
    ...
}

When userspace resumes KVM_RUN, does execution continue at the advanced PSW,
skipping the SIE instruction and corrupting L2 guest state? Does vSIE need
to be addressed similarly to how handle_mvpg_pei() is fixed in this patch?

>  		/* Check if it's read-only memory; don't try to actually handle that case. */
>  		if (f->pfn == KVM_PFN_ERR_RO_FAULT)
>  			return -EOPNOTSUPP;
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 1980df61ef304..a40fe2a10cbfe 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
[ ... ]
> @@ -369,27 +369,30 @@ static int handle_mvpg_pei(struct kvm_vcpu *vcpu)
>  	if (rc)
>  		return kvm_s390_inject_prog_cond(vcpu, rc);
>  
> -	do {
> -		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(srcaddr), false);
> -	} while (rc == -EAGAIN);
> -	if (rc)
> -		return rc;
> -
> -	/* Ensure that the source is paged-in, no actual access -> no key checking */
> +	/* Ensure that the destination is paged-in, no actual access -> no key checking */
>  	rc = guest_translate_address_with_key(vcpu, vcpu->run->s.regs.gprs[reg1],
>  					      reg1, &dstaddr, GACC_STORE, 0);
>  	if (rc)
>  		return kvm_s390_inject_prog_cond(vcpu, rc);
>  
> +	/*
> +	 * If an error happens below, the instruction needs to be retried.
> +	 * Positive error codes are impossible; addressing exceptions would
> +	 * have been discovered and reported above.
> +	 */
> +	kvm_s390_retry_instr(vcpu);

[Severity: High]
This isn't a bug introduced by this patch, but while adding the instruction
retry logic here, do other instruction handlers using read_guest() or
write_guest() in arch/s390/kvm/priv.c also need this fix?

If a signal interrupts kvm_s390_faultin_gfn() during an instruction intercept,
the handler receives -EINTR (or -EAGAIN prior to this patch) and returns it
to userspace without calling kvm_s390_retry_instr().

Does this leave the guest's PSW at the instruction following the intercept,
causing the instruction to be skipped when userspace resumes the vCPU?

arch/s390/kvm/priv.c:handle_stsi() {
    ...
    if (rc) {
        rc = kvm_s390_inject_prog_cond(vcpu, rc);
        goto out;
    }
    ...
}

arch/s390/kvm/priv.c:kvm_s390_handle_lpsw() {
    ...
    rc = read_guest(vcpu, addr, ar, &new_psw, sizeof(new_psw));
    if (rc)
        return kvm_s390_inject_prog_cond(vcpu, rc);
    ...
}

> +
>  	do {
> -		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(dstaddr), true);
> +		rc = kvm_s390_faultin_gfn_simple(vcpu, NULL, gpa_to_gfn(srcaddr), false);
>  	} while (rc == -EAGAIN);
>  	if (rc)
>  		return rc;

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

Re: [PATCH v4 13/13] KVM: s390: Fix sca_clear_ext_call() for UCONTROL

[sashiko-bot]

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

New issues:
- [High] Asymmetric resource teardown of CPUSTAT_ECALL_PEND for UCONTROL VMs leading to a permanent stuck state.
- [High] Double-decrement of `cmma_dirty_pages` counter due to a retry loop.

Pre-existing issues:
- [Critical] Out-of-bounds memory access in `sca_inject_ext_call()` and `sca_ext_call_pending()` for UCONTROL VMs.
--

--- Patch [13]: [PATCH v4 13/13] KVM: s390: Fix sca_clear_ext_call() for UCONTROL ---
commit d27d3f9f6aa096af6e829ecfb494bcc68d320453
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>
Date:   Tue Jul 28 17:07:08 2026 +0000

    KVM: s390: Fix sca_clear_ext_call() for UCONTROL

This commit fixes an out-of-bounds access in sca_clear_ext_call() for UCONTROL
VMs by returning early before touching the ESCA block. UCONTROL VMs bypass ESCA
but can have vCPU IDs larger than the ESCA block capacity.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 2acdff130fa6c..8d146a0681dce 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -84,10 +84,13 @@ static int sca_inject_ext_call(struct kvm_vcpu *vcpu, int src_id)
>  static void sca_clear_ext_call(struct kvm_vcpu *vcpu)
>  {
>  	struct esca_block *sca = vcpu->kvm->arch.sca;
> -	union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
> +	union esca_sigp_ctrl *sigp_ctrl;
>  
> -	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized)
> +	if (!kvm_s390_use_sca_entries() || !vcpu->arch.initialized || kvm_is_ucontrol(vcpu->kvm))
>  		return;

[Severity: High]
Could this early return for UCONTROL VMs skip clearing the CPUSTAT_ECALL_PEND
flag?

Prior to this patch, if an external call was injected into a UCONTROL VM,
sca_inject_ext_call() would set the CPUSTAT_ECALL_PEND cpuflag, and
sca_clear_ext_call() would later clear it. By adding an early return here
without a corresponding check in sca_inject_ext_call(), the flag is set but
never cleared. Would this leave the VM in a permanently corrupted state across
resets?

[Severity: Critical]
This is a pre-existing issue, but UCONTROL VMs bypass the sca_can_add_vcpu()
constraints, allowing vCPU IDs up to KVM_MAX_VCPUS - 1 (254). The ESCA block
only holds KVM_S390_ESCA_CPU_SLOTS (248) entries.

While this patch prevents out-of-bounds access in sca_clear_ext_call(), it
fails to patch identical accesses in sca_inject_ext_call():

arch/s390/kvm/interrupt.c:sca_inject_ext_call() {
    ...
    union esca_sigp_ctrl *sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
    ...
}

and sca_ext_call_pending():

arch/s390/kvm/interrupt.c:sca_ext_call_pending() {
    ...
    union esca_sigp_ctrl sigp_ctrl = sca->cpu[vcpu->vcpu_id].sigp_ctrl;
    ...
}

The entry point for userspace interrupt injection, __inject_extcall(), calls
sca_inject_ext_call() based on host hardware capabilities without verifying
if the target VM is UCONTROL.

Can a malicious userspace process trigger these out-of-bounds accesses by
injecting external calls or polling for pending interrupts on a vCPU with
an ID >= 248?

> +
> +	/* Initialize after the above check, to prevent going out of bounds */
> +	sigp_ctrl = &sca->cpu[vcpu->vcpu_id].sigp_ctrl;
>  	kvm_s390_clear_cpuflags(vcpu, CPUSTAT_ECALL_PEND);
>  
>  	WRITE_ONCE(sigp_ctrl->value, 0);

[Severity: High]
I also noticed a separate issue introduced in commit 'KVM: s390: Fix
kvm_arch_commit_memory_region() when low on memory'.

Is there a risk that the retry loop in kvm_arch_commit_memory_region() causes
a double-decrement of the cmma_dirty_pages counter?

arch/s390/kvm/kvm-s390.c:kvm_arch_commit_memory_region() {
    ...
    retry:
        ...
        _dat_walk_gfn_range(old->base_gfn, old->base_gfn + old->npages,
                            kvm->arch.gmap->asce, &ops, DAT_WALK_IGN_HOLES,
                            &kvm->arch.cmma_dirty_pages);
    ...
}

Inside this loop, _dat_walk_gfn_range() uses cmma_d_count_pte() to decrement
kvm->arch.cmma_dirty_pages for every PTE where cmma_d is set:

arch/s390/kvm/kvm-s390.c:cmma_d_count_pte() {
    if (pgste_of(ptep)->cmma_d)
        atomic64_dec(walk->priv);
    ...
}

Because cmma_d_count_pte() does not clear the cmma_d bit, if
dat_delete_slot() or dat_create_slot() fails with -ENOMEM, the code jumps
to retry and executes the pass again over the exact same range.

Could this decrement the counter a second time for unchanged PTEs, leading to
an underflow and breaking CMMA dirty page tracking during VM migration?

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