Re: [PATCH v1] Fix p2m_set_suppress_ve

["Jan Beulich" <JBeulich@suse.com>]

>>> On 04.04.19 at 16:54, <George.Dunlap@citrix.com> wrote:
>> On Apr 4, 2019, at 3:36 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 04.04.19 at 15:09, <tamas@tklengyel.com> wrote:
>>> I agree that it is confusing. It would be fine to UNSHARE here as well
>>> to keep things consistent but otherwise it's not really an issue as
>>> the entry type is checked later to ensure that this is a p2m_ram_rw
>>> entry. We are simply trying to keep mem_sharing and _modified_ altp2m
>>> entries exclusive. So it is fine to have mem_shared entries in the
>>> hostp2m and have those entries be copied into altp2m tables lazily,
>>> but for altp2m entries that have changed mem_access permissions or are
>>> remapped we want the entries in the hostp2m to be of regular type.
>>> This is not necessarily a technical requirement, it's mostly just to
>>> reduce complexity. So it would be fine to add UNSHARE here as well, I
>>> guess the only reason why I haven't done that is because I already
>>> trigger the unshare and copy-to-altp2m before remapping by setting
>>> dummy mem_access permission on the entries.
>> 
>> I'm afraid I don't agree with this justification: mem-sharing is about
>> contents of pages, whereas altp2m is about meta data (permissions
>> etc). I don't see why one would want to unshare because of a meta
>> data adjustment other than a page becoming non-CoW-writable.
>> Eagerly un-sharing in the end undermines the intentions of sharing.
> 
> Remember also that altp2ms allow someone to set not just alternate views 
> with different permissions, but also alternate views with different backing 
> mfns.  Combining shared mfns with alternate views with different mfns on the 
> same gfn means that you have to be very careful not to end up giving write 
> permission to the shared page, which would be a security issue.  Unsharing 
> when creating an altp2m entry means that any given gfn is *either* shared 
> *or* duplicated across altp2ms, but not both; this simplifies the reasoning.

Hmm, yes, I can see how this gets complicated. But is this behavior
symmetric? I.e. will attempts to share a GFN fail when it has a non-
default setting in one of the alternate views? Looking at the code I
can't seem to recognize such behavior.

Furthermore I'm puzzled by p2m_altp2m_propagate_change()
apparently blindly overwriting (almost) everything. Is it really
intended in almost cases (there looks to be an exception when
the old entry holds INVALID_MFN; I wonder though whether its
condition isn't inverted) to discard special access and/or MFNs in
alternate views when the host p2m's respective slot changes?

Looking at the function I also wonder whether it doesn't
pointlessly call p2m_reset_altp2m() when old and new entry
both hold INVALID_MFN.

Jan



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