Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Apr 5, 2019 at 1:36 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 04.04.19 at 16:54, <George.Dunlap@xxxxxxxxxx> wrote:
> >> On Apr 4, 2019, at 3:36 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>>>> On 04.04.19 at 15:09, <tamas@xxxxxxxxxxxxx> 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.

There are checks in place for that. Take a look at the nominate_page
function in mm/mem_sharing.c.

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

It's not ideal for sure. Both that and the resetting of all altp2m
views completely when the hostp2m changes are troubling behaviors that
limit when altp2m can be used. It's up for debate though how hostp2m
changes should be handled, and handled safely. I think the current
implementation just tabled those hard questions by resetting
everything.

Tamas

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