[PATCH v1 0/3] KVM: s390: A few misc gmap fixes.

[PATCH v1 0/3] KVM: s390: A few misc gmap fixes.

[Claudio Imbrenda]

A few more minor gmap fixes.

Claudio Imbrenda (3):
  KVM: s390: Silence potential warnings in _gmap_crstep_xchg_atomic()
  KVM: s390: Fix unlikely race in try_get_locked_pte()
  KVM: s390: vsie: Use mmu cache to allocate rmap

 arch/s390/kvm/gaccess.c     | 16 ++++++++--------
 arch/s390/kvm/gmap.c        |  7 ++++---
 arch/s390/kvm/gmap.h        |  9 +++++++--
 arch/s390/mm/gmap_helpers.c |  6 +++---
 4 files changed, 22 insertions(+), 16 deletions(-)

-- 
2.54.0

[PATCH v1 1/3] KVM: s390: Silence potential warnings in _gmap_crstep_xchg_atomic()

[Claudio Imbrenda]

While dat_crstep_xchg_atomic() is marked as __must_check, in this
particular case the return value should be ignored.

Add an explicit (void) cast to silence potential compiler warnings, and
a comment to explain why it is ok to do so.

Fixes: d1adc098ce08 ("KVM: s390: Fix _gmap_crstep_xchg_atomic()")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
CC: stable@vger.kernel.org # 7.1
---
 arch/s390/kvm/gmap.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/gmap.h b/arch/s390/kvm/gmap.h
index 5374f21aaf8d..4e6979783e16 100644
--- a/arch/s390/kvm/gmap.h
+++ b/arch/s390/kvm/gmap.h
@@ -279,7 +279,11 @@ static inline bool __must_check _gmap_crstep_xchg_atomic(struct gmap *gmap, unio
 			gmap_handle_vsie_unshadow_event(gmap, gfn);
 		else
 			_gmap_handle_vsie_unshadow_event(gmap, gfn);
-		dat_crstep_xchg_atomic(crstep, oldcrste, newcrste, gfn, gmap->asce);
+		/*
+		 * Ignore the result, it's just a best effort clearing of the
+		 * vsie_notif bit. The caller will have to try again regardless.
+		 */
+		(void)dat_crstep_xchg_atomic(crstep, oldcrste, newcrste, gfn, gmap->asce);
 		return false;
 	}
 	if (!oldcrste.s.fc1.d && newcrste.s.fc1.d && !newcrste.s.fc1.s)
-- 
2.54.0

Re: [PATCH v1 1/3] KVM: s390: Silence potential warnings in _gmap_crstep_xchg_atomic()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `(void)` cast does not silence `__must_check` (`warn_unused_result`) warnings in GCC.
--

commit ba4c03a707e418c0a14a0a8490bf5c5ce461e30c
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: Silence potential warnings in _gmap_crstep_xchg_atomic()

This commit attempts to silence a compiler warning by casting the result of
dat_crstep_xchg_atomic() to (void). The function is annotated with
__must_check, and the caller intentionally ignores the result as it is a
best-effort clearing operation.

Fixes: d1adc098ce08 ("KVM: s390: Fix _gmap_crstep_xchg_atomic()")
CC: stable@vger.kernel.org # 7.1

> diff --git a/arch/s390/kvm/gmap.h b/arch/s390/kvm/gmap.h
> index 5374f21aaf8df..4e6979783e16b 100644
> --- a/arch/s390/kvm/gmap.h
> +++ b/arch/s390/kvm/gmap.h
> @@ -279,7 +279,11 @@ static inline bool __must_check _gmap_crstep_xchg_atomic(struct gmap *gmap, unio
>  			gmap_handle_vsie_unshadow_event(gmap, gfn);
>  		else
>  			_gmap_handle_vsie_unshadow_event(gmap, gfn);
> -		dat_crstep_xchg_atomic(crstep, oldcrste, newcrste, gfn, gmap->asce);
> +		/*
> +		 * Ignore the result, it's just a best effort clearing of the
> +		 * vsie_notif bit. The caller will have to try again regardless.
> +		 */
> +		(void)dat_crstep_xchg_atomic(crstep, oldcrste, newcrste, gfn, gmap->asce);

[Severity: Low]
Does this actually silence the warning on GCC?

The Linux kernel defines __must_check as __attribute__((__warn_unused_result__)).
GCC ignores (void) casts when evaluating this attribute and will continue to emit
the warning, unlike Clang which allows the suppression. 

Since GCC is the primary compiler for KVM on s390x, will this patch accomplish
its stated goal, or should we actively evaluate the return value instead (for
example, using an empty if block)?

>  		return false;
>  	}

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

[PATCH v1 2/3] KVM: s390: Fix unlikely race in try_get_locked_pte()

[Claudio Imbrenda]

Fix an unlikely race in try_get_locked_pte(), which could have happened
if puds or pmds get unmapped between the p?dp_get() and p?d_offset()
functions.

Fixes: 89fa757931dc ("KVM: s390: Avoid potentially sleeping while atomic when zapping pages")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
CC: stable@vger.kernel.org # 7.1
---
 arch/s390/mm/gmap_helpers.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/s390/mm/gmap_helpers.c b/arch/s390/mm/gmap_helpers.c
index 1cfe4724fbe2..ee3f37af8aee 100644
--- a/arch/s390/mm/gmap_helpers.c
+++ b/arch/s390/mm/gmap_helpers.c
@@ -51,15 +51,15 @@ pte_t *try_get_locked_pte(struct mm_struct *mm, unsigned long vmaddr, spinlock_t
 	pgd = pgdp_get(pgdp);
 	if (pgd_none(pgd) || !pgd_present(pgd))
 		return NULL;
-	p4dp = p4d_offset(pgdp, vmaddr);
+	p4dp = p4d_offset_lockless(pgdp, pgd, vmaddr);
 	p4d = p4dp_get(p4dp);
 	if (p4d_none(p4d) || !p4d_present(p4d))
 		return NULL;
-	pudp = pud_offset(p4dp, vmaddr);
+	pudp = pud_offset_lockless(p4dp, p4d, vmaddr);
 	pud = pudp_get(pudp);
 	if (pud_none(pud) || pud_leaf(pud) || !pud_present(pud))
 		return NULL;
-	pmdp = pmd_offset(pudp, vmaddr);
+	pmdp = pmd_offset_lockless(pudp, pud, vmaddr);
 	pmd = pmdp_get_lockless(pmdp);
 	if (pmd_none(pmd) || pmd_leaf(pmd) || !pmd_present(pmd))
 		return NULL;
-- 
2.54.0

[PATCH v1 3/3] KVM: s390: vsie: Use mmu cache to allocate rmap

[Claudio Imbrenda]

Use kvm_s390_mmu_cache_alloc_rmap() to allocate the rmap in
gmap_insert_rmap(), instead of a normal kzalloc_obj() with GFP_ATOMIC.

This guarantees forward progress.

Fixes: a2c17f9270cc ("KVM: s390: New gmap code")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
CC: stable@vger.kernel.org # 7.1
---
 arch/s390/kvm/gaccess.c | 16 ++++++++--------
 arch/s390/kvm/gmap.c    |  7 ++++---
 arch/s390/kvm/gmap.h    |  3 ++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 20e28b183c1a..022ea7736521 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -1419,8 +1419,8 @@ static int walk_guest_tables(struct gmap *sg, unsigned long saddr, struct pgtwal
 	return kvm_s390_get_guest_page(kvm, entries + LEVEL_MEM, table.pte.pfra, wr);
 }
 
-static int _do_shadow_pte(struct gmap *sg, gpa_t raddr, union pte *ptep_h, union pte *ptep,
-			  struct guest_fault *f, bool p)
+static int _do_shadow_pte(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gpa_t raddr,
+			  union pte *ptep_h, union pte *ptep, struct guest_fault *f, bool p)
 {
 	union pgste pgste;
 	union pte newpte;
@@ -1430,7 +1430,7 @@ static int _do_shadow_pte(struct gmap *sg, gpa_t raddr, union pte *ptep_h, union
 	lockdep_assert_held(&sg->parent->children_lock);
 
 	scoped_guard(spinlock, &sg->host_to_rmap_lock)
-		rc = gmap_insert_rmap(sg, f->gfn, gpa_to_gfn(raddr), TABLE_TYPE_PAGE_TABLE);
+		rc = gmap_insert_rmap(mc, sg, f->gfn, gpa_to_gfn(raddr), TABLE_TYPE_PAGE_TABLE);
 	if (rc)
 		return rc;
 
@@ -1462,8 +1462,8 @@ static int _do_shadow_pte(struct gmap *sg, gpa_t raddr, union pte *ptep_h, union
 	return 0;
 }
 
-static int _do_shadow_crste(struct gmap *sg, gpa_t raddr, union crste *host, union crste *table,
-			    struct guest_fault *f, bool p)
+static int _do_shadow_crste(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gpa_t raddr,
+			    union crste *host, union crste *table, struct guest_fault *f, bool p)
 {
 	union crste newcrste, oldcrste;
 	unsigned long mask;
@@ -1476,7 +1476,7 @@ static int _do_shadow_crste(struct gmap *sg, gpa_t raddr, union crste *host, uni
 	mask = is_pmd(*table) ? _SEGMENT_FR_MASK : _REGION3_FR_MASK;
 	r_gfn = gpa_to_gfn(raddr) & mask;
 	scoped_guard(spinlock, &sg->host_to_rmap_lock)
-		rc = gmap_insert_rmap(sg, f->gfn & mask, r_gfn, host->h.tt);
+		rc = gmap_insert_rmap(mc, sg, f->gfn & mask, r_gfn, host->h.tt);
 	if (rc)
 		return rc;
 
@@ -1578,8 +1578,8 @@ static int _gaccess_do_shadow(struct kvm_s390_mmu_cache *mc, struct gmap *sg,
 	if (KVM_BUG_ON(l > TABLE_TYPE_REGION3, sg->kvm))
 		return -EFAULT;
 	if (l == TABLE_TYPE_PAGE_TABLE)
-		return _do_shadow_pte(sg, saddr, ptep_h, ptep, entries + LEVEL_MEM, w->p);
-	return _do_shadow_crste(sg, saddr, host, table, entries + LEVEL_MEM, w->p);
+		return _do_shadow_pte(mc, sg, saddr, ptep_h, ptep, entries + LEVEL_MEM, w->p);
+	return _do_shadow_crste(mc, sg, saddr, host, table, entries + LEVEL_MEM, w->p);
 }
 
 static inline int _gaccess_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg, gpa_t saddr,
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 52d55ddea8d4..1d289f8fa3b2 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -1000,7 +1000,8 @@ int gmap_pv_destroy_range(struct gmap *gmap, gfn_t start, gfn_t end, bool interr
 	return 0;
 }
 
-int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level)
+int gmap_insert_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn,
+		     gfn_t r_gfn, int level)
 {
 	struct vsie_rmap *rmap __free(kvfree) = NULL;
 	struct vsie_rmap *temp;
@@ -1010,7 +1011,7 @@ int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level)
 	KVM_BUG_ON(!is_shadow(sg), sg->kvm);
 	lockdep_assert_held(&sg->host_to_rmap_lock);
 
-	rmap = kzalloc_obj(*rmap, GFP_ATOMIC);
+	rmap = kvm_s390_mmu_cache_alloc_rmap(mc);
 	if (!rmap)
 		return -ENOMEM;
 
@@ -1057,7 +1058,7 @@ int gmap_protect_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gf
 	if (level <= TABLE_TYPE_REGION1) {
 		bitmask = -1UL << (8 + 11 * level);
 		scoped_guard(spinlock, &sg->host_to_rmap_lock)
-			rc = gmap_insert_rmap(sg, p_gfn, r_gfn & bitmask, level);
+			rc = gmap_insert_rmap(mc, sg, p_gfn, r_gfn & bitmask, level);
 	}
 	if (rc)
 		return rc;
diff --git a/arch/s390/kvm/gmap.h b/arch/s390/kvm/gmap.h
index 4e6979783e16..75df5d39bd78 100644
--- a/arch/s390/kvm/gmap.h
+++ b/arch/s390/kvm/gmap.h
@@ -100,7 +100,8 @@ int gmap_ucas_map(struct gmap *gmap, gfn_t p_gfn, gfn_t c_gfn, unsigned long cou
 void gmap_ucas_unmap(struct gmap *gmap, gfn_t c_gfn, unsigned long count);
 int gmap_enable_skeys(struct gmap *gmap);
 int gmap_pv_destroy_range(struct gmap *gmap, gfn_t start, gfn_t end, bool interruptible);
-int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level);
+int gmap_insert_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn,
+		     gfn_t r_gfn, int level);
 int gmap_protect_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn,
 		      kvm_pfn_t pfn, int level, bool wr);
 void gmap_set_cmma_all_dirty(struct gmap *gmap);
-- 
2.54.0

Re: [PATCH v1 3/3] KVM: s390: vsie: Use mmu cache to allocate rmap

[sashiko-bot]

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

New issues:
- [High] Slab out-of-bounds write due to undersized MMU cache allocations

Pre-existing issues:
- [High] Incomplete forward progress guarantee leaves preexisting livelock unresolved
--

commit d137e1b91e093869ffa253ca4d6339ea68a1d49a
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: vsie: Use mmu cache to allocate rmap

Use kvm_s390_mmu_cache_alloc_rmap() to allocate the rmap in
gmap_insert_rmap(), instead of a normal kzalloc_obj() with GFP_ATOMIC.

> This guarantees forward progress.

[Severity: High]
This is a pre-existing issue, but does this actually guarantee
forward progress?

Immediately after allocating the rmap, gmap_insert_rmap() calls
radix_tree_insert(&sg->host_to_rmap, p_gfn, rmap).

Because the radix tree is initialized with GFP_ATOMIC, node allocations
can still fail under memory pressure and return -ENOMEM. The fault loop in
_gaccess_shadow_fault() catches this, drops the lock, calls
kvm_s390_mmu_cache_topup(), and retries:

again:
	rc = kvm_s390_mmu_cache_topup(vcpu->arch.mc);
	...
		rc = _gaccess_do_shadow(vcpu->arch.mc, sg, saddr, walk);
	...
	if (rc == -ENOMEM)
		goto again;

Since kvm_s390_mmu_cache_topup() does not call radix_tree_preload(), the
retry attempts another atomic allocation, potentially leading to an infinite
livelock when atomic memory is exhausted.

> Fixes: a2c17f9270cc ("KVM: s390: New gmap code")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> CC: stable@vger.kernel.org # 7.1

[ ... ]

> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index 52d55ddea8d4f..1d289f8fa3b20 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -1000,7 +1000,8 @@ int gmap_pv_destroy_range(struct gmap *gmap, gfn_t start, gfn_t end, bool interr
>  	return 0;
>  }
>  
> -int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level)
> +int gmap_insert_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn,
> +		     gfn_t r_gfn, int level)
>  {
>  	struct vsie_rmap *rmap __free(kvfree) = NULL;
>  	struct vsie_rmap *temp;
> @@ -1010,7 +1011,7 @@ int gmap_insert_rmap(struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn, int level)
>  	KVM_BUG_ON(!is_shadow(sg), sg->kvm);
>  	lockdep_assert_held(&sg->host_to_rmap_lock);
>  
> -	rmap = kzalloc_obj(*rmap, GFP_ATOMIC);
> +	rmap = kvm_s390_mmu_cache_alloc_rmap(mc);

[Severity: High]
Does this result in a 1-byte allocation for the rmap?

Since mc->rmaps is an array of void pointers, when kvm_s390_mmu_cache_topup()
populates the cache in arch/s390/kvm/dat.c:

	o = kzalloc_obj(*mc->rmaps[0], GFP_KERNEL_ACCOUNT);

it evaluates sizeof(void), which is 1 byte. When gmap_insert_rmap()
fetches this 1-byte object from the cache and writes to rmap->r_gfn
(at offset 8):

	rmap->r_gfn = r_gfn;

can this cause a slab out-of-bounds write and memory corruption?

>  	if (!rmap)
>  		return -ENOMEM;

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