Re: [Xen-devel] [PATCH v5 14/18] x86/mem_sharing: use default_access in add_to_physmap

[Tamas K Lengyel <tamas@tklengyel.com>]

On Thu, Jan 23, 2020 at 9:44 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 1/22/20 5:08 PM, Tamas K Lengyel wrote:
> > On Wed, Jan 22, 2020 at 8:35 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 21.01.2020 18:49, Tamas K Lengyel wrote:
> >>> When plugging a hole in the target physmap don't use the access permission
> >>> returned by __get_gfn_type_access as it can be non-sensical,
> >>
> >> "can be" is too vague for my taste - it suggests there may also be cases
> >> where a sensible value is returned, and hence it should be used. Could
> >> you clarify this please? (The code change itself of course is simple and
> >> mechanical enough to look okay.)
> >
> > Well, I can only speak of what I observed. The case seems to be that
> > most of the time the function actually returns p2m_access_rwx (which
> > is sensible), but occasionally something else. I didn't investigate
> > where that value actually comes from, but when populating a physmap
> > like this only the default_access is sane.
>
> It would be good to get to the bottom of this.  Is it possible that your
> dom0 agent (or whatever it's called) is calling add_to_physmap() on gfns
> that have already been populated?  Is that something you want to catch?
>

OK, I went back and deciphered why sometimes I saw different permissions here.

In the context I ran into this issue there is no dom0 agent calling
add_to_physmap. We wind up in this path with a VM fork where the MMU
faulted. The fault handler is trying to determine whether the page
needs to be forked from its parent: populated with a shared entry if
it's R/X access, or deduplicated if it's a W. This forking only
actually happens if the page type that is returned by ept_get_entry is
of a hole type. When it's a hole type, ept_get_entry always returns
p2m_access_n as the access permission. Copying that access permission
to the newly populated entry is bad - that's what this patch fixes.

But this path also gets hit when the MMU faults for other reasons. In
those cases will get permissions other then p2m_access_n since the
type is not a hole. But when it's not a hole, this function bails as
that's a clear signal that the page doesn't need forking. So I was
seeing p2m_access_rwx permission for page accesses that triggered the
MMU fault for reasons other then mem_access. For example, when a
previously shared entry needs unsharing.

So there is no mystery after all, I was just printing my debug lines
with the mem_access permissions irrespective of the page type before
the path bails due to the type check.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel