* [PATCH v1] Fix p2m_set_suppress_ve
@ 2019-04-03 14:29 Alexandru Stefan ISAILA
2019-04-03 14:58 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Alexandru Stefan ISAILA @ 2019-04-03 14:29 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, george.dunlap, andrew.cooper3, jbeulich,
Alexandru Stefan ISAILA, roger.pau
On a new altp2m view the p2m_set_suppress_ve() func will fail with invalid mfn
from p2m->get_entry() if the p2m->set_entry() was not called before.
This patch solves the problem by getting the mfn from __get_gfn_type_access()
and then returning error if the mfn is invalid.
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
xen/arch/x86/mm/p2m.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9bbb8f485..3e6e5ef152 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
if ( !mfn_valid(mfn) )
{
- rc = -ESRCH;
- goto out;
+ unsigned int page_order;
+
+ mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
+ P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
+
+ if ( !mfn_valid(mfn) )
+ {
+ rc = -ESRCH;
+ goto out;
+ }
}
rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, t, a, suppress_ve);
--
2.17.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 14:29 [PATCH v1] Fix p2m_set_suppress_ve Alexandru Stefan ISAILA
@ 2019-04-03 14:58 ` Jan Beulich
2019-04-03 15:17 ` Razvan Cojocaru
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2019-04-03 14:58 UTC (permalink / raw)
To: aisaila, xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> if ( !mfn_valid(mfn) )
> {
> - rc = -ESRCH;
> - goto out;
> + unsigned int page_order;
> +
> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
at least P2M_UNSHARE is too heavy: Why would you want to force
un-sharing of a page when all you want to alter is #VE behavior?
A recent patch[1] of mine actually tries to move the other direction.
Additionally, when you add such a lookup as error handling attempt,
I think it is important to leave a code comment. But I wonder
whether this shouldn't be done before the call to ->get_entry(), or
whether in fact there's a bug here in how get_entry() behaves in
this case.
The description also doesn't clarify at all why you (need to?) use
host_p2m here, when get_entry() and set_entry() both use p2m.
I suppose that's because there's some sync-ing happening
between the p2m-s, but at least to me this isn't an obviously
necessary (side) effect of that call.
Also note that if you already need to call this lowest level function
directly here, the last argument should be "false".
Jan
[1] https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg00847.html
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 14:58 ` Jan Beulich
@ 2019-04-03 15:17 ` Razvan Cojocaru
2019-04-03 15:30 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Razvan Cojocaru @ 2019-04-03 15:17 UTC (permalink / raw)
To: Jan Beulich, aisaila, xen-devel
Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>> if ( !mfn_valid(mfn) )
>> {
>> - rc = -ESRCH;
>> - goto out;
>> + unsigned int page_order;
>> +
>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>
> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
> at least P2M_UNSHARE is too heavy: Why would you want to force
> un-sharing of a page when all you want to alter is #VE behavior?
That logic was taken from p2m_set_altp2m_mem_access(), we thought the
two cases are very similar.
269 mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
270
271 /* Check host p2m if no valid entry in alternate */
272 if ( !mfn_valid(mfn) )
273 {
274
275 mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
276 P2M_ALLOC | P2M_UNSHARE,
&page_order, 0);
277
278 rc = -ESRCH;
279 if ( !mfn_valid(mfn) || t != p2m_ram_rw )
280 return rc;
281
282 /* If this is a superpage, copy that first */
283 if ( page_order != PAGE_ORDER_4K )
284 {
285 unsigned long mask = ~((1UL << page_order) - 1);
286 gfn_t gfn2 = _gfn(gfn_l & mask);
287 mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
288
289 rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t,
old_a, 1);
290 if ( rc )
291 return rc;
292 }
293 }
294
295 /*
296 * Inherit the old suppress #VE bit value if it is already set,
or set it
297 * to 1 otherwise
298 */
299 return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
300 }
I wonder if we should put the whole logic in the "if ( !mfn_valid(mfn)
)" body in its own function and reuse that for both functions - although
it doesn't look like the extra superpage logic matters for setting the
suppress #VE bit alone (since even the code above only sets it with
PAGE_ORDER_4K).
> Additionally, when you add such a lookup as error handling attempt,
> I think it is important to leave a code comment. But I wonder
> whether this shouldn't be done before the call to ->get_entry(), or
> whether in fact there's a bug here in how get_entry() behaves in
> this case.
Changes to the hostp2m (also known as altp2m view 0) propagate to all
existing altp2ms, but they do so in a lazy manner, and also that won't
happen for altp2ms created after a while. So altp2ms will not
necessarily know about a page that the hostp2m knows about, which should
not stop us from setting mem access restrictions or the value of the SVE
bit.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 15:17 ` Razvan Cojocaru
@ 2019-04-03 15:30 ` Jan Beulich
2019-04-03 15:48 ` Razvan Cojocaru
2019-04-04 12:46 ` Razvan Cojocaru
0 siblings, 2 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-03 15:30 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
> On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>>> if ( !mfn_valid(mfn) )
>>> {
>>> - rc = -ESRCH;
>>> - goto out;
>>> + unsigned int page_order;
>>> +
>>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>>
>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
>> at least P2M_UNSHARE is too heavy: Why would you want to force
>> un-sharing of a page when all you want to alter is #VE behavior?
>
> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
> two cases are very similar.
I see.
> 269 mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> 270
> 271 /* Check host p2m if no valid entry in alternate */
This comment was not cloned.
> 272 if ( !mfn_valid(mfn) )
> 273 {
> 274
> 275 mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
> 276 P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> 277
> 278 rc = -ESRCH;
> 279 if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> 280 return rc;
> 281
> 282 /* If this is a superpage, copy that first */
> 283 if ( page_order != PAGE_ORDER_4K )
> 284 {
> 285 unsigned long mask = ~((1UL << page_order) - 1);
> 286 gfn_t gfn2 = _gfn(gfn_l & mask);
> 287 mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> 288
> 289 rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
> 290 if ( rc )
> 291 return rc;
> 292 }
> 293 }
> 294
> 295 /*
> 296 * Inherit the old suppress #VE bit value if it is already set, or set it
> 297 * to 1 otherwise
> 298 */
> 299 return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
> 300 }
>
> I wonder if we should put the whole logic in the "if ( !mfn_valid(mfn) )"
> body in its own function and reuse that for both functions - although
> it doesn't look like the extra superpage logic matters for setting the
> suppress #VE bit alone (since even the code above only sets it with
> PAGE_ORDER_4K).
Indeed, this latter aspect doesn't make it look very attractive to
introduce a common helper. Otoh I wonder what other functions
might be affected.
>> Additionally, when you add such a lookup as error handling attempt,
>> I think it is important to leave a code comment. But I wonder
>> whether this shouldn't be done before the call to ->get_entry(), or
>> whether in fact there's a bug here in how get_entry() behaves in
>> this case.
>
> Changes to the hostp2m (also known as altp2m view 0) propagate to all
> existing altp2ms, but they do so in a lazy manner, and also that won't
> happen for altp2ms created after a while. So altp2ms will not
> necessarily know about a page that the hostp2m knows about, which should
> not stop us from setting mem access restrictions or the value of the SVE
> bit.
But even if the propagation is lazy, a "get" should then return the
propagated value, shouldn't it? Or else callers (like is the case here)
can be mislead. I do recognize that there may be callers who
intentionally _don't_ want the propagated value, but I'd expect this
to be the exception, not the rule. I wonder therefore whether there
shouldn't be a wrapper around ->get_entry() to take care of this for
callers which want the propagated value. Calling the bare hook would
remain as is.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 15:30 ` Jan Beulich
@ 2019-04-03 15:48 ` Razvan Cojocaru
2019-04-03 16:04 ` Jan Beulich
2019-04-04 12:46 ` Razvan Cojocaru
1 sibling, 1 reply; 27+ messages in thread
From: Razvan Cojocaru @ 2019-04-03 15:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
>> existing altp2ms, but they do so in a lazy manner, and also that won't
>> happen for altp2ms created after a while. So altp2ms will not
>> necessarily know about a page that the hostp2m knows about, which should
>> not stop us from setting mem access restrictions or the value of the SVE
>> bit.
>
> But even if the propagation is lazy, a "get" should then return the
> propagated value, shouldn't it? Or else callers (like is the case here)
> can be mislead. I do recognize that there may be callers who
> intentionally _don't_ want the propagated value, but I'd expect this
> to be the exception, not the rule. I wonder therefore whether there
> shouldn't be a wrapper around ->get_entry() to take care of this for
> callers which want the propagated value. Calling the bare hook would
> remain as is.
But I believe that some hostp2m entries will never be propagated. Sorry
that I've not been clear about this. This is what I meant by "won't
happen for altp2ms created after a while".
Take, for example, the case where there's only the hostp2m for the first
30 minutes of a guest's life, and in this time somebody calls
ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if (
entry_written && p2m_is_hostp2m(p2m) ).
But at this point there are no altp2ms, so there's nowhere to propagate
these changes to.
Now, at minute 31 we create a new altp2m. The changes will never be
propagated here, so altp2m->get_entry() will always (rightfully) return
an invalid MFN.
We only want to make an exception for setting the SVE bit or mem access
restrictions on an entry in the altp2m, but having get_entry() (or a
wrapper) return the hostp2m values and MFN might not necessarily be what
current callers expect.
Whether the described scenario _should_ work the way it does is
debatable, but it appears to do so by design.
Hopefully I'm not missing anything.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 15:48 ` Razvan Cojocaru
@ 2019-04-03 16:04 ` Jan Beulich
2019-04-03 16:16 ` Tamas K Lengyel
2019-04-03 17:41 ` Razvan Cojocaru
0 siblings, 2 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-03 16:04 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
>>> On 03.04.19 at 17:48, <rcojocaru@bitdefender.com> wrote:
> On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
>>> existing altp2ms, but they do so in a lazy manner, and also that won't
>>> happen for altp2ms created after a while. So altp2ms will not
>>> necessarily know about a page that the hostp2m knows about, which should
>>> not stop us from setting mem access restrictions or the value of the SVE
>>> bit.
>>
>> But even if the propagation is lazy, a "get" should then return the
>> propagated value, shouldn't it? Or else callers (like is the case here)
>> can be mislead. I do recognize that there may be callers who
>> intentionally _don't_ want the propagated value, but I'd expect this
>> to be the exception, not the rule. I wonder therefore whether there
>> shouldn't be a wrapper around ->get_entry() to take care of this for
>> callers which want the propagated value. Calling the bare hook would
>> remain as is.
>
> But I believe that some hostp2m entries will never be propagated. Sorry
> that I've not been clear about this. This is what I meant by "won't
> happen for altp2ms created after a while".
>
> Take, for example, the case where there's only the hostp2m for the first
> 30 minutes of a guest's life, and in this time somebody calls
> ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if (
> entry_written && p2m_is_hostp2m(p2m) ).
>
> But at this point there are no altp2ms, so there's nowhere to propagate
> these changes to.
>
> Now, at minute 31 we create a new altp2m. The changes will never be
> propagated here, so altp2m->get_entry() will always (rightfully) return
> an invalid MFN.
I guess I'm missing something here: Why "rightfully"? If the guest
runs on this altp2m, it'll die a quick death then, won't it? Yet if
this is intended to be demand-populate, then why would there be
automatic propagation for existing altp2m-s (as you explain
further up, unless I'm getting this wrong)?
> We only want to make an exception for setting the SVE bit or mem access
> restrictions on an entry in the altp2m, but having get_entry() (or a
> wrapper) return the hostp2m values and MFN might not necessarily be what
> current callers expect.
Then I still don't see why you would want to force propagation
in these cases: If it's generally demand-populate, it can't be right
to _fully_ populate that slot. You ought to be setting the access
type or the "suppress #VE" bit alone, without propagating the
MFN and alike. The later, when the entry actually gets propagated,
the entered values should be used as overrides to what comes
from the host p2m.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 16:04 ` Jan Beulich
@ 2019-04-03 16:16 ` Tamas K Lengyel
2019-04-03 17:07 ` Razvan Cojocaru
2019-04-04 10:16 ` Jan Beulich
2019-04-03 17:41 ` Razvan Cojocaru
1 sibling, 2 replies; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-03 16:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Andrew Cooper,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Wed, Apr 3, 2019 at 10:06 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 03.04.19 at 17:48, <rcojocaru@bitdefender.com> wrote:
> > On 4/3/19 6:30 PM, Jan Beulich wrote:
> >>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
> >>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
> >>> existing altp2ms, but they do so in a lazy manner, and also that won't
> >>> happen for altp2ms created after a while. So altp2ms will not
> >>> necessarily know about a page that the hostp2m knows about, which should
> >>> not stop us from setting mem access restrictions or the value of the SVE
> >>> bit.
> >>
> >> But even if the propagation is lazy, a "get" should then return the
> >> propagated value, shouldn't it? Or else callers (like is the case here)
> >> can be mislead. I do recognize that there may be callers who
> >> intentionally _don't_ want the propagated value, but I'd expect this
> >> to be the exception, not the rule. I wonder therefore whether there
> >> shouldn't be a wrapper around ->get_entry() to take care of this for
> >> callers which want the propagated value. Calling the bare hook would
> >> remain as is.
> >
> > But I believe that some hostp2m entries will never be propagated. Sorry
> > that I've not been clear about this. This is what I meant by "won't
> > happen for altp2ms created after a while".
> >
> > Take, for example, the case where there's only the hostp2m for the first
> > 30 minutes of a guest's life, and in this time somebody calls
> > ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if (
> > entry_written && p2m_is_hostp2m(p2m) ).
> >
> > But at this point there are no altp2ms, so there's nowhere to propagate
> > these changes to.
> >
> > Now, at minute 31 we create a new altp2m. The changes will never be
> > propagated here, so altp2m->get_entry() will always (rightfully) return
> > an invalid MFN.
>
> I guess I'm missing something here: Why "rightfully"? If the guest
> runs on this altp2m, it'll die a quick death then, won't it? Yet if
> this is intended to be demand-populate, then why would there be
> automatic propagation for existing altp2m-s (as you explain
> further up, unless I'm getting this wrong)?
>
> > We only want to make an exception for setting the SVE bit or mem access
> > restrictions on an entry in the altp2m, but having get_entry() (or a
> > wrapper) return the hostp2m values and MFN might not necessarily be what
> > current callers expect.
>
> Then I still don't see why you would want to force propagation
> in these cases: If it's generally demand-populate, it can't be right
> to _fully_ populate that slot. You ought to be setting the access
> type or the "suppress #VE" bit alone, without propagating the
> MFN and alike. The later, when the entry actually gets propagated,
> the entered values should be used as overrides to what comes
> from the host p2m.
It is right to fully populate a slot when for example we want
different mem_access permissions in different altp2m views. We can't
wait for the domain to on-demand populate the altp2m view because we
don't know when (and if) that will actually happen. So
p2m_set_altp2m_mem_access already copies the entry over from the
hostp2m to the altp2m and then applies the requested mem_access
setting. This is done even if the mem_access setting is the same as it
was in the hostp2m.
Doing the same behavior for p2m_set_suppress_ve I think is consistent,
albeit you could easily overcome the issue by first simply setting a
mem_access permission on the gfn to trigger the aforementioned copy to
the altp2m before you try to set the ve settings.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 16:16 ` Tamas K Lengyel
@ 2019-04-03 17:07 ` Razvan Cojocaru
2019-04-03 17:12 ` Tamas K Lengyel
2019-04-04 10:16 ` Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: Razvan Cojocaru @ 2019-04-03 17:07 UTC (permalink / raw)
To: Tamas K Lengyel, Jan Beulich
Cc: Wei Liu, George Dunlap, Andrew Cooper, Alexandru Isaila,
xen-devel, Roger Pau Monne
On 4/3/19 7:16 PM, Tamas K Lengyel wrote:
> It is right to fully populate a slot when for example we want
> different mem_access permissions in different altp2m views. We can't
> wait for the domain to on-demand populate the altp2m view because we
> don't know when (and if) that will actually happen. So
> p2m_set_altp2m_mem_access already copies the entry over from the
> hostp2m to the altp2m and then applies the requested mem_access
> setting. This is done even if the mem_access setting is the same as it
> was in the hostp2m.
>
> Doing the same behavior for p2m_set_suppress_ve I think is consistent,
> albeit you could easily overcome the issue by first simply setting a
> mem_access permission on the gfn to trigger the aforementioned copy to
> the altp2m before you try to set the ve settings.
That's what we're doing now, but it wasn't at all intuitive to figure it
out. I'd expect that setting the SVE bit simply works without
maneuvering something else in place first.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 17:07 ` Razvan Cojocaru
@ 2019-04-03 17:12 ` Tamas K Lengyel
0 siblings, 0 replies; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-03 17:12 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, George Dunlap, Andrew Cooper, Jan Beulich,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Wed, Apr 3, 2019 at 11:08 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> On 4/3/19 7:16 PM, Tamas K Lengyel wrote:
> > It is right to fully populate a slot when for example we want
> > different mem_access permissions in different altp2m views. We can't
> > wait for the domain to on-demand populate the altp2m view because we
> > don't know when (and if) that will actually happen. So
> > p2m_set_altp2m_mem_access already copies the entry over from the
> > hostp2m to the altp2m and then applies the requested mem_access
> > setting. This is done even if the mem_access setting is the same as it
> > was in the hostp2m.
> >
> > Doing the same behavior for p2m_set_suppress_ve I think is consistent,
> > albeit you could easily overcome the issue by first simply setting a
> > mem_access permission on the gfn to trigger the aforementioned copy to
> > the altp2m before you try to set the ve settings.
>
> That's what we're doing now, but it wasn't at all intuitive to figure it
> out. I'd expect that setting the SVE bit simply works without
> maneuvering something else in place first.
Yeap, makes sense to me :)
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 16:04 ` Jan Beulich
2019-04-03 16:16 ` Tamas K Lengyel
@ 2019-04-03 17:41 ` Razvan Cojocaru
2019-04-04 10:47 ` Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: Razvan Cojocaru @ 2019-04-03 17:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
On 4/3/19 7:04 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 17:48, <rcojocaru@bitdefender.com> wrote:
>> On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
>>>> existing altp2ms, but they do so in a lazy manner, and also that won't
>>>> happen for altp2ms created after a while. So altp2ms will not
>>>> necessarily know about a page that the hostp2m knows about, which should
>>>> not stop us from setting mem access restrictions or the value of the SVE
>>>> bit.
>>>
>>> But even if the propagation is lazy, a "get" should then return the
>>> propagated value, shouldn't it? Or else callers (like is the case here)
>>> can be mislead. I do recognize that there may be callers who
>>> intentionally _don't_ want the propagated value, but I'd expect this
>>> to be the exception, not the rule. I wonder therefore whether there
>>> shouldn't be a wrapper around ->get_entry() to take care of this for
>>> callers which want the propagated value. Calling the bare hook would
>>> remain as is.
>>
>> But I believe that some hostp2m entries will never be propagated. Sorry
>> that I've not been clear about this. This is what I meant by "won't
>> happen for altp2ms created after a while".
>>
>> Take, for example, the case where there's only the hostp2m for the first
>> 30 minutes of a guest's life, and in this time somebody calls
>> ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if (
>> entry_written && p2m_is_hostp2m(p2m) ).
>>
>> But at this point there are no altp2ms, so there's nowhere to propagate
>> these changes to.
>>
>> Now, at minute 31 we create a new altp2m. The changes will never be
>> propagated here, so altp2m->get_entry() will always (rightfully) return
>> an invalid MFN.
>
> I guess I'm missing something here: Why "rightfully"? If the guest
> runs on this altp2m, it'll die a quick death then, won't it? Yet if
> this is intended to be demand-populate, then why would there be
> automatic propagation for existing altp2m-s (as you explain
> further up, unless I'm getting this wrong)?
>
>> We only want to make an exception for setting the SVE bit or mem access
>> restrictions on an entry in the altp2m, but having get_entry() (or a
>> wrapper) return the hostp2m values and MFN might not necessarily be what
>> current callers expect.
>
> Then I still don't see why you would want to force propagation
> in these cases: If it's generally demand-populate, it can't be right
> to _fully_ populate that slot. You ought to be setting the access
> type or the "suppress #VE" bit alone, without propagating the
> MFN and alike. The later, when the entry actually gets propagated,
> the entered values should be used as overrides to what comes
> from the host p2m.
To try to answer your question to the best of my knowledge, the altp2m
propagation appears to be a hybrid between lazy propagation, in
hvm_hap_nested_page_fault() (xen/arch/x86/hvm/hvm.c):
1748 ap2m_active = altp2m_active(currd);
1749
1750 /*
1751 * Take a lock on the host p2m speculatively, to avoid potential
1752 * locking order problems later and to handle unshare etc.
1753 */
1754 hostp2m = p2m_get_hostp2m(currd);
1755 mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
1756 P2M_ALLOC | (npfec.write_access ?
P2M_UNSHARE : 0),
1757 NULL);
1758
1759 if ( ap2m_active )
1760 {
1761 if ( p2m_altp2m_lazy_copy(curr, gpa, gla, npfec, &p2m) )
1762 {
1763 /* entry was lazily copied from host -- retry */
1764 __put_gfn(hostp2m, gfn);
1765 rc = 1;
1766 goto out;
1767 }
1768
1769 mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
1770 }
1771 else
1772 p2m = hostp2m;
and immediate propagation, in ept_set_entry() (xen/arch/x86/mm/p2m-ept.c):
827 if ( entry_written && p2m_is_hostp2m(p2m) )
828 {
829 ret = p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order,
p2mt, p2ma);
830 if ( !rc )
831 rc = ret;
832 }
Either way, doing what our patch does should pose no problem as far as I
can tell. For the lazy copy case, p2m_altp2m_lazy_copy() will do
nothing, as our function has already populated the slot correctly:
2378 bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t gpa,
2379 unsigned long gla, struct npfec npfec,
2380 struct p2m_domain **ap2m)
2381 {
2382 struct p2m_domain *hp2m = p2m_get_hostp2m(v->domain);
2383 p2m_type_t p2mt;
2384 p2m_access_t p2ma;
2385 unsigned int page_order;
2386 gfn_t gfn = _gfn(paddr_to_pfn(gpa));
2387 unsigned long mask;
2388 mfn_t mfn;
2389 int rv;
2390
2391 *ap2m = p2m_get_altp2m(v);
2392
2393 mfn = get_gfn_type_access(*ap2m, gfn_x(gfn), &p2mt, &p2ma,
2394 0, &page_order);
2395 __put_gfn(*ap2m, gfn_x(gfn));
2396
2397 if ( !mfn_eq(mfn, INVALID_MFN) )
2398 return 0;
And the other case is not a concern at all.
Am I still misunderstanding something?
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 16:16 ` Tamas K Lengyel
2019-04-03 17:07 ` Razvan Cojocaru
@ 2019-04-04 10:16 ` Jan Beulich
1 sibling, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-04 10:16 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Andrew Cooper, aisaila,
xen-devel, Roger Pau Monne
>>> On 03.04.19 at 18:16, <tamas.k.lengyel@gmail.com> wrote:
> On Wed, Apr 3, 2019 at 10:06 AM Jan Beulich <JBeulich@suse.com> wrote:
>> Then I still don't see why you would want to force propagation
>> in these cases: If it's generally demand-populate, it can't be right
>> to _fully_ populate that slot. You ought to be setting the access
>> type or the "suppress #VE" bit alone, without propagating the
>> MFN and alike. The later, when the entry actually gets propagated,
>> the entered values should be used as overrides to what comes
>> from the host p2m.
>
> It is right to fully populate a slot when for example we want
> different mem_access permissions in different altp2m views. We can't
> wait for the domain to on-demand populate the altp2m view because we
> don't know when (and if) that will actually happen.
But you don't say _why_ it matters whether / when that's going
to happen.
> So
> p2m_set_altp2m_mem_access already copies the entry over from the
> hostp2m to the altp2m and then applies the requested mem_access
> setting. This is done even if the mem_access setting is the same as it
> was in the hostp2m.
And again you describe only current code, without clarifying why
the behavior is (and needs to be) the way it is.
> Doing the same behavior for p2m_set_suppress_ve I think is consistent,
> albeit you could easily overcome the issue by first simply setting a
> mem_access permission on the gfn to trigger the aforementioned copy to
> the altp2m before you try to set the ve settings.
Well, whatever the set-access behavior, the set-suppress-ve behavior
should match it, I agree. What I'm putting under question is whether
the former is really correct, and hence whether the patch here wouldn't
end up proliferating wrong behavior.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 17:41 ` Razvan Cojocaru
@ 2019-04-04 10:47 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-04 10:47 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
>>> On 03.04.19 at 19:41, <rcojocaru@bitdefender.com> wrote:
> On 4/3/19 7:04 PM, Jan Beulich wrote:
>>>>> On 03.04.19 at 17:48, <rcojocaru@bitdefender.com> wrote:
>>> On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>>>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
>>>>> existing altp2ms, but they do so in a lazy manner, and also that won't
>>>>> happen for altp2ms created after a while. So altp2ms will not
>>>>> necessarily know about a page that the hostp2m knows about, which should
>>>>> not stop us from setting mem access restrictions or the value of the SVE
>>>>> bit.
>>>>
>>>> But even if the propagation is lazy, a "get" should then return the
>>>> propagated value, shouldn't it? Or else callers (like is the case here)
>>>> can be mislead. I do recognize that there may be callers who
>>>> intentionally _don't_ want the propagated value, but I'd expect this
>>>> to be the exception, not the rule. I wonder therefore whether there
>>>> shouldn't be a wrapper around ->get_entry() to take care of this for
>>>> callers which want the propagated value. Calling the bare hook would
>>>> remain as is.
>>>
>>> But I believe that some hostp2m entries will never be propagated. Sorry
>>> that I've not been clear about this. This is what I meant by "won't
>>> happen for altp2ms created after a while".
>>>
>>> Take, for example, the case where there's only the hostp2m for the first
>>> 30 minutes of a guest's life, and in this time somebody calls
>>> ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if (
>>> entry_written && p2m_is_hostp2m(p2m) ).
>>>
>>> But at this point there are no altp2ms, so there's nowhere to propagate
>>> these changes to.
>>>
>>> Now, at minute 31 we create a new altp2m. The changes will never be
>>> propagated here, so altp2m->get_entry() will always (rightfully) return
>>> an invalid MFN.
>>
>> I guess I'm missing something here: Why "rightfully"? If the guest
>> runs on this altp2m, it'll die a quick death then, won't it? Yet if
>> this is intended to be demand-populate, then why would there be
>> automatic propagation for existing altp2m-s (as you explain
>> further up, unless I'm getting this wrong)?
>>
>>> We only want to make an exception for setting the SVE bit or mem access
>>> restrictions on an entry in the altp2m, but having get_entry() (or a
>>> wrapper) return the hostp2m values and MFN might not necessarily be what
>>> current callers expect.
>>
>> Then I still don't see why you would want to force propagation
>> in these cases: If it's generally demand-populate, it can't be right
>> to _fully_ populate that slot. You ought to be setting the access
>> type or the "suppress #VE" bit alone, without propagating the
>> MFN and alike. The later, when the entry actually gets propagated,
>> the entered values should be used as overrides to what comes
>> from the host p2m.
> To try to answer your question to the best of my knowledge, the altp2m
> propagation appears to be a hybrid between lazy propagation,
>[...]
> and immediate propagation,
And I've been questioning whether this is an appropriate model,
i.e. whether the result at all times is always the same as either of
the pure variants. Thinking about it some more, it looks like it
indeed is. Yet then the question is why both functions don't use
p2m_altp2m_lazy_copy(), but rather construct things by other
means. I realize the function's gla and npfec parameters might
make it look non-suitable here, but neither parameter is actually
used by the function afaics.
And of course the other point remains: For callers who want to
see the propagate value (irrespective of what the physical
tables say), there would better be a function giving them the
intended result, rather than making every such caller go through
extra hoops.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-03 15:30 ` Jan Beulich
2019-04-03 15:48 ` Razvan Cojocaru
@ 2019-04-04 12:46 ` Razvan Cojocaru
2019-04-04 12:49 ` Andrew Cooper
2019-04-04 12:50 ` Razvan Cojocaru
1 sibling, 2 replies; 27+ messages in thread
From: Razvan Cojocaru @ 2019-04-04 12:46 UTC (permalink / raw)
To: Jan Beulich, Tamas K Lengyel
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>> On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>>>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>>>> if ( !mfn_valid(mfn) )
>>>> {
>>>> - rc = -ESRCH;
>>>> - goto out;
>>>> + unsigned int page_order;
>>>> +
>>>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
>>>> + P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
>>>
>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
>>> at least P2M_UNSHARE is too heavy: Why would you want to force
>>> un-sharing of a page when all you want to alter is #VE behavior?
>>
>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
>> two cases are very similar.
>
> I see.
On the UNSHARE observation, we don't know why the author originally
requested the flag. We decided to keep it on the assumption that it
_probably_ handles some corner-case that somebody has come accross.
We'll prepare a mini-series factoring out the code we've been discussing
in separate functions: one for getting things out of the hostp2m if the
entry is not present in the altp2m, and one for the special
page-order-dependent code (which is duplicated in
p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
Before going into that, are we now certain that ALLOC is sufficient? I
believe it should be for _our_ use-cases, but we don't want to break
anyone's code. Maybe Tamas knows more about this.
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 12:46 ` Razvan Cojocaru
@ 2019-04-04 12:49 ` Andrew Cooper
2019-04-04 13:15 ` Tamas K Lengyel
2019-04-04 12:50 ` Razvan Cojocaru
1 sibling, 1 reply; 27+ messages in thread
From: Andrew Cooper @ 2019-04-04 12:49 UTC (permalink / raw)
To: Razvan Cojocaru, Jan Beulich, Tamas K Lengyel
Cc: George Dunlap, aisaila, Roger Pau Monne, Wei Liu, xen-devel
On 04/04/2019 13:46, Razvan Cojocaru wrote:
> On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>> On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d,
>>>>> gfn_t gfn, bool suppress_ve,
>>>>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>>>>> if ( !mfn_valid(mfn) )
>>>>> {
>>>>> - rc = -ESRCH;
>>>>> - goto out;
>>>>> + unsigned int page_order;
>>>>> +
>>>>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
>>>>> + P2M_ALLOC | P2M_UNSHARE,
>>>>> &page_order, 0);
>>>>
>>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
>>>> at least P2M_UNSHARE is too heavy: Why would you want to force
>>>> un-sharing of a page when all you want to alter is #VE behavior?
>>>
>>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
>>> two cases are very similar.
>>
>> I see.
>
> On the UNSHARE observation, we don't know why the author originally
> requested the flag. We decided to keep it on the assumption that it
> _probably_ handles some corner-case that somebody has come accross.
>
> We'll prepare a mini-series factoring out the code we've been
> discussing in separate functions: one for getting things out of the
> hostp2m if the entry is not present in the altp2m, and one for the
> special page-order-dependent code (which is duplicated in
> p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
>
> Before going into that, are we now certain that ALLOC is sufficient? I
> believe it should be for _our_ use-cases, but we don't want to break
> anyone's code. Maybe Tamas knows more about this.
UNSHARE is only for CoW/Paging, which is some experimental code added to
Xen 4.2(?) and has been slowly bit-rotting ever since.
We try to use good judgement when considering the intentions, but it is
fully out of security support and hasn't seen any testing in years.
You're not going to break anything by making a mistake here.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 12:46 ` Razvan Cojocaru
2019-04-04 12:49 ` Andrew Cooper
@ 2019-04-04 12:50 ` Razvan Cojocaru
2019-04-04 13:09 ` Tamas K Lengyel
2019-04-04 14:32 ` Jan Beulich
1 sibling, 2 replies; 27+ messages in thread
From: Razvan Cojocaru @ 2019-04-04 12:50 UTC (permalink / raw)
To: Jan Beulich, Tamas K Lengyel
Cc: Wei Liu, George Dunlap, Andrew Cooper, aisaila, xen-devel,
Roger Pau Monne
On 4/4/19 3:46 PM, Razvan Cojocaru wrote:
> On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>> On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d,
>>>>> gfn_t gfn, bool suppress_ve,
>>>>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>>>>> if ( !mfn_valid(mfn) )
>>>>> {
>>>>> - rc = -ESRCH;
>>>>> - goto out;
>>>>> + unsigned int page_order;
>>>>> +
>>>>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
>>>>> + P2M_ALLOC | P2M_UNSHARE,
>>>>> &page_order, 0);
>>>>
>>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
>>>> at least P2M_UNSHARE is too heavy: Why would you want to force
>>>> un-sharing of a page when all you want to alter is #VE behavior?
>>>
>>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
>>> two cases are very similar.
>>
>> I see.
>
> On the UNSHARE observation, we don't know why the author originally
> requested the flag. We decided to keep it on the assumption that it
> _probably_ handles some corner-case that somebody has come accross.
>
> We'll prepare a mini-series factoring out the code we've been discussing
> in separate functions: one for getting things out of the hostp2m if the
> entry is not present in the altp2m, and one for the special
> page-order-dependent code (which is duplicated in
> p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
>
> Before going into that, are we now certain that ALLOC is sufficient? I
> believe it should be for _our_ use-cases, but we don't want to break
> anyone's code. Maybe Tamas knows more about this.
Sorry, I forgot to mention that p2m_change_altp2m_gfn() only uses ALLOC:
2649 /* Check host p2m if no valid entry in alternate */
2650 if ( !mfn_valid(mfn) )
2651 {
2652 mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
2653 P2M_ALLOC, &page_order, 0);
2654
2655 if ( !mfn_valid(mfn) || t != p2m_ram_rw )
2656 goto out;
2657
2658 /* If this is a superpage, copy that first */
2659 if ( page_order != PAGE_ORDER_4K )
2660 {
2661 gfn_t gfn;
2662 unsigned long mask;
2663
2664 mask = ~((1UL << page_order) - 1);
2665 gfn = _gfn(gfn_x(old_gfn) & mask);
2666 mfn = _mfn(mfn_x(mfn) & mask);
2667
2668 if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
2669 goto out;
2670 }
2671 }
Confusing...
Thanks,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 12:50 ` Razvan Cojocaru
@ 2019-04-04 13:09 ` Tamas K Lengyel
2019-04-04 14:36 ` Jan Beulich
2019-04-04 14:32 ` Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-04 13:09 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Wei Liu, George Dunlap, Andrew Cooper, Jan Beulich,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Thu, Apr 4, 2019 at 6:50 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> On 4/4/19 3:46 PM, Razvan Cojocaru wrote:
> > On 4/3/19 6:30 PM, Jan Beulich wrote:
> >>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
> >>> On 4/3/19 5:58 PM, Jan Beulich wrote:
> >>>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
> >>>>> --- a/xen/arch/x86/mm/p2m.c
> >>>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d,
> >>>>> gfn_t gfn, bool suppress_ve,
> >>>>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> >>>>> if ( !mfn_valid(mfn) )
> >>>>> {
> >>>>> - rc = -ESRCH;
> >>>>> - goto out;
> >>>>> + unsigned int page_order;
> >>>>> +
> >>>>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
> >>>>> + P2M_ALLOC | P2M_UNSHARE,
> >>>>> &page_order, 0);
> >>>>
> >>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
> >>>> at least P2M_UNSHARE is too heavy: Why would you want to force
> >>>> un-sharing of a page when all you want to alter is #VE behavior?
> >>>
> >>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
> >>> two cases are very similar.
> >>
> >> I see.
> >
> > On the UNSHARE observation, we don't know why the author originally
> > requested the flag. We decided to keep it on the assumption that it
> > _probably_ handles some corner-case that somebody has come accross.
> >
> > We'll prepare a mini-series factoring out the code we've been discussing
> > in separate functions: one for getting things out of the hostp2m if the
> > entry is not present in the altp2m, and one for the special
> > page-order-dependent code (which is duplicated in
> > p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
> >
> > Before going into that, are we now certain that ALLOC is sufficient? I
> > believe it should be for _our_ use-cases, but we don't want to break
> > anyone's code. Maybe Tamas knows more about this.
>
> Sorry, I forgot to mention that p2m_change_altp2m_gfn() only uses ALLOC:
>
> 2649 /* Check host p2m if no valid entry in alternate */
> 2650 if ( !mfn_valid(mfn) )
> 2651 {
> 2652 mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
> 2653 P2M_ALLOC, &page_order, 0);
> 2654
> 2655 if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> 2656 goto out;
> 2657
> 2658 /* If this is a superpage, copy that first */
> 2659 if ( page_order != PAGE_ORDER_4K )
> 2660 {
> 2661 gfn_t gfn;
> 2662 unsigned long mask;
> 2663
> 2664 mask = ~((1UL << page_order) - 1);
> 2665 gfn = _gfn(gfn_x(old_gfn) & mask);
> 2666 mfn = _mfn(mfn_x(mfn) & mask);
> 2667
> 2668 if ( ap2m->set_entry(ap2m, gfn, mfn, page_order, t, a, 1) )
> 2669 goto out;
> 2670 }
> 2671 }
>
> Confusing...
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.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 12:49 ` Andrew Cooper
@ 2019-04-04 13:15 ` Tamas K Lengyel
0 siblings, 0 replies; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-04 13:15 UTC (permalink / raw)
To: Andrew Cooper
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Jan Beulich,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Thu, Apr 4, 2019 at 6:49 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 04/04/2019 13:46, Razvan Cojocaru wrote:
> > On 4/3/19 6:30 PM, Jan Beulich wrote:
> >>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
> >>> On 4/3/19 5:58 PM, Jan Beulich wrote:
> >>>>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
> >>>>> --- a/xen/arch/x86/mm/p2m.c
> >>>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d,
> >>>>> gfn_t gfn, bool suppress_ve,
> >>>>> mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> >>>>> if ( !mfn_valid(mfn) )
> >>>>> {
> >>>>> - rc = -ESRCH;
> >>>>> - goto out;
> >>>>> + unsigned int page_order;
> >>>>> +
> >>>>> + mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
> >>>>> + P2M_ALLOC | P2M_UNSHARE,
> >>>>> &page_order, 0);
> >>>>
> >>>> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
> >>>> at least P2M_UNSHARE is too heavy: Why would you want to force
> >>>> un-sharing of a page when all you want to alter is #VE behavior?
> >>>
> >>> That logic was taken from p2m_set_altp2m_mem_access(), we thought the
> >>> two cases are very similar.
> >>
> >> I see.
> >
> > On the UNSHARE observation, we don't know why the author originally
> > requested the flag. We decided to keep it on the assumption that it
> > _probably_ handles some corner-case that somebody has come accross.
> >
> > We'll prepare a mini-series factoring out the code we've been
> > discussing in separate functions: one for getting things out of the
> > hostp2m if the entry is not present in the altp2m, and one for the
> > special page-order-dependent code (which is duplicated in
> > p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).
> >
> > Before going into that, are we now certain that ALLOC is sufficient? I
> > believe it should be for _our_ use-cases, but we don't want to break
> > anyone's code. Maybe Tamas knows more about this.
>
> UNSHARE is only for CoW/Paging, which is some experimental code added to
> Xen 4.2(?) and has been slowly bit-rotting ever since.
At least mem_sharing still (suprisingly) works - albeit some recent
sanity-check restrictions on grabbing the lock for two gfn's at the
same time breaks it for debug builds. But that's a different story
that I hope to address in the near future.
>
> We try to use good judgement when considering the intentions, but it is
> fully out of security support and hasn't seen any testing in years.
> You're not going to break anything by making a mistake here.
Tsk, I still use mem_sharing and in fact have put in effort to make it
compatible with altp2m. But as far as triggering UNSHARE just to be
safe, I agree. That is fine and it won't break anything. It is
actually inline with what we agreed on last time I worked on it as a
general guide - keep altp2m and mem_sharing compatible but at least
exclusive for each frame.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 12:50 ` Razvan Cojocaru
2019-04-04 13:09 ` Tamas K Lengyel
@ 2019-04-04 14:32 ` Jan Beulich
1 sibling, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-04 14:32 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Tamas K Lengyel, Wei Liu, George Dunlap, Andrew Cooper, aisaila,
xen-devel, Roger Pau Monne
>>> On 04.04.19 at 14:50, <rcojocaru@bitdefender.com> wrote:
> On 4/4/19 3:46 PM, Razvan Cojocaru wrote:
>> Before going into that, are we now certain that ALLOC is sufficient? I
>> believe it should be for _our_ use-cases, but we don't want to break
>> anyone's code. Maybe Tamas knows more about this.
>
> Sorry, I forgot to mention that p2m_change_altp2m_gfn() only uses ALLOC:
I think this together with Andrew's reply and what I have said
earlier on should be sufficient to answer the question with "yes".
But I see Tamas is telling you the opposite, so I guess I'll reply
there as well.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 13:09 ` Tamas K Lengyel
@ 2019-04-04 14:36 ` Jan Beulich
2019-04-04 14:43 ` Tamas K Lengyel
2019-04-04 14:54 ` George Dunlap
0 siblings, 2 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-04 14:36 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Andrew Cooper, aisaila,
xen-devel, Roger Pau Monne
>>> 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.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 14:36 ` Jan Beulich
@ 2019-04-04 14:43 ` Tamas K Lengyel
2019-04-04 14:51 ` Jan Beulich
2019-04-04 14:54 ` George Dunlap
1 sibling, 1 reply; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-04 14:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Andrew Cooper,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Thu, Apr 4, 2019 at 8:36 AM 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,
That's incorrect. Mem sharing doesn't care about the contents of pages at all.
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.
We are unsharing to keep altp2m and mem_sharing compatible but
mutually exclusive. Even if technically they could co-exist, last time
I worked on this we came to this agreement on the mailinglist as to
reduce complexity and to make reviewing easier.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 14:43 ` Tamas K Lengyel
@ 2019-04-04 14:51 ` Jan Beulich
2019-04-04 15:06 ` Tamas K Lengyel
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2019-04-04 14:51 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Andrew Cooper, aisaila,
xen-devel, Roger Pau Monne
>>> On 04.04.19 at 16:43, <tamas@tklengyel.com> wrote:
> On Thu, Apr 4, 2019 at 8:36 AM 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,
>
> That's incorrect. Mem sharing doesn't care about the contents of pages at
> all.
Would you mind clarifying this? It's about sharing of the contents
of the pages, isn't it? (Of course the me-sharing code doesn't care
what the contents are.)
> 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.
>
> We are unsharing to keep altp2m and mem_sharing compatible but
> mutually exclusive. Even if technically they could co-exist, last time
> I worked on this we came to this agreement on the mailinglist as to
> reduce complexity and to make reviewing easier.
But "mutually exclusive" to me means you can do only one of the
two on a particular VM. Which then makes it even less necessary
to request un-share from altp2m code - such requests would then
simple be no-ops.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 14:36 ` Jan Beulich
2019-04-04 14:43 ` Tamas K Lengyel
@ 2019-04-04 14:54 ` George Dunlap
2019-04-05 7:36 ` [Xen-devel] " Jan Beulich
1 sibling, 1 reply; 27+ messages in thread
From: George Dunlap @ 2019-04-04 14:54 UTC (permalink / raw)
To: Jan Beulich
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, Andrew Cooper,
George Dunlap, Alexandru Stefan ISAILA, xen-devel,
Roger Pau Monne
> 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.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
2019-04-04 14:51 ` Jan Beulich
@ 2019-04-04 15:06 ` Tamas K Lengyel
0 siblings, 0 replies; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-04 15:06 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Razvan Cojocaru, George Dunlap, Andrew Cooper,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Thu, Apr 4, 2019 at 8:51 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 04.04.19 at 16:43, <tamas@tklengyel.com> wrote:
> > On Thu, Apr 4, 2019 at 8:36 AM 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,
> >
> > That's incorrect. Mem sharing doesn't care about the contents of pages at
> > all.
>
> Would you mind clarifying this? It's about sharing of the contents
> of the pages, isn't it? (Of course the me-sharing code doesn't care
> what the contents are.)
That's what I mean. The contents of the pages are never checked - you
can share two pages with different contents in which case the "client"
pages' contents are discarded. Once pages are shared it means that the
pages have the same backing mfn. So it's not that the system ensures
that there *contents* are the same, it's only that while the type is
p2m_ram_shared, they have the same backing mfn.
>
> > 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.
> >
> > We are unsharing to keep altp2m and mem_sharing compatible but
> > mutually exclusive. Even if technically they could co-exist, last time
> > I worked on this we came to this agreement on the mailinglist as to
> > reduce complexity and to make reviewing easier.
>
> But "mutually exclusive" to me means you can do only one of the
> two on a particular VM. Which then makes it even less necessary
> to request un-share from altp2m code - such requests would then
> simple be no-ops.
Which is exactly what I wanted to avoid. I want to be able use both on
the same domain. But that complexity was too much so we relaxed the
"exclusivity" to be per-page instead of global per VM.
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
@ 2019-04-05 7:36 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-05 7:36 UTC (permalink / raw)
To: george.dunlap
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, Andrew Cooper,
aisaila, xen-devel, Roger Pau Monne
>>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve
@ 2019-04-05 7:36 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2019-04-05 7:36 UTC (permalink / raw)
To: george.dunlap
Cc: Tamas K Lengyel, Wei Liu, Razvan Cojocaru, Andrew Cooper,
aisaila, xen-devel, Roger Pau Monne
>>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1] Fix p2m_set_suppress_ve
@ 2019-04-05 13:59 ` Tamas K Lengyel
0 siblings, 0 replies; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-05 13:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Razvan Cojocaru, Andrew Cooper, George Dunlap,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Fri, Apr 5, 2019 at 1:36 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> 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.
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve
@ 2019-04-05 13:59 ` Tamas K Lengyel
0 siblings, 0 replies; 27+ messages in thread
From: Tamas K Lengyel @ 2019-04-05 13:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Razvan Cojocaru, Andrew Cooper, George Dunlap,
Alexandru Isaila, xen-devel, Roger Pau Monne
On Fri, Apr 5, 2019 at 1:36 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> 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.
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-04-05 14:00 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-03 14:29 [PATCH v1] Fix p2m_set_suppress_ve Alexandru Stefan ISAILA
2019-04-03 14:58 ` Jan Beulich
2019-04-03 15:17 ` Razvan Cojocaru
2019-04-03 15:30 ` Jan Beulich
2019-04-03 15:48 ` Razvan Cojocaru
2019-04-03 16:04 ` Jan Beulich
2019-04-03 16:16 ` Tamas K Lengyel
2019-04-03 17:07 ` Razvan Cojocaru
2019-04-03 17:12 ` Tamas K Lengyel
2019-04-04 10:16 ` Jan Beulich
2019-04-03 17:41 ` Razvan Cojocaru
2019-04-04 10:47 ` Jan Beulich
2019-04-04 12:46 ` Razvan Cojocaru
2019-04-04 12:49 ` Andrew Cooper
2019-04-04 13:15 ` Tamas K Lengyel
2019-04-04 12:50 ` Razvan Cojocaru
2019-04-04 13:09 ` Tamas K Lengyel
2019-04-04 14:36 ` Jan Beulich
2019-04-04 14:43 ` Tamas K Lengyel
2019-04-04 14:51 ` Jan Beulich
2019-04-04 15:06 ` Tamas K Lengyel
2019-04-04 14:54 ` George Dunlap
2019-04-05 7:36 ` Jan Beulich
2019-04-05 7:36 ` [Xen-devel] " Jan Beulich
2019-04-05 13:59 ` Tamas K Lengyel
2019-04-05 13:59 ` [Xen-devel] " Tamas K Lengyel
2019-04-04 14:32 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.