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

[PATCH v2 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.

v1->v2:
* Fix racy usage of sg->parent in patch 2
* Fix wrong number of arguments for dat_delete_slot() in patch 3
* Restrict srcu usage to the bare minimum necessary to avoid potentially
  triggering page faults in the guest while holding the srcu
* Fix races also in other skey functions


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_s390_set_irq_state()
  KVM: s390: Fix potential races in dat skey functions

 arch/s390/kvm/dat.c       |  65 +++++++++++---------
 arch/s390/kvm/dat.h       |  10 ++--
 arch/s390/kvm/gaccess.c   |  13 ++++
 arch/s390/kvm/interrupt.c |  44 +++++++-------
 arch/s390/kvm/kvm-s390.c  | 122 +++++++++++++++++---------------------
 5 files changed, 132 insertions(+), 122 deletions(-)

-- 
2.55.0

[PATCH v2 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

[PATCH v2 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..7c1f614ec314 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, 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

[PATCH v2 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 |  4 ++--
 3 files changed, 15 insertions(+), 28 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..212ff0f8eecc 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5860,10 +5860,10 @@ 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);
+			rc = dat_delete_slot(kvm->arch.gmap->asce, old->base_gfn, old->npages);
 			if (rc)
 				break;
 			fallthrough;
-- 
2.55.0

[PATCH v2 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 212ff0f8eecc..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:
-			rc = dat_delete_slot(kvm->arch.gmap->asce, old->base_gfn, old->npages);
-			break;
-		case KVM_MR_MOVE:
+		if (change == KVM_MR_DELETE || change == KVM_MR_MOVE)
 			rc = dat_delete_slot(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

[PATCH v2 5/6] KVM: s390: Add missing srcu in kvm_s390_set_irq_state()

[Claudio Imbrenda]

Like kvm_s390_inject_vcpu(), kvm_s390_set_irq_state() also needs the
kvm->srcu or the slots lock when performing the Store status operation.

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

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/interrupt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index fc4d1f8193d9..8a251f83d323 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -3230,9 +3230,9 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
 				break;
 		}
 	}
-
 	if (storestatus) {
-		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);
+		scoped_guard(srcu, &vcpu->kvm->srcu)
+			n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);
 		return r ? r : n;
 	}
 
-- 
2.55.0

[PATCH v2 6/6] KVM: s390: Fix potential races in dat skey functions

[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.

Similar issues were also present in dat_set_storage_key().

dat_get_storage_key() and dat_reset_reference_bit() did instead check
for a potential concurrent splitting of the large page, but then
handled it incorrectly.

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 | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kvm/dat.c b/arch/s390/kvm/dat.c
index 7e5dd5a1eb1e..b4c318ebd91e 100644
--- a/arch/s390/kvm/dat.c
+++ b/arch/s390/kvm/dat.c
@@ -620,17 +620,20 @@ int dat_get_storage_key(union asce asce, gfn_t gfn, union skey *skey)
 	union pte *ptep;
 	int rc;
 
+again:
 	skey->skey = 0;
 	rc = dat_entry_walk(NULL, gfn, asce, DAT_WALK_ANY, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
 	if (rc)
 		return rc;
 
 	if (!ptep) {
-		union crste crste;
+		union crste crste = READ_ONCE(*crstep);
 
-		crste = READ_ONCE(*crstep);
-		if (!crste.h.fc || !crste.s.fc1.pr)
+		if (!crste_leaf(crste))
+			goto again;
+		if (!crste.s.fc1.pr)
 			return 0;
+
 		skey->skey = page_get_storage_key(large_crste_to_phys(crste, gfn));
 		return 0;
 	}
@@ -661,13 +664,20 @@ int dat_set_storage_key(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t gf
 	union pte *ptep;
 	int rc;
 
+again:
 	rc = dat_entry_walk(mc, gfn, asce, DAT_WALK_LEAF_ALLOC, TABLE_TYPE_PAGE_TABLE,
 			    &crstep, &ptep);
 	if (rc)
 		return rc;
 
 	if (!ptep) {
-		page_set_storage_key(large_crste_to_phys(*crstep, gfn), skey.skey, !nq);
+		union crste crste = READ_ONCE(*crstep);
+
+		/* A large page has been split concurrently, try again */
+		if (!crste_leaf(crste))
+			goto again;
+
+		page_set_storage_key(large_crste_to_phys(crste, gfn), skey.skey, !nq);
 		return 0;
 	}
 
@@ -717,15 +727,22 @@ int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gf
 	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)
 		return rc;
 
 	if (!ptep) {
+		union crste crste = READ_ONCE(*crstep);
+
+		/* A large page has been split concurrently, try again */
+		if (!crste_leaf(crste))
+			goto again;
 		if (!oldkey)
 			oldkey = &prev;
-		return page_cond_set_storage_key(large_crste_to_phys(*crstep, gfn), skey, oldkey,
+
+		return page_cond_set_storage_key(large_crste_to_phys(crste, gfn), skey, oldkey,
 						 nq, mr, mc);
 	}
 
@@ -767,7 +784,7 @@ int dat_reset_reference_bit(union asce asce, gfn_t gfn, union skey *skey)
 	int rc;
 
 	skey->skey = 0;
-
+again:
 	rc = dat_entry_walk(NULL, gfn, asce, DAT_WALK_ANY, TABLE_TYPE_PAGE_TABLE, &crstep, &ptep);
 	if (rc)
 		return rc;
@@ -775,9 +792,12 @@ int dat_reset_reference_bit(union asce asce, gfn_t gfn, union skey *skey)
 	if (!ptep) {
 		union crste crste = READ_ONCE(*crstep);
 
-		if (!crste.h.fc || !crste.s.fc1.pr)
+		/* A large page has been split concurrently, try again */
+		if (!crste_leaf(crste))
+			goto again;
+		if (!crste.s.fc1.pr)
 			return 0;
-		skey->skey = page_reset_referenced(large_crste_to_phys(*crstep, gfn)) << 1;
+		skey->skey = page_reset_referenced(large_crste_to_phys(crste, gfn)) << 1;
 		return 0;
 	}
 	old = pgste_get_lock(ptep);
-- 
2.55.0

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

[Christian Borntraeger]

Am 14.08.26 um 18:33 schrieb 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>

Doug, Matt, can you have a look?



> ---
>   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;
>   }
>   

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

[Matthew Rosato]

On 8/14/26 12:33 PM, Claudio Imbrenda wrote:
> 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")

So I'm not strictly opposed to adding the dirtying on the 'fast path'
but AFAIU the idea here was to still mark the page as dirty right before
unpin.

Is it strictly required to dirty the page every time it's touched even
if it's long-term pinned, so long as we make sure to mark the page dirty
before we eventually unpin it?

I think the answer to that dictates whether this is a fix or not (or if
there was a missing path that failed to mark the page dirty)

Thanks,
Matt

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

[Christian Borntraeger]

Am 18.08.26 um 15:26 schrieb Matthew Rosato:
> On 8/14/26 12:33 PM, Claudio Imbrenda wrote:
>> 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")
> 
> So I'm not strictly opposed to adding the dirtying on the 'fast path'
> but AFAIU the idea here was to still mark the page as dirty right before
> unpin.
> 
> Is it strictly required to dirty the page every time it's touched even
> if it's long-term pinned, so long as we make sure to mark the page dirty
> before we eventually unpin it?

Isnt that important for migration?

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

[Matthew Rosato]

On 8/14/26 12:33 PM, Claudio Imbrenda wrote:
> 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>

The code itself looks good to me:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Doug, can you please test (w/ lockdep enabled)?

Thanks,
Matt

> ---
>  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;
>  }
>  

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

[Christoph Schlameuss]

On Fri Aug 14, 2026 at 6:33 PM CEST, Claudio Imbrenda wrote:
> Like kvm_s390_inject_vcpu(), kvm_s390_set_irq_state() also needs the
> kvm->srcu or the slots lock when performing the Store status operation.
>
> Fix by taking kvm->srcu in kvm_s390_set_irq_state().
>
> 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>

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

> ---
>  arch/s390/kvm/interrupt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index fc4d1f8193d9..8a251f83d323 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -3230,9 +3230,9 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu, void __user *irqstate, int len
>  				break;
>  		}
>  	}
> -
>  	if (storestatus) {
> -		n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);
> +		scoped_guard(srcu, &vcpu->kvm->srcu)
> +			n = kvm_s390_store_status_unloaded(vcpu, KVM_S390_STORE_STATUS_NOADDR);
>  		return r ? r : n;
>  	}
>