Re: Fwd: Question regarding the behavior of guest_physmap_remove_page on x86

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/11/15 18:09, Julien Grall wrote:
> (Resending with this time xen-devel in CC)
>
> Hi,
>
> On ARM, it's possible to fail when removing a page from the P2M. It's
> happening if we are trying to shatter a superpage and we don't have
> memory to allocate the table. Therefore the mapping won't be removed
> from the P2M.
>
> However on ARM (and until recently on x86 [1]), the function
> guest_physmap_remove_page is not supposed to return an error. So we
> would free the page even if we fail to remove the page. This will end up
> to re-use the page by someone else even though the mapping is still
> present in the P2M.
>
> I looked to the x86 version and I'm not sure how the function is
> behaving. Maybe an x86 maintainers could give me insight here.
>
> I'm thinking to fix the problem by checking the return of
> guest_physmap_remove_page to avoid the page being reallocate to someone
> else (see for instance guest_remove_page in xen/common/memory.c). Is it
> a sensible way to fix it?

x86 can just as easily fail because of a failure to shatter a superpage.

Despite the below changeset, none of the callee's were updated to
actually act upon the error.

As a result, the same issue affects x86, in principle.

Does ARM have a shadow pool?  On x86, we arrange that the shadow pool
(should be) large enough so that we never actually encounter an
out-of-memory when shattering a superpage.

I also observe that there is a latent bug with iommu_unmap_page() (which
is part of guest_physmap_remove_page()) as (almost) nothing checks its
return value.  Currently all (x86) callpaths either return success, or
crash the domain.

Looking at other codepaths, other possible errors (other than -ENOMEM
from shattering) are:

    if ( unlikely(p2m_is_foreign(p2mt)) )
    {
        /* pvh fixme: foreign types are only supported on ept at present */
        gdprintk(XENLOG_WARNING, "Unimplemented foreign p2m type.\n");
        return -EINVAL;
    }

or:

    if ( !(p2m_entry = p2m_find_entry(*table, gfn_remainder, gfn,
                                      shift, max)) )
        return -ENOENT;


All this code looks quite rotten through, and is in some serious need of
some error handling hygiene.

I would not be surprised if some correctness issues are lurking in here.

~Andrew