* [PATCH v4 0/1] KVM: s390: fix a newly introduced bug
@ 2025-03-04 18:23 Claudio Imbrenda
2025-03-04 18:23 ` [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure Claudio Imbrenda
0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2025-03-04 18:23 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, linux-s390, frankja, borntraeger, david, nrb,
seiden, nsg, schlameuss, hca
Fix race when making a page secure (hold pte lock again)
This should fix the issues I have seen, which I think/hope are also the same
issues that David found.
v3->v4:
* move and rename s390_wiggle_split_folio() to fix a compile issue when
KVM is not selected
* removed obsolete reference to __() from comments
v2->v3:
* added check for pte_write() in make_hva_secure() [thanks David]
v1->v2:
* major refactoring
* walk the page tables only once
* when importing, manually fault in pages if needed
Claudio Imbrenda (1):
KVM: s390: pv: fix race when making a page secure
arch/s390/include/asm/gmap.h | 1 -
arch/s390/include/asm/uv.h | 3 +-
arch/s390/kernel/uv.c | 135 +++++++++++++++++++++++++++++++++--
arch/s390/kvm/gmap.c | 101 ++------------------------
arch/s390/kvm/kvm-s390.c | 25 ++++---
arch/s390/mm/gmap.c | 28 --------
6 files changed, 153 insertions(+), 140 deletions(-)
--
2.48.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure
2025-03-04 18:23 [PATCH v4 0/1] KVM: s390: fix a newly introduced bug Claudio Imbrenda
@ 2025-03-04 18:23 ` Claudio Imbrenda
2025-03-06 10:23 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: Claudio Imbrenda @ 2025-03-04 18:23 UTC (permalink / raw)
To: kvm
Cc: linux-kernel, linux-s390, frankja, borntraeger, david, nrb,
seiden, nsg, schlameuss, hca
Holding the pte lock for the page that is being converted to secure is
needed to avoid races. A previous commit removed the locking, which
caused issues. Fix by locking the pte again.
Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
Reported-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Tested-by: David Hildenbrand <david@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
arch/s390/include/asm/gmap.h | 1 -
arch/s390/include/asm/uv.h | 3 +-
arch/s390/kernel/uv.c | 135 +++++++++++++++++++++++++++++++++--
arch/s390/kvm/gmap.c | 101 ++------------------------
arch/s390/kvm/kvm-s390.c | 25 ++++---
arch/s390/mm/gmap.c | 28 --------
6 files changed, 153 insertions(+), 140 deletions(-)
diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
index 4e73ef46d4b2..9f2814d0e1e9 100644
--- a/arch/s390/include/asm/gmap.h
+++ b/arch/s390/include/asm/gmap.h
@@ -139,7 +139,6 @@ int s390_replace_asce(struct gmap *gmap);
void s390_uv_destroy_pfns(unsigned long count, unsigned long *pfns);
int __s390_uv_destroy_range(struct mm_struct *mm, unsigned long start,
unsigned long end, bool interruptible);
-int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split);
unsigned long *gmap_table_walk(struct gmap *gmap, unsigned long gaddr, int level);
/**
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index b11f5b6d0bd1..fa33a6ff2fab 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -631,9 +631,10 @@ int uv_pin_shared(unsigned long paddr);
int uv_destroy_folio(struct folio *folio);
int uv_destroy_pte(pte_t pte);
int uv_convert_from_secure_pte(pte_t pte);
-int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb);
+int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
int uv_convert_from_secure(unsigned long paddr);
int uv_convert_from_secure_folio(struct folio *folio);
+int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split);
void setup_uv(void);
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 9f05df2da2f7..63420a5f3ee5 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -206,6 +206,39 @@ int uv_convert_from_secure_pte(pte_t pte)
return uv_convert_from_secure_folio(pfn_folio(pte_pfn(pte)));
}
+/**
+ * should_export_before_import - Determine whether an export is needed
+ * before an import-like operation
+ * @uvcb: the Ultravisor control block of the UVC to be performed
+ * @mm: the mm of the process
+ *
+ * Returns whether an export is needed before every import-like operation.
+ * This is needed for shared pages, which don't trigger a secure storage
+ * exception when accessed from a different guest.
+ *
+ * Although considered as one, the Unpin Page UVC is not an actual import,
+ * so it is not affected.
+ *
+ * No export is needed also when there is only one protected VM, because the
+ * page cannot belong to the wrong VM in that case (there is no "other VM"
+ * it can belong to).
+ *
+ * Return: true if an export is needed before every import, otherwise false.
+ */
+static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_struct *mm)
+{
+ /*
+ * The misc feature indicates, among other things, that importing a
+ * shared page from a different protected VM will automatically also
+ * transfer its ownership.
+ */
+ if (uv_has_feature(BIT_UV_FEAT_MISC))
+ return false;
+ if (uvcb->cmd == UVC_CMD_UNPIN_PAGE_SHARED)
+ return false;
+ return atomic_read(&mm->context.protected_count) > 1;
+}
+
/*
* Calculate the expected ref_count for a folio that would otherwise have no
* further pins. This was cribbed from similar functions in other places in
@@ -228,7 +261,7 @@ static int expected_folio_refs(struct folio *folio)
}
/**
- * make_folio_secure() - make a folio secure
+ * __make_folio_secure() - make a folio secure
* @folio: the folio to make secure
* @uvcb: the uvcb that describes the UVC to be used
*
@@ -243,14 +276,13 @@ static int expected_folio_refs(struct folio *folio)
* -EINVAL if the UVC failed for other reasons.
*
* Context: The caller must hold exactly one extra reference on the folio
- * (it's the same logic as split_folio())
+ * (it's the same logic as split_folio()), and the folio must be
+ * locked.
*/
-int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
+static int __make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
{
int expected, cc = 0;
- if (folio_test_large(folio))
- return -E2BIG;
if (folio_test_writeback(folio))
return -EBUSY;
expected = expected_folio_refs(folio) + 1;
@@ -277,7 +309,98 @@ int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
return -EAGAIN;
return uvcb->rc == 0x10a ? -ENXIO : -EINVAL;
}
-EXPORT_SYMBOL_GPL(make_folio_secure);
+
+static int make_folio_secure(struct mm_struct *mm, struct folio *folio, struct uv_cb_header *uvcb)
+{
+ int rc;
+
+ if (!folio_trylock(folio))
+ return -EAGAIN;
+ if (should_export_before_import(uvcb, mm))
+ uv_convert_from_secure(folio_to_phys(folio));
+ rc = __make_folio_secure(folio, uvcb);
+ folio_unlock(folio);
+
+ return rc;
+}
+
+static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spinlock_t **ptl)
+{
+ pte_t *ptep = get_locked_pte(mm, hva, ptl);
+
+ if (ptep && (pte_val(*ptep) & _PAGE_INVALID)) {
+ pte_unmap_unlock(ptep, *ptl);
+ ptep = NULL;
+ }
+ return ptep;
+}
+
+/**
+ * s390_wiggle_split_folio() - try to drain extra references to a folio and optionally split
+ * @mm: the mm containing the folio to work on
+ * @folio: the folio
+ * @split: whether to split a large folio
+ *
+ * Context: Must be called while holding an extra reference to the folio;
+ * the mm lock should not be held.
+ */
+int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
+{
+ int rc;
+
+ lockdep_assert_not_held(&mm->mmap_lock);
+ folio_wait_writeback(folio);
+ lru_add_drain_all();
+ if (split) {
+ folio_lock(folio);
+ rc = split_folio(folio);
+ folio_unlock(folio);
+
+ if (rc != -EBUSY)
+ return rc;
+ }
+ return -EAGAIN;
+}
+EXPORT_SYMBOL_GPL(s390_wiggle_split_folio);
+
+int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
+{
+ struct folio *folio;
+ spinlock_t *ptelock;
+ pte_t *ptep;
+ int rc;
+
+ ptep = get_locked_valid_pte(mm, hva, &ptelock);
+ if (!ptep)
+ return -ENXIO;
+
+ folio = page_folio(pte_page(*ptep));
+ folio_get(folio);
+ /*
+ * Secure pages cannot be huge and userspace should not combine both.
+ * In case userspace does it anyway this will result in an -EFAULT for
+ * the unpack. The guest is thus never reaching secure mode.
+ * If userspace plays dirty tricks and decides to map huge pages at a
+ * later point in time, it will receive a segmentation fault or
+ * KVM_RUN will return -EFAULT.
+ */
+ if (folio_test_hugetlb(folio))
+ rc = -EFAULT;
+ else if (folio_test_large(folio))
+ rc = -E2BIG;
+ else if (!pte_write(*ptep))
+ rc = -ENXIO;
+ else
+ rc = make_folio_secure(mm, folio, uvcb);
+ pte_unmap_unlock(ptep, ptelock);
+
+ if (rc == -E2BIG || rc == -EBUSY)
+ rc = s390_wiggle_split_folio(mm, folio, rc == -E2BIG);
+ folio_put(folio);
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(make_hva_secure);
/*
* To be called with the folio locked or with an extra reference! This will
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 02adf151d4de..21580cfecc6a 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -22,92 +22,6 @@
#include "gmap.h"
-/**
- * should_export_before_import - Determine whether an export is needed
- * before an import-like operation
- * @uvcb: the Ultravisor control block of the UVC to be performed
- * @mm: the mm of the process
- *
- * Returns whether an export is needed before every import-like operation.
- * This is needed for shared pages, which don't trigger a secure storage
- * exception when accessed from a different guest.
- *
- * Although considered as one, the Unpin Page UVC is not an actual import,
- * so it is not affected.
- *
- * No export is needed also when there is only one protected VM, because the
- * page cannot belong to the wrong VM in that case (there is no "other VM"
- * it can belong to).
- *
- * Return: true if an export is needed before every import, otherwise false.
- */
-static bool should_export_before_import(struct uv_cb_header *uvcb, struct mm_struct *mm)
-{
- /*
- * The misc feature indicates, among other things, that importing a
- * shared page from a different protected VM will automatically also
- * transfer its ownership.
- */
- if (uv_has_feature(BIT_UV_FEAT_MISC))
- return false;
- if (uvcb->cmd == UVC_CMD_UNPIN_PAGE_SHARED)
- return false;
- return atomic_read(&mm->context.protected_count) > 1;
-}
-
-static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
-{
- struct folio *folio = page_folio(page);
- int rc;
-
- /*
- * Secure pages cannot be huge and userspace should not combine both.
- * In case userspace does it anyway this will result in an -EFAULT for
- * the unpack. The guest is thus never reaching secure mode.
- * If userspace plays dirty tricks and decides to map huge pages at a
- * later point in time, it will receive a segmentation fault or
- * KVM_RUN will return -EFAULT.
- */
- if (folio_test_hugetlb(folio))
- return -EFAULT;
- if (folio_test_large(folio)) {
- mmap_read_unlock(gmap->mm);
- rc = kvm_s390_wiggle_split_folio(gmap->mm, folio, true);
- mmap_read_lock(gmap->mm);
- if (rc)
- return rc;
- folio = page_folio(page);
- }
-
- if (!folio_trylock(folio))
- return -EAGAIN;
- if (should_export_before_import(uvcb, gmap->mm))
- uv_convert_from_secure(folio_to_phys(folio));
- rc = make_folio_secure(folio, uvcb);
- folio_unlock(folio);
-
- /*
- * In theory a race is possible and the folio might have become
- * large again before the folio_trylock() above. In that case, no
- * action is performed and -EAGAIN is returned; the callers will
- * have to try again later.
- * In most cases this implies running the VM again, getting the same
- * exception again, and make another attempt in this function.
- * This is expected to happen extremely rarely.
- */
- if (rc == -E2BIG)
- return -EAGAIN;
- /* The folio has too many references, try to shake some off */
- if (rc == -EBUSY) {
- mmap_read_unlock(gmap->mm);
- kvm_s390_wiggle_split_folio(gmap->mm, folio, false);
- mmap_read_lock(gmap->mm);
- return -EAGAIN;
- }
-
- return rc;
-}
-
/**
* gmap_make_secure() - make one guest page secure
* @gmap: the guest gmap
@@ -115,22 +29,21 @@ static int __gmap_make_secure(struct gmap *gmap, struct page *page, void *uvcb)
* @uvcb: the UVCB specifying which operation needs to be performed
*
* Context: needs to be called with kvm->srcu held.
- * Return: 0 on success, < 0 in case of error (see __gmap_make_secure()).
+ * Return: 0 on success, < 0 in case of error.
*/
int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
{
struct kvm *kvm = gmap->private;
- struct page *page;
+ unsigned long vmaddr;
int rc = 0;
lockdep_assert_held(&kvm->srcu);
- page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
- mmap_read_lock(gmap->mm);
- if (page)
- rc = __gmap_make_secure(gmap, page, uvcb);
- kvm_release_page_clean(page);
- mmap_read_unlock(gmap->mm);
+ vmaddr = gfn_to_hva(kvm, gpa_to_gfn(gaddr));
+ if (kvm_is_error_hva(vmaddr))
+ rc = -ENXIO;
+ else
+ rc = make_hva_secure(gmap->mm, vmaddr, uvcb);
return rc;
}
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ebecb96bacce..020502af7dc9 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -4952,6 +4952,7 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
{
unsigned int flags = 0;
unsigned long gaddr;
+ int rc;
gaddr = current->thread.gmap_teid.addr * PAGE_SIZE;
if (kvm_s390_cur_gmap_fault_is_write())
@@ -4961,16 +4962,6 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
case 0:
vcpu->stat.exit_null++;
break;
- case PGM_NON_SECURE_STORAGE_ACCESS:
- kvm_s390_assert_primary_as(vcpu);
- /*
- * This is normal operation; a page belonging to a protected
- * guest has not been imported yet. Try to import the page into
- * the protected guest.
- */
- if (gmap_convert_to_secure(vcpu->arch.gmap, gaddr) == -EINVAL)
- send_sig(SIGSEGV, current, 0);
- break;
case PGM_SECURE_STORAGE_ACCESS:
case PGM_SECURE_STORAGE_VIOLATION:
kvm_s390_assert_primary_as(vcpu);
@@ -4995,6 +4986,20 @@ static int vcpu_post_run_handle_fault(struct kvm_vcpu *vcpu)
send_sig(SIGSEGV, current, 0);
}
break;
+ case PGM_NON_SECURE_STORAGE_ACCESS:
+ kvm_s390_assert_primary_as(vcpu);
+ /*
+ * This is normal operation; a page belonging to a protected
+ * guest has not been imported yet. Try to import the page into
+ * the protected guest.
+ */
+ rc = gmap_convert_to_secure(vcpu->arch.gmap, gaddr);
+ if (rc == -EINVAL)
+ send_sig(SIGSEGV, current, 0);
+ if (rc != -ENXIO)
+ break;
+ flags = FAULT_FLAG_WRITE;
+ fallthrough;
case PGM_PROTECTION:
case PGM_SEGMENT_TRANSLATION:
case PGM_PAGE_TRANSLATION:
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 94d927785800..d14b488e7a1f 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2626,31 +2626,3 @@ int s390_replace_asce(struct gmap *gmap)
return 0;
}
EXPORT_SYMBOL_GPL(s390_replace_asce);
-
-/**
- * kvm_s390_wiggle_split_folio() - try to drain extra references to a folio and optionally split
- * @mm: the mm containing the folio to work on
- * @folio: the folio
- * @split: whether to split a large folio
- *
- * Context: Must be called while holding an extra reference to the folio;
- * the mm lock should not be held.
- */
-int kvm_s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
-{
- int rc;
-
- lockdep_assert_not_held(&mm->mmap_lock);
- folio_wait_writeback(folio);
- lru_add_drain_all();
- if (split) {
- folio_lock(folio);
- rc = split_folio(folio);
- folio_unlock(folio);
-
- if (rc != -EBUSY)
- return rc;
- }
- return -EAGAIN;
-}
-EXPORT_SYMBOL_GPL(kvm_s390_wiggle_split_folio);
--
2.48.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure
2025-03-04 18:23 ` [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure Claudio Imbrenda
@ 2025-03-06 10:23 ` David Hildenbrand
2025-03-06 22:07 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2025-03-06 10:23 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: linux-kernel, linux-s390, frankja, borntraeger, nrb, seiden, nsg,
schlameuss, hca
> /**
> - * make_folio_secure() - make a folio secure
> + * __make_folio_secure() - make a folio secure
> * @folio: the folio to make secure
> * @uvcb: the uvcb that describes the UVC to be used
> *
> @@ -243,14 +276,13 @@ static int expected_folio_refs(struct folio *folio)
> * -EINVAL if the UVC failed for other reasons.
> *
> * Context: The caller must hold exactly one extra reference on the folio
> - * (it's the same logic as split_folio())
> + * (it's the same logic as split_folio()), and the folio must be
> + * locked.
> */
> -int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
> +static int __make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
One more nit: -EBUSY can no longer be returned from his function, so you
might just remove it from the doc above.
While chasing a very weird folio split bug that seems to result in late
validation issues (:/), I was wondering if __gmap_destroy_page could
similarly be problematic.
We're now no longer holding the PTL while performing the operation.
(not that that would explain the issue I am chasing, because
gmap_destroy_page() is never called in my setup)
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure
2025-03-06 10:23 ` David Hildenbrand
@ 2025-03-06 22:07 ` David Hildenbrand
2025-03-06 22:17 ` David Hildenbrand
2025-03-07 13:18 ` Claudio Imbrenda
0 siblings, 2 replies; 6+ messages in thread
From: David Hildenbrand @ 2025-03-06 22:07 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: linux-kernel, linux-s390, frankja, borntraeger, nrb, seiden, nsg,
schlameuss, hca
On 06.03.25 11:23, David Hildenbrand wrote:
>> /**
>> - * make_folio_secure() - make a folio secure
>> + * __make_folio_secure() - make a folio secure
>> * @folio: the folio to make secure
>> * @uvcb: the uvcb that describes the UVC to be used
>> *
>> @@ -243,14 +276,13 @@ static int expected_folio_refs(struct folio *folio)
>> * -EINVAL if the UVC failed for other reasons.
>> *
>> * Context: The caller must hold exactly one extra reference on the folio
>> - * (it's the same logic as split_folio())
>> + * (it's the same logic as split_folio()), and the folio must be
>> + * locked.
>> */
>> -int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
>> +static int __make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
>
> One more nit: -EBUSY can no longer be returned from his function, so you
> might just remove it from the doc above.
>
>
> While chasing a very weird folio split bug that seems to result in late
> validation issues (:/), I was wondering if __gmap_destroy_page could
> similarly be problematic.
>
> We're now no longer holding the PTL while performing the operation.
>
> (not that that would explain the issue I am chasing, because
> gmap_destroy_page() is never called in my setup)
>
Okay, I've been debugging for way to long the weird issue I am seeing, and I
did not find the root cause yet. But the following things are problematic:
1) To walk the page tables, we need the mmap lock in read mode.
2) To walk the page tables, we must know that a VMA exists
3) get_locked_pte() must not be used on hugetlb areas.
Further, the following things should be cleaned up
4) s390_wiggle_split_folio() is only used in that file
5) gmap_make_secure() likely should be returning -EFAULT
See below, I went with a folio_walk (which also checks for pte_present()
like the old code did, but that should not matter here) so we can get rid of the
get_locked_pte() usage completely.
From 1b9a4306b79a352daf80708252d166114e7335de Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Thu, 6 Mar 2025 22:43:43 +0100
Subject: [PATCH] merge
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/s390/include/asm/uv.h | 1 -
arch/s390/kernel/uv.c | 41 ++++++++++++++++++--------------------
arch/s390/kvm/gmap.c | 2 +-
3 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index fa33a6ff2fabf..46fb0ef6f9847 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -634,7 +634,6 @@ int uv_convert_from_secure_pte(pte_t pte);
int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
int uv_convert_from_secure(unsigned long paddr);
int uv_convert_from_secure_folio(struct folio *folio);
-int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split);
void setup_uv(void);
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 63420a5f3ee57..11a1894e63405 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -270,7 +270,6 @@ static int expected_folio_refs(struct folio *folio)
*
* Return: 0 on success;
* -EBUSY if the folio is in writeback or has too many references;
- * -E2BIG if the folio is large;
* -EAGAIN if the UVC needs to be attempted again;
* -ENXIO if the address is not mapped;
* -EINVAL if the UVC failed for other reasons.
@@ -324,17 +323,6 @@ static int make_folio_secure(struct mm_struct *mm, struct folio *folio, struct u
return rc;
}
-static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spinlock_t **ptl)
-{
- pte_t *ptep = get_locked_pte(mm, hva, ptl);
-
- if (ptep && (pte_val(*ptep) & _PAGE_INVALID)) {
- pte_unmap_unlock(ptep, *ptl);
- ptep = NULL;
- }
- return ptep;
-}
-
/**
* s390_wiggle_split_folio() - try to drain extra references to a folio and optionally split
* @mm: the mm containing the folio to work on
@@ -344,7 +332,7 @@ static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spin
* Context: Must be called while holding an extra reference to the folio;
* the mm lock should not be held.
*/
-int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
+static int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
{
int rc;
@@ -361,20 +349,28 @@ int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool spli
}
return -EAGAIN;
}
-EXPORT_SYMBOL_GPL(s390_wiggle_split_folio);
int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
{
+ struct vm_area_struct *vma;
+ struct folio_walk fw;
struct folio *folio;
- spinlock_t *ptelock;
- pte_t *ptep;
int rc;
- ptep = get_locked_valid_pte(mm, hva, &ptelock);
- if (!ptep)
+ mmap_read_lock(mm);
+
+ vma = vma_lookup(mm, hva);
+ if (!vma) {
+ mmap_read_unlock(mm);
+ return -EFAULT;
+ }
+
+ folio = folio_walk_start(&fw, vma, hva, 0);
+ if (!folio) {
+ mmap_read_unlock(mm);
return -ENXIO;
+ }
- folio = page_folio(pte_page(*ptep));
folio_get(folio);
/*
* Secure pages cannot be huge and userspace should not combine both.
@@ -385,14 +381,15 @@ int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header
* KVM_RUN will return -EFAULT.
*/
if (folio_test_hugetlb(folio))
- rc = -EFAULT;
+ rc = -EFAULT;
else if (folio_test_large(folio))
rc = -E2BIG;
- else if (!pte_write(*ptep))
+ else if (!pte_write(fw.pte) || (pte_val(fw.pte) & _PAGE_INVALID))
rc = -ENXIO;
else
rc = make_folio_secure(mm, folio, uvcb);
- pte_unmap_unlock(ptep, ptelock);
+ folio_walk_end(&fw, vma);
+ mmap_read_unlock(mm);
if (rc == -E2BIG || rc == -EBUSY)
rc = s390_wiggle_split_folio(mm, folio, rc == -E2BIG);
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 21580cfecc6ac..1a88b32e7c134 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -41,7 +41,7 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
vmaddr = gfn_to_hva(kvm, gpa_to_gfn(gaddr));
if (kvm_is_error_hva(vmaddr))
- rc = -ENXIO;
+ rc = -EFAULT;
else
rc = make_hva_secure(gmap->mm, vmaddr, uvcb);
--
2.48.1
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure
2025-03-06 22:07 ` David Hildenbrand
@ 2025-03-06 22:17 ` David Hildenbrand
2025-03-07 13:18 ` Claudio Imbrenda
1 sibling, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2025-03-06 22:17 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: linux-kernel, linux-s390, frankja, borntraeger, nrb, seiden, nsg,
schlameuss, hca
On 06.03.25 23:07, David Hildenbrand wrote:
> On 06.03.25 11:23, David Hildenbrand wrote:
>>> /**
>>> - * make_folio_secure() - make a folio secure
>>> + * __make_folio_secure() - make a folio secure
>>> * @folio: the folio to make secure
>>> * @uvcb: the uvcb that describes the UVC to be used
>>> *
>>> @@ -243,14 +276,13 @@ static int expected_folio_refs(struct folio *folio)
>>> * -EINVAL if the UVC failed for other reasons.
>>> *
>>> * Context: The caller must hold exactly one extra reference on the folio
>>> - * (it's the same logic as split_folio())
>>> + * (it's the same logic as split_folio()), and the folio must be
>>> + * locked.
>>> */
>>> -int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
>>> +static int __make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
>>
>> One more nit: -EBUSY can no longer be returned from his function, so you
>> might just remove it from the doc above.
>>
>>
>> While chasing a very weird folio split bug that seems to result in late
>> validation issues (:/), I was wondering if __gmap_destroy_page could
>> similarly be problematic.
>>
>> We're now no longer holding the PTL while performing the operation.
>>
>> (not that that would explain the issue I am chasing, because
>> gmap_destroy_page() is never called in my setup)
>>
>
> Okay, I've been debugging for way to long the weird issue I am seeing, and I
> did not find the root cause yet. But the following things are problematic:
>
> 1) To walk the page tables, we need the mmap lock in read mode.
>
> 2) To walk the page tables, we must know that a VMA exists
>
> 3) get_locked_pte() must not be used on hugetlb areas.
>
> Further, the following things should be cleaned up
>
> 4) s390_wiggle_split_folio() is only used in that file
>
> 5) gmap_make_secure() likely should be returning -EFAULT
>
>
> See below, I went with a folio_walk (which also checks for pte_present()
> like the old code did, but that should not matter here) so we can get rid of the
> get_locked_pte() usage completely.
>
>
> From 1b9a4306b79a352daf80708252d166114e7335de Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Thu, 6 Mar 2025 22:43:43 +0100
> Subject: [PATCH] merge
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/include/asm/uv.h | 1 -
> arch/s390/kernel/uv.c | 41 ++++++++++++++++++--------------------
> arch/s390/kvm/gmap.c | 2 +-
> 3 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index fa33a6ff2fabf..46fb0ef6f9847 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -634,7 +634,6 @@ int uv_convert_from_secure_pte(pte_t pte);
> int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
> int uv_convert_from_secure(unsigned long paddr);
> int uv_convert_from_secure_folio(struct folio *folio);
> -int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split);
>
> void setup_uv(void);
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 63420a5f3ee57..11a1894e63405 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -270,7 +270,6 @@ static int expected_folio_refs(struct folio *folio)
> *
> * Return: 0 on success;
> * -EBUSY if the folio is in writeback or has too many references;
> - * -E2BIG if the folio is large;
> * -EAGAIN if the UVC needs to be attempted again;
> * -ENXIO if the address is not mapped;
> * -EINVAL if the UVC failed for other reasons.
> @@ -324,17 +323,6 @@ static int make_folio_secure(struct mm_struct *mm, struct folio *folio, struct u
> return rc;
> }
>
> -static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spinlock_t **ptl)
> -{
> - pte_t *ptep = get_locked_pte(mm, hva, ptl);
> -
> - if (ptep && (pte_val(*ptep) & _PAGE_INVALID)) {
> - pte_unmap_unlock(ptep, *ptl);
> - ptep = NULL;
> - }
> - return ptep;
> -}
> -
> /**
> * s390_wiggle_split_folio() - try to drain extra references to a folio and optionally split
> * @mm: the mm containing the folio to work on
> @@ -344,7 +332,7 @@ static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spin
> * Context: Must be called while holding an extra reference to the folio;
> * the mm lock should not be held.
> */
> -int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
> +static int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
> {
> int rc;
>
> @@ -361,20 +349,28 @@ int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool spli
> }
> return -EAGAIN;
> }
> -EXPORT_SYMBOL_GPL(s390_wiggle_split_folio);
>
> int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
> {
> + struct vm_area_struct *vma;
> + struct folio_walk fw;
> struct folio *folio;
> - spinlock_t *ptelock;
> - pte_t *ptep;
> int rc;
>
> - ptep = get_locked_valid_pte(mm, hva, &ptelock);
> - if (!ptep)
> + mmap_read_lock(mm);
> +
> + vma = vma_lookup(mm, hva);
> + if (!vma) {
> + mmap_read_unlock(mm);
> + return -EFAULT;
> + }
> +
> + folio = folio_walk_start(&fw, vma, hva, 0);
> + if (!folio) {
> + mmap_read_unlock(mm);
> return -ENXIO;
> + }
>
> - folio = page_folio(pte_page(*ptep));
> folio_get(folio);
> /*
> * Secure pages cannot be huge and userspace should not combine both.
> @@ -385,14 +381,15 @@ int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header
> * KVM_RUN will return -EFAULT.
> */
> if (folio_test_hugetlb(folio))
> - rc = -EFAULT;
> + rc = -EFAULT;
> else if (folio_test_large(folio))
> rc = -E2BIG;
> - else if (!pte_write(*ptep))
> + else if (!pte_write(fw.pte) || (pte_val(fw.pte) & _PAGE_INVALID))
> rc = -ENXIO;
> else
> rc = make_folio_secure(mm, folio, uvcb);
> - pte_unmap_unlock(ptep, ptelock);
> + folio_walk_end(&fw, vma);
> + mmap_read_unlock(mm);
>
> if (rc == -E2BIG || rc == -EBUSY)
> rc = s390_wiggle_split_folio(mm, folio, rc == -E2BIG);
> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index 21580cfecc6ac..1a88b32e7c134 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -41,7 +41,7 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>
> vmaddr = gfn_to_hva(kvm, gpa_to_gfn(gaddr));
> if (kvm_is_error_hva(vmaddr))
> - rc = -ENXIO;
> + rc = -EFAULT;
> else
> rc = make_hva_secure(gmap->mm, vmaddr, uvcb);
>
The following likely on top as a cleanup as well:
diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
index 1a88b32e7c134..6d8944d1b4a0c 100644
--- a/arch/s390/kvm/gmap.c
+++ b/arch/s390/kvm/gmap.c
@@ -35,17 +35,13 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
{
struct kvm *kvm = gmap->private;
unsigned long vmaddr;
- int rc = 0;
lockdep_assert_held(&kvm->srcu);
vmaddr = gfn_to_hva(kvm, gpa_to_gfn(gaddr));
if (kvm_is_error_hva(vmaddr))
- rc = -EFAULT;
- else
- rc = make_hva_secure(gmap->mm, vmaddr, uvcb);
-
- return rc;
+ return -EFAULT;
+ return make_hva_secure(gmap->mm, vmaddr, uvcb);
}
int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr)
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure
2025-03-06 22:07 ` David Hildenbrand
2025-03-06 22:17 ` David Hildenbrand
@ 2025-03-07 13:18 ` Claudio Imbrenda
1 sibling, 0 replies; 6+ messages in thread
From: Claudio Imbrenda @ 2025-03-07 13:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: kvm, linux-kernel, linux-s390, frankja, borntraeger, nrb, seiden,
nsg, schlameuss, hca
On Thu, 6 Mar 2025 23:07:04 +0100
David Hildenbrand <david@redhat.com> wrote:
> On 06.03.25 11:23, David Hildenbrand wrote:
> >> /**
> >> - * make_folio_secure() - make a folio secure
> >> + * __make_folio_secure() - make a folio secure
> >> * @folio: the folio to make secure
> >> * @uvcb: the uvcb that describes the UVC to be used
> >> *
> >> @@ -243,14 +276,13 @@ static int expected_folio_refs(struct folio *folio)
> >> * -EINVAL if the UVC failed for other reasons.
> >> *
> >> * Context: The caller must hold exactly one extra reference on the folio
> >> - * (it's the same logic as split_folio())
> >> + * (it's the same logic as split_folio()), and the folio must be
> >> + * locked.
> >> */
> >> -int make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
> >> +static int __make_folio_secure(struct folio *folio, struct uv_cb_header *uvcb)
> >
> > One more nit: -EBUSY can no longer be returned from his function, so you
> > might just remove it from the doc above.
> >
> >
> > While chasing a very weird folio split bug that seems to result in late
> > validation issues (:/), I was wondering if __gmap_destroy_page could
> > similarly be problematic.
> >
> > We're now no longer holding the PTL while performing the operation.
> >
> > (not that that would explain the issue I am chasing, because
> > gmap_destroy_page() is never called in my setup)
> >
>
> Okay, I've been debugging for way to long the weird issue I am seeing, and I
> did not find the root cause yet. But the following things are problematic:
>
> 1) To walk the page tables, we need the mmap lock in read mode.
>
> 2) To walk the page tables, we must know that a VMA exists
>
> 3) get_locked_pte() must not be used on hugetlb areas.
>
> Further, the following things should be cleaned up
>
> 4) s390_wiggle_split_folio() is only used in that file
>
> 5) gmap_make_secure() likely should be returning -EFAULT
>
>
> See below, I went with a folio_walk (which also checks for pte_present()
> like the old code did, but that should not matter here) so we can get rid of the
> get_locked_pte() usage completely.
I shall merge this into my patch, thanks a lot!
>
>
> From 1b9a4306b79a352daf80708252d166114e7335de Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Thu, 6 Mar 2025 22:43:43 +0100
> Subject: [PATCH] merge
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/s390/include/asm/uv.h | 1 -
> arch/s390/kernel/uv.c | 41 ++++++++++++++++++--------------------
> arch/s390/kvm/gmap.c | 2 +-
> 3 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index fa33a6ff2fabf..46fb0ef6f9847 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -634,7 +634,6 @@ int uv_convert_from_secure_pte(pte_t pte);
> int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb);
> int uv_convert_from_secure(unsigned long paddr);
> int uv_convert_from_secure_folio(struct folio *folio);
> -int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split);
>
> void setup_uv(void);
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 63420a5f3ee57..11a1894e63405 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -270,7 +270,6 @@ static int expected_folio_refs(struct folio *folio)
> *
> * Return: 0 on success;
> * -EBUSY if the folio is in writeback or has too many references;
> - * -E2BIG if the folio is large;
> * -EAGAIN if the UVC needs to be attempted again;
> * -ENXIO if the address is not mapped;
> * -EINVAL if the UVC failed for other reasons.
> @@ -324,17 +323,6 @@ static int make_folio_secure(struct mm_struct *mm, struct folio *folio, struct u
> return rc;
> }
>
> -static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spinlock_t **ptl)
> -{
> - pte_t *ptep = get_locked_pte(mm, hva, ptl);
> -
> - if (ptep && (pte_val(*ptep) & _PAGE_INVALID)) {
> - pte_unmap_unlock(ptep, *ptl);
> - ptep = NULL;
> - }
> - return ptep;
> -}
> -
> /**
> * s390_wiggle_split_folio() - try to drain extra references to a folio and optionally split
> * @mm: the mm containing the folio to work on
> @@ -344,7 +332,7 @@ static pte_t *get_locked_valid_pte(struct mm_struct *mm, unsigned long hva, spin
> * Context: Must be called while holding an extra reference to the folio;
> * the mm lock should not be held.
> */
> -int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
> +static int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool split)
> {
> int rc;
>
> @@ -361,20 +349,28 @@ int s390_wiggle_split_folio(struct mm_struct *mm, struct folio *folio, bool spli
> }
> return -EAGAIN;
> }
> -EXPORT_SYMBOL_GPL(s390_wiggle_split_folio);
>
> int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
> {
> + struct vm_area_struct *vma;
> + struct folio_walk fw;
> struct folio *folio;
> - spinlock_t *ptelock;
> - pte_t *ptep;
> int rc;
>
> - ptep = get_locked_valid_pte(mm, hva, &ptelock);
> - if (!ptep)
> + mmap_read_lock(mm);
> +
> + vma = vma_lookup(mm, hva);
> + if (!vma) {
> + mmap_read_unlock(mm);
> + return -EFAULT;
> + }
> +
> + folio = folio_walk_start(&fw, vma, hva, 0);
> + if (!folio) {
> + mmap_read_unlock(mm);
> return -ENXIO;
> + }
>
> - folio = page_folio(pte_page(*ptep));
> folio_get(folio);
> /*
> * Secure pages cannot be huge and userspace should not combine both.
> @@ -385,14 +381,15 @@ int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header
> * KVM_RUN will return -EFAULT.
> */
> if (folio_test_hugetlb(folio))
> - rc = -EFAULT;
> + rc = -EFAULT;
> else if (folio_test_large(folio))
> rc = -E2BIG;
> - else if (!pte_write(*ptep))
> + else if (!pte_write(fw.pte) || (pte_val(fw.pte) & _PAGE_INVALID))
> rc = -ENXIO;
> else
> rc = make_folio_secure(mm, folio, uvcb);
> - pte_unmap_unlock(ptep, ptelock);
> + folio_walk_end(&fw, vma);
> + mmap_read_unlock(mm);
>
> if (rc == -E2BIG || rc == -EBUSY)
> rc = s390_wiggle_split_folio(mm, folio, rc == -E2BIG);
> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index 21580cfecc6ac..1a88b32e7c134 100644
> --- a/arch/s390/kvm/gmap.c
> +++ b/arch/s390/kvm/gmap.c
> @@ -41,7 +41,7 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
>
> vmaddr = gfn_to_hva(kvm, gpa_to_gfn(gaddr));
> if (kvm_is_error_hva(vmaddr))
> - rc = -ENXIO;
> + rc = -EFAULT;
> else
> rc = make_hva_secure(gmap->mm, vmaddr, uvcb);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-07 13:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-04 18:23 [PATCH v4 0/1] KVM: s390: fix a newly introduced bug Claudio Imbrenda
2025-03-04 18:23 ` [PATCH v4 1/1] KVM: s390: pv: fix race when making a page secure Claudio Imbrenda
2025-03-06 10:23 ` David Hildenbrand
2025-03-06 22:07 ` David Hildenbrand
2025-03-06 22:17 ` David Hildenbrand
2025-03-07 13:18 ` Claudio Imbrenda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox