| Message ID | 20220414080311.1084834-2-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: pv: implement lazy destroy for reboot | expand |
On 4/14/22 10:02, Claudio Imbrenda wrote: > Each secure guest must have a unique ASCE (address space control > element); we must avoid that new guests use the same page for their > ASCE, to avoid errors. > > Since the ASCE mostly consists of the address of the topmost page table > (plus some flags), we must not return that memory to the pool unless > the ASCE is no longer in use. > > Only a successful Destroy Secure Configuration UVC will make the ASCE > reusable again. > > If the Destroy Configuration UVC fails, the ASCE cannot be reused for a > secure guest (either for the ASCE or for other memory areas). To avoid > a collision, it must not be used again. This is a permanent error and > the page becomes in practice unusable, so we set it aside and leak it. > On failure we already leak other memory that belongs to the ultravisor > (i.e. the variable and base storage for a guest) and not leaking the > topmost page table was an oversight. > > This error (and thus the leakage) should not happen unless the hardware > is broken or KVM has some unknown serious bug. > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > + /* > + * In case the ASCE needs to be "removed" multiple times, for example > + * if the VM is rebooted into secure mode several times > + * concurrently, or if s390_replace_asce fails after calling > + * s390_remove_old_asce and is attempted again later. In that case > + * the old asce has been removed from the list, and therefore it > + * will not be freed when the VM terminates, but the ASCE is still > + * in use and still pointed to. > + * A subsequent call to replace_asce will follow the pointer and try > + * to remove the same page from the list again. > + * Therefore it's necessary that the page of the ASCE has valid > + * pointers, so list_del can work (and do nothing) without > + * dereferencing stale or invalid pointers. > + */ > + INIT_LIST_HEAD(&old->lru); > + spin_unlock(&gmap->guest_table_lock); > +} > +EXPORT_SYMBOL_GPL(s390_remove_old_asce); > + > +/** > + * s390_replace_asce - Try to replace the current ASCE of a gmap with > + * another equivalent one. with a copy? > + * @gmap the gmap > + * > + * If the allocation of the new top level page table fails, the ASCE is not > + * replaced. > + * In any case, the old ASCE is always removed from the list. Therefore the removed from the gmap crst list > + * caller has to make sure to save a pointer to it beforehands, unless an > + * intentional leak is intended. > + */ > +int s390_replace_asce(struct gmap *gmap) > +{ > + unsigned long asce; > + struct page *page; > + void *table; > + > + s390_remove_old_asce(gmap); > + > + page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER); > + if (!page) > + return -ENOMEM; > + table = page_to_virt(page); > + memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT)); > + > + /* > + * The caller has to deal with the old ASCE, but here we make sure > + * the new one is properly added to the list of page tables, so that > + * it will be freed when the VM is torn down. > + */ > + spin_lock(&gmap->guest_table_lock); > + list_add(&page->lru, &gmap->crst_list); > + spin_unlock(&gmap->guest_table_lock); > + > + /* Set new table origin while preserving existing ASCE control bits */ > + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); > + WRITE_ONCE(gmap->asce, asce); > + WRITE_ONCE(gmap->mm->context.gmap_asce, asce); > + WRITE_ONCE(gmap->table, table); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(s390_replace_asce);
On Thu, 14 Apr 2022 13:30:33 +0200 Janosch Frank <frankja@linux.ibm.com> wrote: > On 4/14/22 10:02, Claudio Imbrenda wrote: > > Each secure guest must have a unique ASCE (address space control > > element); we must avoid that new guests use the same page for their > > ASCE, to avoid errors. > > > > Since the ASCE mostly consists of the address of the topmost page table > > (plus some flags), we must not return that memory to the pool unless > > the ASCE is no longer in use. > > > > Only a successful Destroy Secure Configuration UVC will make the ASCE > > reusable again. > > > > If the Destroy Configuration UVC fails, the ASCE cannot be reused for a > > secure guest (either for the ASCE or for other memory areas). To avoid > > a collision, it must not be used again. This is a permanent error and > > the page becomes in practice unusable, so we set it aside and leak it. > > On failure we already leak other memory that belongs to the ultravisor > > (i.e. the variable and base storage for a guest) and not leaking the > > topmost page table was an oversight. > > > > This error (and thus the leakage) should not happen unless the hardware > > is broken or KVM has some unknown serious bug. > > > > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > > > + /* > > + * In case the ASCE needs to be "removed" multiple times, for example > > + * if the VM is rebooted into secure mode several times > > + * concurrently, or if s390_replace_asce fails after calling > > + * s390_remove_old_asce and is attempted again later. In that case > > + * the old asce has been removed from the list, and therefore it > > + * will not be freed when the VM terminates, but the ASCE is still > > + * in use and still pointed to. > > + * A subsequent call to replace_asce will follow the pointer and try > > + * to remove the same page from the list again. > > + * Therefore it's necessary that the page of the ASCE has valid > > + * pointers, so list_del can work (and do nothing) without > > + * dereferencing stale or invalid pointers. > > + */ > > + INIT_LIST_HEAD(&old->lru); > > + spin_unlock(&gmap->guest_table_lock); > > +} > > +EXPORT_SYMBOL_GPL(s390_remove_old_asce); > > + > > +/** > > + * s390_replace_asce - Try to replace the current ASCE of a gmap with > > + * another equivalent one. > > with a copy? will fix > > > + * @gmap the gmap > > + * > > + * If the allocation of the new top level page table fails, the ASCE is not > > + * replaced. > > + * In any case, the old ASCE is always removed from the list. Therefore the > > removed from the gmap crst list will fix > > > + * caller has to make sure to save a pointer to it beforehands, unless an > > + * intentional leak is intended. > > + */ > > +int s390_replace_asce(struct gmap *gmap) > > +{ > > + unsigned long asce; > > + struct page *page; > > + void *table; > > + > > + s390_remove_old_asce(gmap); > > + > > + page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER); > > + if (!page) > > + return -ENOMEM; > > + table = page_to_virt(page); > > + memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT)); > > + > > + /* > > + * The caller has to deal with the old ASCE, but here we make sure > > + * the new one is properly added to the list of page tables, so that > > + * it will be freed when the VM is torn down. > > + */ > > + spin_lock(&gmap->guest_table_lock); > > + list_add(&page->lru, &gmap->crst_list); > > + spin_unlock(&gmap->guest_table_lock); > > + > > + /* Set new table origin while preserving existing ASCE control bits */ > > + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); > > + WRITE_ONCE(gmap->asce, asce); > > + WRITE_ONCE(gmap->mm->context.gmap_asce, asce); > > + WRITE_ONCE(gmap->table, table); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(s390_replace_asce); >
On 14/04/2022 10.02, Claudio Imbrenda wrote: > Each secure guest must have a unique ASCE (address space control > element); we must avoid that new guests use the same page for their > ASCE, to avoid errors. > > Since the ASCE mostly consists of the address of the topmost page table > (plus some flags), we must not return that memory to the pool unless > the ASCE is no longer in use. > > Only a successful Destroy Secure Configuration UVC will make the ASCE > reusable again. > > If the Destroy Configuration UVC fails, the ASCE cannot be reused for a > secure guest (either for the ASCE or for other memory areas). To avoid > a collision, it must not be used again. This is a permanent error and > the page becomes in practice unusable, so we set it aside and leak it. > On failure we already leak other memory that belongs to the ultravisor > (i.e. the variable and base storage for a guest) and not leaking the > topmost page table was an oversight. > > This error (and thus the leakage) should not happen unless the hardware > is broken or KVM has some unknown serious bug. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > Fixes: 29b40f105ec8d55 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling") > --- > arch/s390/include/asm/gmap.h | 2 + > arch/s390/kvm/pv.c | 9 ++-- > arch/s390/mm/gmap.c | 80 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 88 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h > index 40264f60b0da..746e18bf8984 100644 > --- a/arch/s390/include/asm/gmap.h > +++ b/arch/s390/include/asm/gmap.h > @@ -148,4 +148,6 @@ void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4], > unsigned long gaddr, unsigned long vmaddr); > int gmap_mark_unmergeable(void); > void s390_reset_acc(struct mm_struct *mm); > +void s390_remove_old_asce(struct gmap *gmap); > +int s390_replace_asce(struct gmap *gmap); > #endif /* _ASM_S390_GMAP_H */ > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c > index 7f7c0d6af2ce..3c59ef763dde 100644 > --- a/arch/s390/kvm/pv.c > +++ b/arch/s390/kvm/pv.c > @@ -166,10 +166,13 @@ int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > atomic_set(&kvm->mm->context.is_protected, 0); > KVM_UV_EVENT(kvm, 3, "PROTVIRT DESTROY VM: rc %x rrc %x", *rc, *rrc); > WARN_ONCE(cc, "protvirt destroy vm failed rc %x rrc %x", *rc, *rrc); > - /* Inteded memory leak on "impossible" error */ > - if (!cc) > + /* Intended memory leak on "impossible" error */ > + if (!cc) { > kvm_s390_pv_dealloc_vm(kvm); > - return cc ? -EIO : 0; > + return 0; > + } > + s390_replace_asce(kvm->arch.gmap); > + return -EIO; > } > > int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index af03cacf34ec..e8904cb9dc38 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2714,3 +2714,83 @@ void s390_reset_acc(struct mm_struct *mm) > mmput(mm); > } > EXPORT_SYMBOL_GPL(s390_reset_acc); > + > +/** > + * s390_remove_old_asce - Remove the topmost level of page tables from the > + * list of page tables of the gmap. > + * @gmap the gmap whose table is to be removed > + * > + * This means that it will not be freed when the VM is torn down, and needs > + * to be handled separately by the caller, unless an intentional leak is > + * intended. Notice that this function will only remove the page from the "intentional leak is intended" ... sounds redundant. Scratch the first "intentional" ? > + * list, the page will still be used as a top level page table (and ASCE). > + */ > +void s390_remove_old_asce(struct gmap *gmap) > +{ > + struct page *old; > + > + old = virt_to_page(gmap->table); > + spin_lock(&gmap->guest_table_lock); > + list_del(&old->lru); > + /* > + * In case the ASCE needs to be "removed" multiple times, for example > + * if the VM is rebooted into secure mode several times > + * concurrently, or if s390_replace_asce fails after calling > + * s390_remove_old_asce and is attempted again later. In that case "In case the ASCE ... . In that case" - this should be either one big sentence, or better, scratch the first "In case" and use "Sometimes" or something similar to start the first sentence. > + * the old asce has been removed from the list, and therefore it > + * will not be freed when the VM terminates, but the ASCE is still > + * in use and still pointed to. > + * A subsequent call to replace_asce will follow the pointer and try > + * to remove the same page from the list again. > + * Therefore it's necessary that the page of the ASCE has valid > + * pointers, so list_del can work (and do nothing) without > + * dereferencing stale or invalid pointers. > + */ > + INIT_LIST_HEAD(&old->lru); > + spin_unlock(&gmap->guest_table_lock); > +} > +EXPORT_SYMBOL_GPL(s390_remove_old_asce); > + > +/** > + * s390_replace_asce - Try to replace the current ASCE of a gmap with > + * another equivalent one. > + * @gmap the gmap "the gmap" is not a very helpful description for a parmeter that is already called gmap. Write at least "the guest map that should be replaced" ? > + * > + * If the allocation of the new top level page table fails, the ASCE is not > + * replaced. > + * In any case, the old ASCE is always removed from the list. Therefore the > + * caller has to make sure to save a pointer to it beforehands, unless an s/beforehands/beforehand/ > + * intentional leak is intended. scratch "intentional". > + */ > +int s390_replace_asce(struct gmap *gmap) > +{ > + unsigned long asce; > + struct page *page; > + void *table; > + > + s390_remove_old_asce(gmap); > + > + page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER); > + if (!page) > + return -ENOMEM; > + table = page_to_virt(page); > + memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT)); > + > + /* > + * The caller has to deal with the old ASCE, but here we make sure > + * the new one is properly added to the list of page tables, so that > + * it will be freed when the VM is torn down. > + */ > + spin_lock(&gmap->guest_table_lock); > + list_add(&page->lru, &gmap->crst_list); > + spin_unlock(&gmap->guest_table_lock); > + > + /* Set new table origin while preserving existing ASCE control bits */ > + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); > + WRITE_ONCE(gmap->asce, asce); > + WRITE_ONCE(gmap->mm->context.gmap_asce, asce); > + WRITE_ONCE(gmap->table, table); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(s390_replace_asce); Thomas
On Thu, 5 May 2022 16:45:13 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 14/04/2022 10.02, Claudio Imbrenda wrote: > > Each secure guest must have a unique ASCE (address space control > > element); we must avoid that new guests use the same page for their > > ASCE, to avoid errors. > > > > Since the ASCE mostly consists of the address of the topmost page table > > (plus some flags), we must not return that memory to the pool unless > > the ASCE is no longer in use. > > > > Only a successful Destroy Secure Configuration UVC will make the ASCE > > reusable again. > > > > If the Destroy Configuration UVC fails, the ASCE cannot be reused for a > > secure guest (either for the ASCE or for other memory areas). To avoid > > a collision, it must not be used again. This is a permanent error and > > the page becomes in practice unusable, so we set it aside and leak it. > > On failure we already leak other memory that belongs to the ultravisor > > (i.e. the variable and base storage for a guest) and not leaking the > > topmost page table was an oversight. > > > > This error (and thus the leakage) should not happen unless the hardware > > is broken or KVM has some unknown serious bug. > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > Fixes: 29b40f105ec8d55 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling") > > --- > > arch/s390/include/asm/gmap.h | 2 + > > arch/s390/kvm/pv.c | 9 ++-- > > arch/s390/mm/gmap.c | 80 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 88 insertions(+), 3 deletions(-) > > > > diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h > > index 40264f60b0da..746e18bf8984 100644 > > --- a/arch/s390/include/asm/gmap.h > > +++ b/arch/s390/include/asm/gmap.h > > @@ -148,4 +148,6 @@ void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4], > > unsigned long gaddr, unsigned long vmaddr); > > int gmap_mark_unmergeable(void); > > void s390_reset_acc(struct mm_struct *mm); > > +void s390_remove_old_asce(struct gmap *gmap); > > +int s390_replace_asce(struct gmap *gmap); > > #endif /* _ASM_S390_GMAP_H */ > > diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c > > index 7f7c0d6af2ce..3c59ef763dde 100644 > > --- a/arch/s390/kvm/pv.c > > +++ b/arch/s390/kvm/pv.c > > @@ -166,10 +166,13 @@ int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > > atomic_set(&kvm->mm->context.is_protected, 0); > > KVM_UV_EVENT(kvm, 3, "PROTVIRT DESTROY VM: rc %x rrc %x", *rc, *rrc); > > WARN_ONCE(cc, "protvirt destroy vm failed rc %x rrc %x", *rc, *rrc); > > - /* Inteded memory leak on "impossible" error */ > > - if (!cc) > > + /* Intended memory leak on "impossible" error */ > > + if (!cc) { > > kvm_s390_pv_dealloc_vm(kvm); > > - return cc ? -EIO : 0; > > + return 0; > > + } > > + s390_replace_asce(kvm->arch.gmap); > > + return -EIO; > > } > > > > int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > > index af03cacf34ec..e8904cb9dc38 100644 > > --- a/arch/s390/mm/gmap.c > > +++ b/arch/s390/mm/gmap.c > > @@ -2714,3 +2714,83 @@ void s390_reset_acc(struct mm_struct *mm) > > mmput(mm); > > } > > EXPORT_SYMBOL_GPL(s390_reset_acc); > > + > > +/** > > + * s390_remove_old_asce - Remove the topmost level of page tables from the > > + * list of page tables of the gmap. > > + * @gmap the gmap whose table is to be removed > > + * > > + * This means that it will not be freed when the VM is torn down, and needs > > + * to be handled separately by the caller, unless an intentional leak is > > + * intended. Notice that this function will only remove the page from the > > "intentional leak is intended" ... sounds redundant. Scratch the first > "intentional" ? > > > + * list, the page will still be used as a top level page table (and ASCE). > > + */ > > +void s390_remove_old_asce(struct gmap *gmap) > > +{ > > + struct page *old; > > + > > + old = virt_to_page(gmap->table); > > + spin_lock(&gmap->guest_table_lock); > > + list_del(&old->lru); > > + /* > > + * In case the ASCE needs to be "removed" multiple times, for example > > + * if the VM is rebooted into secure mode several times > > + * concurrently, or if s390_replace_asce fails after calling > > + * s390_remove_old_asce and is attempted again later. In that case > > "In case the ASCE ... . In that case" - this should be either one big > sentence, or better, scratch the first "In case" and use "Sometimes" or > something similar to start the first sentence. > > > + * the old asce has been removed from the list, and therefore it > > + * will not be freed when the VM terminates, but the ASCE is still > > + * in use and still pointed to. > > + * A subsequent call to replace_asce will follow the pointer and try > > + * to remove the same page from the list again. > > + * Therefore it's necessary that the page of the ASCE has valid > > + * pointers, so list_del can work (and do nothing) without > > + * dereferencing stale or invalid pointers. > > + */ > > + INIT_LIST_HEAD(&old->lru); > > + spin_unlock(&gmap->guest_table_lock); > > +} > > +EXPORT_SYMBOL_GPL(s390_remove_old_asce); > > + > > +/** > > + * s390_replace_asce - Try to replace the current ASCE of a gmap with > > + * another equivalent one. > > + * @gmap the gmap > > "the gmap" is not a very helpful description for a parmeter that is already > called gmap. Write at least "the guest map that should be replaced" ? > > > + * > > + * If the allocation of the new top level page table fails, the ASCE is not > > + * replaced. > > + * In any case, the old ASCE is always removed from the list. Therefore the > > + * caller has to make sure to save a pointer to it beforehands, unless an > > s/beforehands/beforehand/ > > > + * intentional leak is intended. > > scratch "intentional". > > > + */ > > +int s390_replace_asce(struct gmap *gmap) > > +{ > > + unsigned long asce; > > + struct page *page; > > + void *table; > > + > > + s390_remove_old_asce(gmap); > > + > > + page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER); > > + if (!page) > > + return -ENOMEM; > > + table = page_to_virt(page); > > + memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT)); > > + > > + /* > > + * The caller has to deal with the old ASCE, but here we make sure > > + * the new one is properly added to the list of page tables, so that > > + * it will be freed when the VM is torn down. > > + */ > > + spin_lock(&gmap->guest_table_lock); > > + list_add(&page->lru, &gmap->crst_list); > > + spin_unlock(&gmap->guest_table_lock); > > + > > + /* Set new table origin while preserving existing ASCE control bits */ > > + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); > > + WRITE_ONCE(gmap->asce, asce); > > + WRITE_ONCE(gmap->mm->context.gmap_asce, asce); > > + WRITE_ONCE(gmap->table, table); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(s390_replace_asce); > > Thomas > thanks, I will fix them all
On Thu, 2022-04-14 at 10:02 +0200, Claudio Imbrenda wrote: [...] > +/** > + * s390_replace_asce - Try to replace the current ASCE of a gmap > with > + * another equivalent one. > + * @gmap the gmap > + * > + * If the allocation of the new top level page table fails, the ASCE > is not > + * replaced. > + * In any case, the old ASCE is always removed from the list. > Therefore the > + * caller has to make sure to save a pointer to it beforehands, > unless an > + * intentional leak is intended. > + */ > +int s390_replace_asce(struct gmap *gmap) > +{ > [...] > + /* Set new table origin while preserving existing ASCE > control bits */ > + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); > + WRITE_ONCE(gmap->asce, asce); Can someone concurrently touch the control bits?
On Mon, 16 May 2022 09:22:09 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > On Thu, 2022-04-14 at 10:02 +0200, Claudio Imbrenda wrote: > > [...] > > +/** > > + * s390_replace_asce - Try to replace the current ASCE of a gmap > > with > > + * another equivalent one. > > + * @gmap the gmap > > + * > > + * If the allocation of the new top level page table fails, the ASCE > > is not > > + * replaced. > > + * In any case, the old ASCE is always removed from the list. > > Therefore the > > + * caller has to make sure to save a pointer to it beforehands, > > unless an > > + * intentional leak is intended. > > + */ > > +int s390_replace_asce(struct gmap *gmap) > > +{ > > > [...] > > + /* Set new table origin while preserving existing ASCE > > control bits */ > > + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); > > + WRITE_ONCE(gmap->asce, asce); > > Can someone concurrently touch the control bits? should not happen, nobody is using the ASCE while we replace it moreover, all callers of s390_replace_asce hold kvm->lock, as do all functions changing the ASCE
diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h index 40264f60b0da..746e18bf8984 100644 --- a/arch/s390/include/asm/gmap.h +++ b/arch/s390/include/asm/gmap.h @@ -148,4 +148,6 @@ void gmap_sync_dirty_log_pmd(struct gmap *gmap, unsigned long dirty_bitmap[4], unsigned long gaddr, unsigned long vmaddr); int gmap_mark_unmergeable(void); void s390_reset_acc(struct mm_struct *mm); +void s390_remove_old_asce(struct gmap *gmap); +int s390_replace_asce(struct gmap *gmap); #endif /* _ASM_S390_GMAP_H */ diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c index 7f7c0d6af2ce..3c59ef763dde 100644 --- a/arch/s390/kvm/pv.c +++ b/arch/s390/kvm/pv.c @@ -166,10 +166,13 @@ int kvm_s390_pv_deinit_vm(struct kvm *kvm, u16 *rc, u16 *rrc) atomic_set(&kvm->mm->context.is_protected, 0); KVM_UV_EVENT(kvm, 3, "PROTVIRT DESTROY VM: rc %x rrc %x", *rc, *rrc); WARN_ONCE(cc, "protvirt destroy vm failed rc %x rrc %x", *rc, *rrc); - /* Inteded memory leak on "impossible" error */ - if (!cc) + /* Intended memory leak on "impossible" error */ + if (!cc) { kvm_s390_pv_dealloc_vm(kvm); - return cc ? -EIO : 0; + return 0; + } + s390_replace_asce(kvm->arch.gmap); + return -EIO; } int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index af03cacf34ec..e8904cb9dc38 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -2714,3 +2714,83 @@ void s390_reset_acc(struct mm_struct *mm) mmput(mm); } EXPORT_SYMBOL_GPL(s390_reset_acc); + +/** + * s390_remove_old_asce - Remove the topmost level of page tables from the + * list of page tables of the gmap. + * @gmap the gmap whose table is to be removed + * + * This means that it will not be freed when the VM is torn down, and needs + * to be handled separately by the caller, unless an intentional leak is + * intended. Notice that this function will only remove the page from the + * list, the page will still be used as a top level page table (and ASCE). + */ +void s390_remove_old_asce(struct gmap *gmap) +{ + struct page *old; + + old = virt_to_page(gmap->table); + spin_lock(&gmap->guest_table_lock); + list_del(&old->lru); + /* + * In case the ASCE needs to be "removed" multiple times, for example + * if the VM is rebooted into secure mode several times + * concurrently, or if s390_replace_asce fails after calling + * s390_remove_old_asce and is attempted again later. In that case + * the old asce has been removed from the list, and therefore it + * will not be freed when the VM terminates, but the ASCE is still + * in use and still pointed to. + * A subsequent call to replace_asce will follow the pointer and try + * to remove the same page from the list again. + * Therefore it's necessary that the page of the ASCE has valid + * pointers, so list_del can work (and do nothing) without + * dereferencing stale or invalid pointers. + */ + INIT_LIST_HEAD(&old->lru); + spin_unlock(&gmap->guest_table_lock); +} +EXPORT_SYMBOL_GPL(s390_remove_old_asce); + +/** + * s390_replace_asce - Try to replace the current ASCE of a gmap with + * another equivalent one. + * @gmap the gmap + * + * If the allocation of the new top level page table fails, the ASCE is not + * replaced. + * In any case, the old ASCE is always removed from the list. Therefore the + * caller has to make sure to save a pointer to it beforehands, unless an + * intentional leak is intended. + */ +int s390_replace_asce(struct gmap *gmap) +{ + unsigned long asce; + struct page *page; + void *table; + + s390_remove_old_asce(gmap); + + page = alloc_pages(GFP_KERNEL_ACCOUNT, CRST_ALLOC_ORDER); + if (!page) + return -ENOMEM; + table = page_to_virt(page); + memcpy(table, gmap->table, 1UL << (CRST_ALLOC_ORDER + PAGE_SHIFT)); + + /* + * The caller has to deal with the old ASCE, but here we make sure + * the new one is properly added to the list of page tables, so that + * it will be freed when the VM is torn down. + */ + spin_lock(&gmap->guest_table_lock); + list_add(&page->lru, &gmap->crst_list); + spin_unlock(&gmap->guest_table_lock); + + /* Set new table origin while preserving existing ASCE control bits */ + asce = (gmap->asce & ~_ASCE_ORIGIN) | __pa(table); + WRITE_ONCE(gmap->asce, asce); + WRITE_ONCE(gmap->mm->context.gmap_asce, asce); + WRITE_ONCE(gmap->table, table); + + return 0; +} +EXPORT_SYMBOL_GPL(s390_replace_asce);
Each secure guest must have a unique ASCE (address space control element); we must avoid that new guests use the same page for their ASCE, to avoid errors. Since the ASCE mostly consists of the address of the topmost page table (plus some flags), we must not return that memory to the pool unless the ASCE is no longer in use. Only a successful Destroy Secure Configuration UVC will make the ASCE reusable again. If the Destroy Configuration UVC fails, the ASCE cannot be reused for a secure guest (either for the ASCE or for other memory areas). To avoid a collision, it must not be used again. This is a permanent error and the page becomes in practice unusable, so we set it aside and leak it. On failure we already leak other memory that belongs to the ultravisor (i.e. the variable and base storage for a guest) and not leaking the topmost page table was an oversight. This error (and thus the leakage) should not happen unless the hardware is broken or KVM has some unknown serious bug. Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> Fixes: 29b40f105ec8d55 ("KVM: s390: protvirt: Add initial vm and cpu lifecycle handling") --- arch/s390/include/asm/gmap.h | 2 + arch/s390/kvm/pv.c | 9 ++-- arch/s390/mm/gmap.c | 80 ++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 3 deletions(-)