[PATCH v1 0/6] KVM: s390: Even more misc fixes

[PATCH v1 0/6] KVM: s390: Even more misc fixes

[Claudio Imbrenda]

A few fixes that had been dropped from previous series due to still
having issues, plus a few new fixes.

Claudio Imbrenda (6):
  KVM: s390: Fix dirty marking in adapter_indicators_set*()
  KVM: s390: Fix _gaccess_shadow_fault()
  KVM: s390: Refactor dat_set_slot()
  KVM: s390: Move all code into kvm_arch_prepare_memory_region()
  KVM: s390: Add missing srcu in kvm_arch_vcpu_ioctl()
  KVM: s390: Fix potential race in dat_cond_set_storage_key()

 arch/s390/kvm/dat.c       |  37 +++++------
 arch/s390/kvm/dat.h       |  10 ++-
 arch/s390/kvm/gaccess.c   |  13 ++++
 arch/s390/kvm/interrupt.c |  40 ++++++------
 arch/s390/kvm/kvm-s390.c  | 129 +++++++++++++++++---------------------
 5 files changed, 113 insertions(+), 116 deletions(-)

-- 
2.55.0

[PATCH v1 1/6] 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 in KVM 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 mark_page_dirty(). Note that
for the inatomic path set_page_dirty{,_lock}() is not needed as the
page stays pinned; the unpin path correctly marks it as dirty.

Opportunistically reorder the local variables to be in reverse
Christmas tree order and refactor to use guard().

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 | 40 ++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index da740a378a8c..fc4d1f8193d9 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2984,12 +2984,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;
 
@@ -3000,14 +3002,11 @@ 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);
 		unpin_user_page(ind_page);
 	} else {
 		map = page_address(ind_info->page);
@@ -3015,6 +3014,7 @@ static int adapter_indicators_set(struct kvm *kvm,
 		set_bit(bit, map);
 		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);
@@ -3023,14 +3023,11 @@ static int adapter_indicators_set(struct kvm *kvm,
 		summary_page = pin_map_page(kvm, adapter_int->summary_addr, 0);
 		if (!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);
 		unpin_user_page(summary_page);
 	} else {
 		map = page_address(summary_info->page);
@@ -3039,6 +3036,7 @@ static int adapter_indicators_set(struct kvm *kvm,
 		summary_set = test_and_set_bit(bit, map);
 		spin_unlock_irqrestore(&adapter->maps_lock, flags);
 	}
+	mark_page_dirty(kvm, gpa_to_gfn(adapter_int->summary_gaddr));
 
 	return summary_set ? 0 : 1;
 }
@@ -3048,26 +3046,29 @@ 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) {
 		set_bit(bit, map);
+		mark_page_dirty(kvm, gpa_to_gfn(adapter_int->ind_gaddr));
+	}
+
 	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);
@@ -3077,7 +3078,8 @@ 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));
+
 	return summary_set ? 0 : 1;
 }
 
-- 
2.55.0

Re: [PATCH v1 1/6] KVM: s390: Fix dirty marking in adapter_indicators_set*()

[sashiko-bot]

> When the indicator and/or summary bits are set in the guest, the
> accessed page was only marked dirty in KVM 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 mark_page_dirty(). Note that
> for the inatomic path set_page_dirty{,_lock}() is not needed as the
> page stays pinned; the unpin path correctly marks it as dirty.
> 
> Opportunistically reorder the local variables to be in reverse
> Christmas tree order and refactor to use guard().
> 
> Fixes: 1e95e3bc6b05 ("KVM: s390: Enable adapter_indicators_set to use mapped pages")
> 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/20260814144041.169025-1-imbrenda@linux.ibm.com?part=1

[PATCH v1 2/6] 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 36102b2727fb..0d5d46d4c66f 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -1593,12 +1593,25 @@ static inline int ___gaccess_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg
 	parent = READ_ONCE(sg->parent);
 	if (!parent)
 		return -EAGAIN;
+retry:
 	scoped_guard(spinlock, &parent->children_lock) {
 		if (READ_ONCE(sg->parent) != parent)
 			return -EAGAIN;
 		sg->invalidated = false;
 		rc = _gaccess_do_shadow(vcpu->arch.mc, sg, saddr, walk);
 	}
+	if (rc == -ENOENT) {
+		struct kvm_memory_slot *slot;
+		struct guest_fault *entries;
+
+		entries = get_entries(walk);
+		slot = kvm_vcpu_gfn_to_memslot(vcpu, entries[LEVEL_MEM].gfn);
+		if (!slot)
+			return PGM_ADDRESSING;
+		rc = gmap_link(vcpu->arch.mc, sg->parent, entries + LEVEL_MEM, slot);
+		if (!rc)
+			goto retry;
+	}
 	if (!rc)
 		kvm_s390_release_faultin_array(vcpu->kvm, walk->raw_entries, false);
 	return rc;
-- 
2.55.0

Re: [PATCH v1 2/6] KVM: s390: Fix _gaccess_shadow_fault()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Accessing `sg->parent` outside of `children_lock` leads to a TOCTOU NULL pointer dereference if the shadow gmap is concurrently unshadowed.
--

--- Patch [2]: [PATCH v1 2/6] KVM: s390: Fix _gaccess_shadow_fault() ---
commit 64108760233a08b4a69c94be48f33b269ea23a85
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix _gaccess_shadow_fault()

This commit addresses an issue where nested guest memory is not present in
the parent guest gmap, causing dat_entry_walk() to return -ENOENT. It fixes
this by manually calling gmap_link() on the nested guest's memory if the
mapping is absent.

> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 36102b2727fbf..0d5d46d4c66fd 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -1593,12 +1593,25 @@ static inline int ___gaccess_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg
>  	parent = READ_ONCE(sg->parent);
>  	if (!parent)
>  		return -EAGAIN;
> +retry:
>  	scoped_guard(spinlock, &parent->children_lock) {
>  		if (READ_ONCE(sg->parent) != parent)
>  			return -EAGAIN;
>  		sg->invalidated = false;
>  		rc = _gaccess_do_shadow(vcpu->arch.mc, sg, saddr, walk);
>  	}
> +	if (rc == -ENOENT) {
> +		struct kvm_memory_slot *slot;
> +		struct guest_fault *entries;
> +
> +		entries = get_entries(walk);
> +		slot = kvm_vcpu_gfn_to_memslot(vcpu, entries[LEVEL_MEM].gfn);
> +		if (!slot)
> +			return PGM_ADDRESSING;
> +		rc = gmap_link(vcpu->arch.mc, sg->parent, entries + LEVEL_MEM, slot);

[Severity: Critical]
Could accessing sg->parent here lead to a NULL pointer dereference?

The scoped_guard block drops parent->children_lock before this code is
reached. If another vCPU concurrently unshadows sg, sg->parent could be
set to NULL. Because this passes sg->parent directly instead of using the
cached parent variable, does this risk passing NULL into gmap_link() and
causing a host kernel panic?

> +		if (!rc)
> +			goto retry;
> +	}
>  	if (!rc)
>  		kvm_s390_release_faultin_array(vcpu->kvm, walk->raw_entries, false);
>  	return rc;

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

[PATCH v1 3/6] KVM: s390: Refactor dat_set_slot()

[Claudio Imbrenda]

Refactor dat_set_slot(), _dat_slot_pte(), _dat_slot_crste(). Now they
only take a struct kvm_s390_mmu_cache as priv. For dat_delete_slot(),
mc is NULL, as no allocations should take place.

This is needed as a prerequisite to move gmap DAT table setup from
kvm_arch_commit_memory_region() to kvm_arch_prepare_memory_region().

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/dat.c      | 29 +++++++++--------------------
 arch/s390/kvm/dat.h      | 10 ++++------
 arch/s390/kvm/kvm-s390.c |  2 +-
 3 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index f2ea013cb33e..7e5dd5a1eb1e 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -844,19 +844,12 @@ long dat_reset_skeys(union asce asce, gfn_t start)
 	return _dat_walk_gfn_range(start, asce_end(asce), asce, &ops, DAT_WALK_IGN_HOLES, NULL);
 }
 
-struct slot_priv {
-	unsigned long token;
-	struct kvm_s390_mmu_cache *mc;
-};
-
 static long _dat_slot_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
 {
-	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);
+	new_pte = walk->priv ? _PTE_EMPTY : _PTE_TOK(_DAT_TOKEN_PIC, PGM_ADDRESSING);
 
 	/* Table entry already in the desired state. */
 	if (pte.val == new_pte.val)
@@ -873,10 +866,9 @@ static long _dat_slot_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_wal
 static long _dat_slot_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
 {
 	union crste new_crste, crste = READ_ONCE(*crstep);
-	struct slot_priv *p = walk->priv;
+	struct kvm_s390_mmu_cache *mc = walk->priv;
 
-	new_crste.val = p->token;
-	new_crste.h.tt = crste.h.tt;
+	new_crste = mc ? _CRSTE_EMPTY(crste.h.tt) : _CRSTE_HOLE(crste.h.tt);
 
 	/* Table entry already in the desired state. */
 	if (crste.val == new_crste.val)
@@ -900,7 +892,10 @@ static long _dat_slot_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct d
 	if (!crste.h.fc && !crste.h.i)
 		return 0;
 	/* Split (install a lower level table), and handle things there. */
-	return dat_split_crste(p->mc, crstep, gfn, walk->asce, false);
+	if (mc)
+		return dat_split_crste(mc, crstep, gfn, walk->asce, false);
+	/* A large page should never cross memslots boundaries */
+	return -EINVAL;
 }
 
 static const struct dat_walk_ops dat_slot_ops = {
@@ -908,16 +903,10 @@ static const struct dat_walk_ops dat_slot_ops = {
 	.crste_ops = { _dat_slot_crste, _dat_slot_crste, _dat_slot_crste, _dat_slot_crste, },
 };
 
-int dat_set_slot(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t start, gfn_t end,
-		 u16 type, u16 param)
+int dat_set_slot(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t start, gfn_t end)
 {
-	struct slot_priv priv = {
-		.token = _CRSTE_TOK(0, type, param).val,
-		.mc = mc,
-	};
-
 	return _dat_walk_gfn_range(start, end, asce, &dat_slot_ops,
-				   DAT_WALK_IGN_HOLES | DAT_WALK_ANY, &priv);
+				   DAT_WALK_IGN_HOLES | DAT_WALK_ANY, mc);
 }
 
 static void pgste_set_unlock_multiple(union pte *first, int n, union pgste *pgstes)
diff --git a/arch/s390/kvm/dat.h b/arch/s390/kvm/dat.h
index 141ee7b9f019..57f32ac9ffed 100644
--- a/arch/s390/kvm/dat.h
+++ b/arch/s390/kvm/dat.h
@@ -543,8 +543,7 @@ long dat_reset_skeys(union asce asce, gfn_t start);
 unsigned long dat_get_ptval(struct page_table *table, struct ptval_param param);
 void dat_set_ptval(struct page_table *table, struct ptval_param param, unsigned long val);
 
-int dat_set_slot(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t start, gfn_t end,
-		 u16 type, u16 param);
+int dat_set_slot(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t start, gfn_t end);
 int dat_set_prefix_notif_bit(union asce asce, gfn_t gfn);
 bool dat_test_age_gfn(union asce asce, gfn_t start, gfn_t end);
 
@@ -958,16 +957,15 @@ static inline int get_level(union crste *crstep, union pte *ptep)
 	return ptep ? TABLE_TYPE_PAGE_TABLE : crstep->h.tt;
 }
 
-static inline int dat_delete_slot(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t start,
-				  unsigned long npages)
+static inline int dat_delete_slot(union asce asce, gfn_t start, unsigned long npages)
 {
-	return dat_set_slot(mc, asce, start, start + npages, _DAT_TOKEN_PIC, PGM_ADDRESSING);
+	return dat_set_slot(NULL, asce, start, start + npages);
 }
 
 static inline int dat_create_slot(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t start,
 				  unsigned long npages)
 {
-	return dat_set_slot(mc, asce, start, start + npages, _DAT_TOKEN_NONE, 0);
+	return dat_set_slot(mc, asce, start, start + npages);
 }
 
 static inline bool crste_is_ucas(union crste crste)
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 91b975835457..0a08a3c7b023 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5860,7 +5860,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 
 		switch (change) {
 		case KVM_MR_DELETE:
-			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
+			rc = dat_delete_slot(kvm->arch.gmap->asce, old->base_gfn, old->npages);
 			break;
 		case KVM_MR_MOVE:
 			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
-- 
2.55.0

Re: [PATCH v1 3/6] KVM: s390: Refactor dat_set_slot()

[sashiko-bot]

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

New issues:
- [Low] Build failure: dat_delete_slot() is called with too many arguments in KVM_MR_MOVE.

Pre-existing issues:
- [High] Spurious failure of memslot deletion and VM crash due to concurrent hardware RC bit updates.
--

--- Patch [3]: [PATCH v1 3/6] KVM: s390: Refactor dat_set_slot() ---
commit 15c005eb250e6c4e079b0b85ff6c42d583b09782
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Refactor dat_set_slot()

This commit refactors dat_set_slot() and related functions to only take a
struct kvm_s390_mmu_cache. For dat_delete_slot(), mc is now NULL. This
serves as a prerequisite for moving gmap DAT table setup.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index f2ea013cb33e2..7e5dd5a1eb1ef 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c

[ ... ]

> @@ -873,10 +866,9 @@ static long _dat_slot_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_wal
>  static long _dat_slot_crste(union crste *crstep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
>  {
>  	union crste new_crste, crste = READ_ONCE(*crstep);
> -	struct slot_priv *p = walk->priv;
> +	struct kvm_s390_mmu_cache *mc = walk->priv;

[Severity: High]
This is a pre-existing issue, but does the atomic exchange in this function
need a retry loop?

If the hardware page table walker asynchronously updates the Reference or
Change (RC) bits on a valid large page CRSTE while KVM is deleting the slot,
it seems dat_crstep_xchg_atomic() can fail because the memory value no longer
matches crste:

arch/s390/kvm/dat.c:_dat_slot_crste() {
	...
	/* This table entry needs to be updated. */
	if (walk->start <= gfn && walk->end >= next) {
		if (!dat_crstep_xchg_atomic(crstep, crste, new_crste, gfn, walk->asce))
			return -EINVAL;
	...
}

Would this spurious -EINVAL propagate up to kvm_arch_prepare_memory_region()
and trigger KVM_BUG_ON(rc, kvm), causing the VM to panic?

> -	new_crste.val = p->token;
> -	new_crste.h.tt = crste.h.tt;
> +	new_crste = mc ? _CRSTE_EMPTY(crste.h.tt) : _CRSTE_HOLE(crste.h.tt);
>  
>  	/* Table entry already in the desired state. */
>  	if (crste.val == new_crste.val)

[ ... ]

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 91b9758354574..0a08a3c7b023d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5860,7 +5860,7 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>  
>  		switch (change) {
>  		case KVM_MR_DELETE:
> -			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
> +			rc = dat_delete_slot(kvm->arch.gmap->asce, old->base_gfn, old->npages);
>  			break;
>  		case KVM_MR_MOVE:
>  			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);

[Severity: Low]
Does this code fail to compile?

Since dat_delete_slot() was updated to only take 3 arguments, it looks like
the KVM_MR_MOVE case is still passing 4 arguments.

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

[PATCH v1 4/6] KVM: s390: Move all code into kvm_arch_prepare_memory_region()

[Claudio Imbrenda]

Move all code from kvm_arch_commit_memory_region() into
kvm_arch_prepare_memory_region(). This allows the function to fail
gracefully if needed. The previous behaviour was to print a warning and
continue execution with page tables incosistent with the memslots.

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

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0a08a3c7b023..50eb72447fd5 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5772,11 +5772,28 @@ bool kvm_arch_irqchip_in_kernel(struct kvm *kvm)
 }
 
 /* Section: memory related */
+static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
+{
+	union pgste pgste;
+
+	pgste = pgste_get_lock(ptep);
+	if (pgste.cmma_d) {
+		pgste.cmma_d = 0;
+		atomic64_dec(walk->priv);
+	}
+	pgste_set_unlock(ptep, pgste);
+	return 0;
+}
+
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				   const struct kvm_memory_slot *old,
 				   struct kvm_memory_slot *new,
 				   enum kvm_mr_change change)
 {
+	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;
+
 	if (kvm_is_ucontrol(kvm) && new && new->id < KVM_USER_MEM_SLOTS)
 		return -EINVAL;
 
@@ -5791,6 +5808,10 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 		 * and munmap() stuff in this slot after doing this call at any
 		 * time.
 		 */
+		if (change != KVM_MR_MOVE && change != KVM_MR_CREATE) {
+			WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
+			return -EINVAL;
+		}
 		if (new->userspace_addr & ~PAGE_MASK)
 			return -EINVAL;
 		if ((new->base_gfn + new->npages) * PAGE_SIZE > kvm->arch.mem_limit)
@@ -5799,56 +5820,28 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 			return -EINVAL;
 	}
 
-	if (!kvm->arch.migration_mode)
-		return 0;
-
-	/*
-	 * Turn off migration mode when:
-	 * - userspace creates a new memslot with dirty logging off,
-	 * - userspace modifies an existing memslot (MOVE or FLAGS_ONLY) and
-	 *   dirty logging is turned off.
-	 * Migration mode expects dirty page logging being enabled to store
-	 * its dirty bitmap.
-	 */
-	if (change != KVM_MR_DELETE &&
-	    !(new->flags & KVM_MEM_LOG_DIRTY_PAGES))
-		WARN(kvm_s390_vm_stop_migration(kvm),
-		     "Failed to stop migration mode");
-
-	return 0;
-}
-
-static long cmma_d_count_pte(union pte *ptep, gfn_t gfn, gfn_t next, struct dat_walk *walk)
-{
-	union pgste pgste;
-
-	pgste = pgste_get_lock(ptep);
-	if (pgste.cmma_d) {
-		pgste.cmma_d = 0;
-		atomic64_dec(walk->priv);
+	if (kvm->arch.migration_mode) {
+		/*
+		 * Turn off migration mode when:
+		 * - userspace creates a new memslot with dirty logging off,
+		 * - userspace modifies an existing memslot (MOVE or FLAGS_ONLY)
+		 *   and dirty logging is turned off.
+		 * Migration mode expects dirty page logging being enabled to
+		 * store its dirty bitmap.
+		 */
+		if (change != KVM_MR_DELETE &&
+		    !(new->flags & KVM_MEM_LOG_DIRTY_PAGES))
+			WARN(kvm_s390_vm_stop_migration(kvm),
+			     "Failed to stop migration mode");
 	}
-	pgste_set_unlock(ptep, pgste);
-	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)
-{
-	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;
-
-	mc = kvm_s390_new_mmu_cache();
-	if (!mc) {
-		rc = -ENOMEM;
-		goto out;
+		return 0;
+	if (change != KVM_MR_DELETE) {
+		/* Enough capacity to add a new memslot */
+		mc = kvm_s390_new_mmu_cache();
+		if (!mc)
+			return -ENOMEM;
 	}
 
 	scoped_guard(write_lock, &kvm->mmu_lock) {
@@ -5858,28 +5851,23 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 					    &kvm->arch.cmma_dirty_pages);
 		}
 
-		switch (change) {
-		case KVM_MR_DELETE:
+		if (change == KVM_MR_DELETE || change == KVM_MR_MOVE)
 			rc = dat_delete_slot(kvm->arch.gmap->asce, old->base_gfn, old->npages);
-			break;
-		case KVM_MR_MOVE:
-			rc = dat_delete_slot(mc, kvm->arch.gmap->asce, old->base_gfn, old->npages);
-			if (rc)
-				break;
-			fallthrough;
-		case KVM_MR_CREATE:
+		if (!rc && (change == KVM_MR_MOVE || change == KVM_MR_CREATE))
 			rc = dat_create_slot(mc, kvm->arch.gmap->asce, new->base_gfn, new->npages);
-			break;
-		case KVM_MR_FLAGS_ONLY:
-			break;
-		default:
-			WARN(1, "Unknown KVM MR CHANGE: %d\n", change);
-		}
 	}
-out:
-	if (rc)
-		pr_warn("failed to commit memory region\n");
-	return;
+	/*
+	 * Can only be triggered if dat_{create,delete}_slot() found an
+	 * internal inconsistency or if the mmu cache ran out of memory;
+	 * both should be impossible.
+	 */
+	KVM_BUG_ON(rc, kvm);
+	return rc;
+}
+
+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)
+{
 }
 
 /**
-- 
2.55.0

Re: [PATCH v1 4/6] KVM: s390: Move all code into kvm_arch_prepare_memory_region()

[sashiko-bot]

> Move all code from kvm_arch_commit_memory_region() into
> kvm_arch_prepare_memory_region(). This allows the function to fail
> gracefully if needed. The previous behaviour was to print a warning and
> continue execution with page tables incosistent with the memslots.
> 
> 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/20260814144041.169025-1-imbrenda@linux.ibm.com?part=4

[PATCH v1 5/6] KVM: s390: Add missing srcu in kvm_arch_vcpu_ioctl()

[Claudio Imbrenda]

Like kvm_s390_inject_vcpu(), kvm_s390_set_irq_state() also needs to be
called holding kvm->srcu or the slots lock. When fixing
kvm_s390_inject_vcpu() in a previous patch, the path through
kvm_s390_set_irq_state() was missed.

Fix by taking kvm->srcu in kvm_arch_vcpu_ioctl().

Fixes: ba5c1e9b6cee ("KVM: s390: interrupt subsystem, cpu timer, waitpsw")
Fixes: 062e44a9319f ("KVM: s390: Use srcu in kvm_arch_vcpu_unlocked_ioctl()")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 50eb72447fd5..7fb5a566e2d8 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5697,9 +5697,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 			break;
 		}
 		/* do not use irq_state.flags, it will break old QEMUs */
-		r = kvm_s390_set_irq_state(vcpu,
-					   (void __user *) irq_state.buf,
-					   irq_state.len);
+		scoped_guard(srcu, &vcpu->kvm->srcu) {
+			r = kvm_s390_set_irq_state(vcpu, (void __user *)irq_state.buf,
+						   irq_state.len);
+		}
 		break;
 	}
 	case KVM_S390_GET_IRQ_STATE: {
-- 
2.55.0

Re: [PATCH v1 5/6] KVM: s390: Add missing srcu in kvm_arch_vcpu_ioctl()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Circular deadlock between KVM SRCU lock and userfaultfd during copy_from_user().
--

--- Patch [5]: [PATCH v1 5/6] KVM: s390: Add missing srcu in kvm_arch_vcpu_ioctl() ---
commit 4558246eff181af6ab8735d60c453a1298252bfe
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Add missing srcu in kvm_arch_vcpu_ioctl()

This commit adds an SRCU read lock around kvm_s390_set_irq_state()
in kvm_arch_vcpu_ioctl().

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 50eb72447fd53..7fb5a566e2d85 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5697,9 +5697,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  			break;
>  		}
>  		/* do not use irq_state.flags, it will break old QEMUs */
> -		r = kvm_s390_set_irq_state(vcpu,
> -					   (void __user *) irq_state.buf,
> -					   irq_state.len);
> +		scoped_guard(srcu, &vcpu->kvm->srcu) {
> +			r = kvm_s390_set_irq_state(vcpu, (void __user *)irq_state.buf,
> +						   irq_state.len);
> +		}

[Severity: High]
Does this scoped_guard result in calling copy_from_user() while holding
the SRCU read lock?

Looking at kvm_s390_set_irq_state() in arch/s390/kvm/interrupt.c, it
performs a user copy:

int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len)
{
	...
	buf = vmalloc(len);
	if (!buf)
		return -ENOMEM;

	if (copy_from_user((void *)buf, irqstate, len))
		return -EFAULT;
	...
}

If userspace passes a userfaultfd-managed region as the irq state buffer,
copy_from_user() can block and wait for the userfaultfd manager.

If that manager then attempts an operation that updates KVM memslots
(such as KVM_SET_USER_MEMORY_REGION), it will acquire the slots lock
and wait on synchronize_srcu().

Could this cause a circular deadlock between the SRCU read lock held
here and the slots lock acquired by the userfaultfd manager?

>  		break;
>  	}
>  	case KVM_S390_GET_IRQ_STATE: {

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

[PATCH v1 6/6] KVM: s390: Fix potential race in dat_cond_set_storage_key()

[Claudio Imbrenda]

When dat_cond_set_storage_key() finds a large page, it will
conditionally set the storage key in absolute memory using
large_crste_to_phys() to get the absolute address.

There is a race window between dat_entry_walk() and
large_crste_to_phys(): the large page could have been split
concurrently, and large_crste_to_phys() might be called with a crste
that does not designate a large page, leading to crashes.

Fix by performing a READ_ONCE on the crste pointer, checking and using
the result, instead of dereferencing the pointer again. In case a race
is detacted, try dat_entry_walk() again.

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

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 7e5dd5a1eb1e..b467b95689b2 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -713,10 +713,12 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
 {
 	union pgste pgste, old;
 	union crste *crstep;
+	union crste crste;
 	union skey prev;
 	union pte *ptep;
 	int rc;
 
+again:
 	rc = dat_entry_walk(mmc, gfn, asce, DAT_WALK_LEAF_ALLOC, TABLE_TYPE_PAGE_TABLE,
 			    &crstep, &ptep);
 	if (rc)
@@ -725,7 +727,11 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
 	if (!ptep) {
 		if (!oldkey)
 			oldkey = &prev;
-		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
+		crste = READ_ONCE(*crstep);
+		/* A large page has been split concurrently, try again */
+		if (!crste_leaf(crste))
+			goto again;
+		return page_cond_set_storage_key(large_crste_to_phys(crste, gfn), skey, oldkey,
 						 nq, mr, mc);
 	}
 
-- 
2.55.0

Re: [PATCH v1 6/6] KVM: s390: Fix potential race in dat_cond_set_storage_key()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Identical TOCTOU vulnerabilities remain in `dat_set_storage_key()`, `dat_reset_reference_bit()`, and `dat_get_storage_key()`.
--

--- Patch [6]: [PATCH v1 6/6] KVM: s390: Fix potential race in dat_cond_set_storage_key() ---
commit 5e22031091ce720a5918edc8a96db1b780f83398
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Fix potential race in dat_cond_set_storage_key()

This commit addresses a race window in dat_cond_set_storage_key() where a
large page could be concurrently split between the page table walk and the
dereferencing of the crste pointer. By reading the entry exactly once and
checking if it is still a leaf, it prevents passing an invalid physical
address to the storage key functions.

> diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
> index 7e5dd5a1eb1ef..b467b95689b21 100644
> --- a/arch/s390/kvm/dat.c
> +++ b/arch/s390/kvm/dat.c
[ ... ]
> @@ -725,7 +727,11 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
>  	if (!ptep) {
>  		if (!oldkey)
>  			oldkey = &prev;
> -		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
> +		crste = READ_ONCE(*crstep);
> +		/* A large page has been split concurrently, try again */
> +		if (!crste_leaf(crste))
> +			goto again;
> +		return page_cond_set_storage_key(large_crste_to_phys(crste, gfn), skey, oldkey,
>  						 nq, mr, mc);
>  	}

[Severity: Critical]
This is a pre-existing issue, but do other functions in this file suffer from
the exact same TOCTOU race that this patch fixes?

Looking at dat_set_storage_key() in the same file, the pointer is dereferenced
directly without a READ_ONCE() and leaf check:

arch/s390/kvm/dat.c:dat_set_storage_key() {
    ...
    page_set_storage_key(large_crste_to_phys(*crstep, gfn), skey.skey, !nq);
    ...
}

If a concurrent split occurs here, large_crste_to_phys() will return -1, which
can result in an invalid physical address access.

Similarly, in dat_reset_reference_bit(), *crstep is dereferenced a second time:

arch/s390/kvm/dat.c:dat_reset_reference_bit() {
    ...
    skey->skey = page_reset_referenced(large_crste_to_phys(*crstep, gfn)) << 1;
    ...
}

Does this TOCTOU window also pass -1 to page_reset_referenced() if a concurrent
split happens between the read and the usage?

Finally, in dat_get_storage_key():

arch/s390/kvm/dat.c:dat_get_storage_key() {
    ...
    crste = READ_ONCE(*crstep);
    if (!crste.h.fc || !crste.s.fc1.pr)
        return 0;
    ...
}

Instead of retrying the walk via a goto loop like the fix in
dat_cond_set_storage_key(), it returns 0. Could this silently give a false
storage key to the guest if a race occurred?

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