On Wed, Mar 31, 2021 at 02:31:25PM +0100, Andrew Cooper wrote:
> vlapic_init()'s caller calls vlapic_destroy() on error. Therefore, the error
> path from __map_domain_page_global() failing would doubly free
> vlapic->regs_page.
>
> Rework vlapic_destroy() to be properly idempotent, introducing the necessary
> UNMAP_DOMAIN_PAGE_GLOBAL() and FREE_DOMHEAP_PAGE() wrappers.
>
> Rearrange vlapic_init() to group all trivial initialisation, and leave all
> cleanup to the caller, in line with our longer term plans.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> ---
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Roger Pau Monné <roger....@citrix.com>
> CC: Wei Liu <w...@xen.org>
> ---
> xen/arch/x86/hvm/vlapic.c | 11 ++++-------
> xen/include/xen/domain_page.h | 5 +++++
> xen/include/xen/mm.h | 6 ++++++
> 3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 5e21fb4937..da030f41b5 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1608,7 +1608,9 @@ int vlapic_init(struct vcpu *v)
> return 0;
> }
>
> + /* Trivial init. */
> vlapic->pt.source = PTSRC_lapic;
> + spin_lock_init(&vlapic->esr_lock);
>
> vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
> if ( !vlapic->regs_page )
> @@ -1616,17 +1618,12 @@ int vlapic_init(struct vcpu *v)
>
> vlapic->regs = __map_domain_page_global(vlapic->regs_page);
> if ( vlapic->regs == NULL )
> - {
> - free_domheap_page(vlapic->regs_page);
> return -ENOMEM;
> - }
>
> clear_page(vlapic->regs);
>
> vlapic_reset(vlapic);
>
> - spin_lock_init(&vlapic->esr_lock);
> -
> tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
>
> if ( v->vcpu_id == 0 )
> @@ -1645,8 +1642,8 @@ void vlapic_destroy(struct vcpu *v)
> tasklet_kill(&vlapic->init_sipi.tasklet);
> TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
> destroy_periodic_time(&vlapic->pt);
> - unmap_domain_page_global(vlapic->regs);
> - free_domheap_page(vlapic->regs_page);
> + UNMAP_DOMAIN_PAGE_GLOBAL(vlapic->regs);
I think you need to check whether vlapic->regs_page is NULL here...
> + FREE_DOMHEAP_PAGE(vlapic->regs_page);
> }
>
> /*
> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> index a182d33b67..0cb7f2aad3 100644
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -77,4 +77,9 @@ static inline void unmap_domain_page_global(const void *va)
> {};
> (p) = NULL; \
> } while ( false )
>
> +#define UNMAP_DOMAIN_PAGE_GLOBAL(p) do { \
> + unmap_domain_page_global(p); \
> + (p) = NULL; \
> +} while ( false )
> +
> #endif /* __XEN_DOMAIN_PAGE_H__ */
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 667f9dac83..c274e2eac4 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
> } while ( false )
> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>
> +#define FREE_DOMHEAP_PAGES(p, o) do { \
> + free_domheap_pages(p, o); \
...as both unmap_domain_page_global and free_domheap_pages don't
support being passed a NULL pointer.
Passing such NULL pointer will result in unmap_domain_page_global
hitting an assert AFAICT, and free_domheap_pages will try to use
page_get_owner(NULL).
Thanks, Roger.
