Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2023 06:07, Tamas K Lengyel wrote:
> An assert failure has been observed at p2m-basic.c:173 when performing vm

Please can you at least also name the function here? The line number is
going to change, and when coming back to this change later, it'll be
more troublesome to figure out which precise assertion was meant. Yes,
...

> forking and then destroying the forked VM. The assert checks whether the
> domain's shared pages counter is 0.

... you verbally describe it here, but still.

> According to the patch that originally
> added the assert (7bedbbb5c31) the p2m_teardown should only happen after
> mem_sharing already relinquished all shared pages.
> 
> In this patch we flip the order in which relinquish ops are called to avoid
> tripping the assert.
> 
> Signed-off-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> ---
> Note: it is unclear why this assert hasn't tripped in the past. It hasn't
> been observed with 4.17-rc1 but it is in RELEASE-4.17.0

That's almost certainly a result of e7aa55c0aab3 ("x86/p2m: free the paging
memory pool preemptively"), which added calls to p2m_teardown() to
hap_teardown(). If you agree, this wants recording in a Fixes: tag, as
- being an XSA fix - that change has been backported to everywhere, and
hence the issue now also exists everywhere.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -2310,6 +2310,32 @@ int domain_relinquish_resources(struct domain *d)
>          if ( ret )
>              return ret;
>  
> +#ifdef CONFIG_MEM_SHARING
> +    PROGRESS(shared):

If we go with the re-ordering as you suggest, then please also move the
enumerator near the top of the switch() body.

Did you consider the alternative of adjusting the ASSERT() in question
instead? It could reasonably become

#ifdef CONFIG_MEM_SHARING
    ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || !atomic_read(&d->shr_pages));
#endif

now, I think. That would be less intrusive a change (helpful for
backporting), but there may be other (so far unnamed) benefits of pulling
ahead the shared memory teardown.

> +        if ( is_hvm_domain(d) )
> +        {
> +            /* If the domain has shared pages, relinquish them allowing
> +             * for preemption. */

Similarly, if the code is to be moved, please correct style here at this
occasion.

> +            ret = relinquish_shared_pages(d);
> +            if ( ret )
> +                return ret;

While I can easily agree with the movement ahead of this being okay, ...

> +            /*
> +             * If the domain is forked, decrement the parent's pause count
> +             * and release the domain.
> +             */
> +            if ( mem_sharing_is_fork(d) )
> +            {
> +                struct domain *parent = d->parent;
> +
> +                d->parent = NULL;
> +                domain_unpause(parent);
> +                put_domain(parent);
> +            }

... I can only trust you on being sure that moving ahead of this is
okay, too.

Jan