[PATCH v2 0/2] KVM: s390: pv: fix two small bugs

[PATCH v2 0/2] KVM: s390: pv: fix two small bugs

[Claudio Imbrenda]

This series fixes 2 small bugs introduced with asynchronous teardown.

The first patch (unmodified since v1, but did not get any reviews)
fixes a potential race that can cause crashes during shutdown on
machines that don't support protected virtualization.

The second patch adds a missing initialization when replacing the ASCE
for a protected guest. This could potentially cause issues and crashes
if lowcore is affected.


v1->v2:

* added the second patch

Claudio Imbrenda (2):
  KVM: s390: pv: simplify shutdown and fix race
  KVM: s390: pv: fix index value of replaced ASCE

 arch/s390/kvm/pv.c  | 8 ++++++--
 arch/s390/mm/gmap.c | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] KVM: s390: pv: simplify shutdown and fix race

[Claudio Imbrenda]

Simplify the shutdown of non-protected VMs. There is no need to do
complex manipulations of the counter if it was zero.

This also fixes a very rare race which caused pages to be torn down
from the address space with a non-zero counter even on older machines
that don't support the UVC instruction, causing a crash.

Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Fixes: fb491d5500a7 ("KVM: s390: pv: asynchronous destroy for reboot")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/kvm/pv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c
index 3ce5f4351156..899f3b8ac011 100644
--- a/arch/s390/kvm/pv.c
+++ b/arch/s390/kvm/pv.c
@@ -411,8 +411,12 @@ int kvm_s390_pv_deinit_cleanup_all(struct kvm *kvm, u16 *rc, u16 *rrc)
 	u16 _rc, _rrc;
 	int cc = 0;
 
-	/* Make sure the counter does not reach 0 before calling s390_uv_destroy_range */
-	atomic_inc(&kvm->mm->context.protected_count);
+	/*
+	 * Nothing to do if the counter was already 0. Otherwise make sure
+	 * the counter does not reach 0 before calling s390_uv_destroy_range.
+	 */
+	if (!atomic_inc_not_zero(&kvm->mm->context.protected_count))
+		return 0;
 
 	*rc = 1;
 	/* If the current VM is protected, destroy it */
-- 
2.41.0

Re: [PATCH v2 1/2] KVM: s390: pv: simplify shutdown and fix race

[Nico Boehr]

Quoting Claudio Imbrenda (2023-07-05 13:19:36)
> Simplify the shutdown of non-protected VMs. There is no need to do
> complex manipulations of the counter if it was zero.
> 
> This also fixes a very rare race which caused pages to be torn down
> from the address space with a non-zero counter even on older machines
> that don't support the UVC instruction, causing a crash.
> 
> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> Fixes: fb491d5500a7 ("KVM: s390: pv: asynchronous destroy for reboot")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>

[PATCH v2 2/2] KVM: s390: pv: fix index value of replaced ASCE

[Claudio Imbrenda]

The index field of the struct page corresponding to a guest ASCE should
be 0. When replacing the ASCE in s390_replace_asce(), the index of the
new ASCE should also be set to 0.

Having the wrong index might lead to the wrong addresses being passed
around when notifying pte invalidations, and eventually to validity
intercepts (VM crash) if the prefix gets unmapped and the notifier gets
called with the wrong address.

Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/mm/gmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index f4b6fc746fce..7c77f246e101 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2853,6 +2853,7 @@ int s390_replace_asce(struct gmap *gmap)
 	page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER);
 	if (!page)
 		return -ENOMEM;
+	page->index = 0;
 	table = page_to_virt(page);
 	memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT));
 
-- 
2.41.0

Re: [PATCH v2 2/2] KVM: s390: pv: fix index value of replaced ASCE

[Philippe Mathieu-Daudé]

On 5/7/23 13:19, Claudio Imbrenda wrote:
> The index field of the struct page corresponding to a guest ASCE should
> be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> new ASCE should also be set to 0.
> 
> Having the wrong index might lead to the wrong addresses being passed
> around when notifying pte invalidations, and eventually to validity
> intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> called with the wrong address.

Can that also happen in crst_table_alloc()?

> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>   arch/s390/mm/gmap.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/2] KVM: s390: pv: fix index value of replaced ASCE

[Claudio Imbrenda]

On Tue, 11 Jul 2023 16:12:54 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 5/7/23 13:19, Claudio Imbrenda wrote:
> > The index field of the struct page corresponding to a guest ASCE should
> > be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> > new ASCE should also be set to 0.
> > 
> > Having the wrong index might lead to the wrong addresses being passed
> > around when notifying pte invalidations, and eventually to validity
> > intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> > called with the wrong address.  
> 
> Can that also happen in crst_table_alloc()?

no. ->index is only used for gmap page tables (guest mapping), from
the root to pmd tables. the last level (ptes) is shared between guest
mapping and QEMU address space. i.e. the ptes are the ones from QEMU.

the last level of page tables is not expected to have ->index set
(since that one actually belongs to QEMU and not to the guest mapping)

guest page tables (all levels, except ptes) are allocated with
gmap_alloc_table(), which correctly sets ->index

the top level of a guest mapping is allocated in gmap_alloc(), which
also correctly sets ->index to 0.

> 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > ---
> >   arch/s390/mm/gmap.c | 1 +
> >   1 file changed, 1 insertion(+)  
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

thank you!
 

Re: [PATCH v2 2/2] KVM: s390: pv: fix index value of replaced ASCE

[Janosch Frank]

On 7/5/23 13:19, Claudio Imbrenda wrote:
> The index field of the struct page corresponding to a guest ASCE should
> be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> new ASCE should also be set to 0.
> 
> Having the wrong index might lead to the wrong addresses being passed
> around when notifying pte invalidations, and eventually to validity
> intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> called with the wrong address.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

No fixes tag?

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [PATCH v2 2/2] KVM: s390: pv: fix index value of replaced ASCE

[Claudio Imbrenda]

On Wed, 12 Jul 2023 13:58:49 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 7/5/23 13:19, Claudio Imbrenda wrote:
> > The index field of the struct page corresponding to a guest ASCE should
> > be 0. When replacing the ASCE in s390_replace_asce(), the index of the
> > new ASCE should also be set to 0.
> > 
> > Having the wrong index might lead to the wrong addresses being passed
> > around when notifying pte invalidations, and eventually to validity
> > intercepts (VM crash) if the prefix gets unmapped and the notifier gets
> > called with the wrong address.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>  
> 
> No fixes tag?

oops, you're right

Fixes: faa2f72cb356 ("KVM: s390: pv: leak the topmost page table when
destroy fails")


> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>