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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Sure.

>
> > 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.

Ough.. In that case we'll need this patch backported too. Will add the Fixes: tag.

>
> > --- 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.

Sure.

>
> 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.

I have a hard time understanding this proposed ASSERT.

>
> > +        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.

Sure.

>
> > +            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.

That's fine, we are in the teardown of the fork so it doesn't matter where you are releasing the parent as long as its after the fork is unlinked from it.

Thanks,

Tamas