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
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 <[email protected]>
---
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
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 <[email protected]>
Fixes: fb491d5500a7 ("KVM: s390: pv: asynchronous destroy for reboot")
Signed-off-by: Claudio Imbrenda <[email protected]>
---
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
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 <[email protected]>
> ---
> arch/s390/mm/gmap.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
On Tue, 11 Jul 2023 16:12:54 +0200
Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]>
> > ---
> > arch/s390/mm/gmap.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
thank you!
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 <[email protected]>
> Fixes: fb491d5500a7 ("KVM: s390: pv: asynchronous destroy for reboot")
> Signed-off-by: Claudio Imbrenda <[email protected]>
Reviewed-by: Nico Boehr <[email protected]>
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 <[email protected]>
No fixes tag?
Reviewed-by: Janosch Frank <[email protected]>
On Wed, 12 Jul 2023 13:58:49 +0200
Janosch Frank <[email protected]> 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 <[email protected]>
>
> No fixes tag?
oops, you're right
Fixes: faa2f72cb356 ("KVM: s390: pv: leak the topmost page table when
destroy fails")
>
> Reviewed-by: Janosch Frank <[email protected]>
>