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(-)
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.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@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);
+ 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); \
+ (p) = NULL; \
+} while ( false )
+#define FREE_DOMHEAP_PAGE(p) FREE_DOMHEAP_PAGES(p, 0)
+
/* Map machine page range in Xen virtual address space. */
int map_pages_to_xen(
unsigned long virt,
--
2.11.0
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.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@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.
On 31.03.2021 15:49, Roger Pau Monné wrote:
> On Wed, Mar 31, 2021 at 02:31:25PM +0100, Andrew Cooper wrote:
>> @@ -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.
Except that such checking would better go into the new macros,
alongside their clearing the pointers afterwards.
Jan
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.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@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
On 31.03.2021 15:31, 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.
I'm having difficulty seeing this. What I find at present is
rc = vlapic_init(v);
if ( rc != 0 ) /* teardown: vlapic_destroy */
goto fail2;
and then
fail3:
vlapic_destroy(v);
fail2:
Am I missing some important aspect?
> 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.
Cleanup functions becoming idempotent is what I understand is the
longer term plan. I didn't think this necessarily included leaving
cleanup after failure in a function to it caller(s). At least in the
general case I think it would be quite a bit better if functions
cleaned up after themselves - perhaps (using the example here) by
vlapic_init() calling vlapic_destroy() in such a case.
Jan
On 31/03/2021 15:03, Jan Beulich wrote:
> On 31.03.2021 15:31, 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.
> I'm having difficulty seeing this. What I find at present is
>
> rc = vlapic_init(v);
> if ( rc != 0 ) /* teardown: vlapic_destroy */
> goto fail2;
>
> and then
>
> fail3:
> vlapic_destroy(v);
> fail2:
>
> Am I missing some important aspect?
No - I'm blind. (although I do blame the code comment for being
actively misleading.)
I retract the patch at this point. It needs to be part of a bigger
series making changes like this consistently across the callers.
>> 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.
> Cleanup functions becoming idempotent is what I understand is the
> longer term plan. I didn't think this necessarily included leaving
> cleanup after failure in a function to it caller(s).
That's literally the point of the exercise.
> At least in the
> general case I think it would be quite a bit better if functions
> cleaned up after themselves - perhaps (using the example here) by
> vlapic_init() calling vlapic_destroy() in such a case.
That pattern is the cause of code duplication (not a problem per say),
and many bugs (the problem needing solving) caused by the duplicated
logic not being equivalent.
We've got the start of the top-level pattern in progress, with
{domain,vcpu}_create() calling {d,v}_teardown() then {d,v}_destroy() for
errors.
This top-level pattern is also necessary so we can remove the need to
put partially constructed objects into full view of the system, and wait
for a subsequent hypercall to clean them up. This series of bugs is
still active, and there are still a load of NULL pointer deferences
reachable from out-of-order toolstack hypercalls in failure cases.
We've also got some subsystems (vmtrace) moved into the new pattern, and
some minor parts of existing mess moved over too, but the end goal is to
have exactly one create() path and exactly one cleanup path, so its
impossible to introduce mismatched duplicate cleanup logic.
~Andrew
On 01.04.2021 15:20, Andrew Cooper wrote:
> On 31/03/2021 15:03, Jan Beulich wrote:
>> On 31.03.2021 15:31, 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.
>> I'm having difficulty seeing this. What I find at present is
>>
>> rc = vlapic_init(v);
>> if ( rc != 0 ) /* teardown: vlapic_destroy */
>> goto fail2;
>>
>> and then
>>
>> fail3:
>> vlapic_destroy(v);
>> fail2:
>>
>> Am I missing some important aspect?
>
> No - I'm blind. (although I do blame the code comment for being
> actively misleading.)
>
> I retract the patch at this point. It needs to be part of a bigger
> series making changes like this consistently across the callers.
>
>>> 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.
>> Cleanup functions becoming idempotent is what I understand is the
>> longer term plan. I didn't think this necessarily included leaving
>> cleanup after failure in a function to it caller(s).
>
> That's literally the point of the exercise.
>
>> At least in the
>> general case I think it would be quite a bit better if functions
>> cleaned up after themselves - perhaps (using the example here) by
>> vlapic_init() calling vlapic_destroy() in such a case.
>
> That pattern is the cause of code duplication (not a problem per say),
> and many bugs (the problem needing solving) caused by the duplicated
> logic not being equivalent.
>
> We've got the start of the top-level pattern in progress, with
> {domain,vcpu}_create() calling {d,v}_teardown() then {d,v}_destroy() for
> errors.
Hmm, in which case you mean to shift the responsibility not to "the
caller" (many instances) but "the top level caller" (a single
instance)?
Jan
On 01/04/2021 14:45, Jan Beulich wrote:
> On 01.04.2021 15:20, Andrew Cooper wrote:
>> On 31/03/2021 15:03, Jan Beulich wrote:
>>> On 31.03.2021 15:31, 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.
>>> I'm having difficulty seeing this. What I find at present is
>>>
>>> rc = vlapic_init(v);
>>> if ( rc != 0 ) /* teardown: vlapic_destroy */
>>> goto fail2;
>>>
>>> and then
>>>
>>> fail3:
>>> vlapic_destroy(v);
>>> fail2:
>>>
>>> Am I missing some important aspect?
>> No - I'm blind. (although I do blame the code comment for being
>> actively misleading.)
>>
>> I retract the patch at this point. It needs to be part of a bigger
>> series making changes like this consistently across the callers.
>>
>>>> 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.
>>> Cleanup functions becoming idempotent is what I understand is the
>>> longer term plan. I didn't think this necessarily included leaving
>>> cleanup after failure in a function to it caller(s).
>> That's literally the point of the exercise.
>>
>>> At least in the
>>> general case I think it would be quite a bit better if functions
>>> cleaned up after themselves - perhaps (using the example here) by
>>> vlapic_init() calling vlapic_destroy() in such a case.
>> That pattern is the cause of code duplication (not a problem per say),
>> and many bugs (the problem needing solving) caused by the duplicated
>> logic not being equivalent.
>>
>> We've got the start of the top-level pattern in progress, with
>> {domain,vcpu}_create() calling {d,v}_teardown() then {d,v}_destroy() for
>> errors.
> Hmm, in which case you mean to shift the responsibility not to "the
> caller" (many instances) but "the top level caller" (a single
> instance)?
Yes, but the route to managing that needs to move the responsibility up
one level at a time for us not to break things.
i.e. we'd next want to make {pv,hvm}_*_create/initialise() call the
appropriate {pv,hvm}_*_destroy() covering the entire sub-call-tree.
Moving the responsibility across the arch_{d,v}_*() boundaries in
particular needs careful coordination.
~Andrew
© 2016 - 2024 Red Hat, Inc.