On 31/03/2021 14:49, Roger Pau Monné wrote:
> 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).
Urgh - very good points.
Do we perhaps want to take the opportunity to make these functions
tolerate NULL, to simplify all cleanup code across the hypervisor?
~Andrew
