1. Patchew
  2. Xen
  3. View series

[PATCH] common: map_vcpu_info() wants to unshare the underlying page

Jan Beulich posted 1 patch 1 year, 6 months ago
Failed in applying to current master (apply log)
[PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Jan Beulich 1 year, 6 months ago 
Not passing P2M_UNSHARE to get_page_from_gfn() means there won't even be
an attempt to unshare the referenced page, without any indication to the
caller (e.g. -EAGAIN). Note that guests have no direct control over
which of their pages are shared (or paged out), and hence they have no
way to make sure all on their own that the subsequent obtaining of a
writable type reference can actually succeed.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Really I wonder whether the function wouldn't better use
check_get_page_from_gfn() _and_ permit p2m_ram_rw only. Otoh the P2M
type is stale by the time it is being looked at, so all depends on the
subsequent obtaining of a writable type reference anyway ...

A similar issue then apparently exists in guest_wrmsr_xen() when writing
the hypercall page. Interestingly there p2m_is_paging() is being checked
for (but shared pages aren't).

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1484,7 +1484,7 @@ int map_vcpu_info(struct vcpu *v, unsign
     if ( (v != current) && !(v->pause_flags & VPF_down) )
         return -EINVAL;
 
-    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
+    page = get_page_from_gfn(d, gfn, NULL, P2M_UNSHARE);
     if ( !page )
         return -EINVAL;
Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Roger Pau Monné 1 year, 6 months ago 
On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't even be
> an attempt to unshare the referenced page, without any indication to the
> caller (e.g. -EAGAIN). Note that guests have no direct control over
> which of their pages are shared (or paged out), and hence they have no
> way to make sure all on their own that the subsequent obtaining of a
> writable type reference can actually succeed.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> Really I wonder whether the function wouldn't better use
> check_get_page_from_gfn() _and_ permit p2m_ram_rw only. Otoh the P2M
> type is stale by the time it is being looked at, so all depends on the
> subsequent obtaining of a writable type reference anyway ...
> 
> A similar issue then apparently exists in guest_wrmsr_xen() when writing
> the hypercall page. Interestingly there p2m_is_paging() is being checked
> for (but shared pages aren't).

Doesn't guest_wrmsr_xen() also needs to use UNSHARE?

I wonder if it would be helpful to introduce some kind of helper so
that all functions can use it, get_guest_writable_page() or similar.

> 
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1484,7 +1484,7 @@ int map_vcpu_info(struct vcpu *v, unsign
>      if ( (v != current) && !(v->pause_flags & VPF_down) )
>          return -EINVAL;
>  
> -    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> +    page = get_page_from_gfn(d, gfn, NULL, P2M_UNSHARE);

I had to go look up that P2M_UNSHARE implies P2M_ALLOC for the users
of the parameter, it would be helpful to add a comment in p2m.h that
UNSHARE implies ALLOC.

Thanks, Roger.
[4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Jan Beulich 1 year, 6 months ago 
On 25.10.2022 17:42, Roger Pau Monné wrote:
> On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
>> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't even be
>> an attempt to unshare the referenced page, without any indication to the
>> caller (e.g. -EAGAIN). Note that guests have no direct control over
>> which of their pages are shared (or paged out), and hence they have no
>> way to make sure all on their own that the subsequent obtaining of a
>> writable type reference can actually succeed.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I didn't Cc you on the initial submission because mem-sharing isn't a
supported feature, but upon reconsideration I thought I'd at least ask
whether you would want to give this a release-ack. I don't really see
any risk associated with it.

Jan
RE: [4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Henry Wang 1 year, 6 months ago 
(+Arm maintainers)

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, October 26, 2022 12:07 AM
> To: Henry Wang <Henry.Wang@arm.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
> Subject: [4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare
> the underlying page
> 
> On 25.10.2022 17:42, Roger Pau Monné wrote:
> > On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
> >> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't
> even be
> >> an attempt to unshare the referenced page, without any indication to the
> >> caller (e.g. -EAGAIN). Note that guests have no direct control over
> >> which of their pages are shared (or paged out), and hence they have no
> >> way to make sure all on their own that the subsequent obtaining of a
> >> writable type reference can actually succeed.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I didn't Cc you on the initial submission because mem-sharing isn't a
> supported feature, but upon reconsideration I thought I'd at least ask
> whether you would want to give this a release-ack. I don't really see
> any risk associated with it.

By looking at the patch itself, this change seems ok to me, so I think
I will not block it, but I think Arm maintainers' approval might be needed
because of the discussion in [1], so I added them for their information.
If Arm maintainers do not object the change, you can have my release-ack.

[1] https://lore.kernel.org/xen-devel/1780acb3-d297-edc6-3a1e-adf8b28a5262@suse.com/

Kind regards,
Henry

> 
> Jan
Re: [4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Julien Grall 1 year, 6 months ago 
Hi Henry, Jan,

Sorry for the late reply.

On 26/10/2022 03:03, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, October 26, 2022 12:07 AM
>> To: Henry Wang <Henry.Wang@arm.com>
>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>;
>> Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>> Subject: [4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare
>> the underlying page
>>
>> On 25.10.2022 17:42, Roger Pau Monné wrote:
>>> On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
>>>> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't
>> even be
>>>> an attempt to unshare the referenced page, without any indication to the
>>>> caller (e.g. -EAGAIN). Note that guests have no direct control over
>>>> which of their pages are shared (or paged out), and hence they have no
>>>> way to make sure all on their own that the subsequent obtaining of a
>>>> writable type reference can actually succeed.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> I didn't Cc you on the initial submission because mem-sharing isn't a
>> supported feature, but upon reconsideration I thought I'd at least ask
>> whether you would want to give this a release-ack. I don't really see
>> any risk associated with it.
> 
> By looking at the patch itself, this change seems ok to me, so I think
> I will not block it, but I think Arm maintainers' approval might be needed
> because of the discussion in [1], so I added them for their information.
> If Arm maintainers do not object the change, you can have my release-ack.

The P2M query type is so far ignored on Arm as we neither support 
populate-on-demand nor memsharing.

I am assuming that if we ever introduce any those of features, we would 
follow the same behavior as x86. So:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall
Re: [4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Jan Beulich 1 year, 6 months ago 
On 27.10.2022 14:22, Julien Grall wrote:
> On 26/10/2022 03:03, Henry Wang wrote:
>>> -----Original Message-----
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: Wednesday, October 26, 2022 12:07 AM
>>> To: Henry Wang <Henry.Wang@arm.com>
>>> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
>>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>>> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>;
>>> Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>
>>> Subject: [4.17?] Re: [PATCH] common: map_vcpu_info() wants to unshare
>>> the underlying page
>>>
>>> On 25.10.2022 17:42, Roger Pau Monné wrote:
>>>> On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
>>>>> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't
>>> even be
>>>>> an attempt to unshare the referenced page, without any indication to the
>>>>> caller (e.g. -EAGAIN). Note that guests have no direct control over
>>>>> which of their pages are shared (or paged out), and hence they have no
>>>>> way to make sure all on their own that the subsequent obtaining of a
>>>>> writable type reference can actually succeed.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>>
>>> I didn't Cc you on the initial submission because mem-sharing isn't a
>>> supported feature, but upon reconsideration I thought I'd at least ask
>>> whether you would want to give this a release-ack. I don't really see
>>> any risk associated with it.
>>
>> By looking at the patch itself, this change seems ok to me, so I think
>> I will not block it, but I think Arm maintainers' approval might be needed
>> because of the discussion in [1], so I added them for their information.
>> If Arm maintainers do not object the change, you can have my release-ack.

I'll take the liberty then to translate this into an actual tag, with ...

> The P2M query type is so far ignored on Arm as we neither support 
> populate-on-demand nor memsharing.
> 
> I am assuming that if we ever introduce any those of features, we would 
> follow the same behavior as x86. So:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

... this now in place (thanks Julien).

Jan
Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Jan Beulich 1 year, 6 months ago 
On 25.10.2022 17:42, Roger Pau Monné wrote:
> On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
>> Not passing P2M_UNSHARE to get_page_from_gfn() means there won't even be
>> an attempt to unshare the referenced page, without any indication to the
>> caller (e.g. -EAGAIN). Note that guests have no direct control over
>> which of their pages are shared (or paged out), and hence they have no
>> way to make sure all on their own that the subsequent obtaining of a
>> writable type reference can actually succeed.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> Really I wonder whether the function wouldn't better use
>> check_get_page_from_gfn() _and_ permit p2m_ram_rw only. Otoh the P2M
>> type is stale by the time it is being looked at, so all depends on the
>> subsequent obtaining of a writable type reference anyway ...
>>
>> A similar issue then apparently exists in guest_wrmsr_xen() when writing
>> the hypercall page. Interestingly there p2m_is_paging() is being checked
>> for (but shared pages aren't).
> 
> Doesn't guest_wrmsr_xen() also needs to use UNSHARE?

I think so (hence I did say "A similar issue then apparently exists ...").
With the one caveat that a page that was already initialized as a hypercall
one (and was shared afterwards) wouldn't need to be unshared.

> I wonder if it would be helpful to introduce some kind of helper so
> that all functions can use it, get_guest_writable_page() or similar.

Maybe. Using check_get_page_from_gfn() would already help, I guess.

>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1484,7 +1484,7 @@ int map_vcpu_info(struct vcpu *v, unsign
>>      if ( (v != current) && !(v->pause_flags & VPF_down) )
>>          return -EINVAL;
>>  
>> -    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
>> +    page = get_page_from_gfn(d, gfn, NULL, P2M_UNSHARE);
> 
> I had to go look up that P2M_UNSHARE implies P2M_ALLOC for the users
> of the parameter, it would be helpful to add a comment in p2m.h that
> UNSHARE implies ALLOC.

Same here, plus I needed to further figure out that the same implication
missing on Arm is okay merely because they ignore the respective argument.

Jan
Re: [PATCH] common: map_vcpu_info() wants to unshare the underlying page
Posted by Roger Pau Monné 1 year, 6 months ago 
On Tue, Oct 25, 2022 at 06:04:49PM +0200, Jan Beulich wrote:
> On 25.10.2022 17:42, Roger Pau Monné wrote:
> > On Tue, Oct 11, 2022 at 10:48:38AM +0200, Jan Beulich wrote:
> >> --- a/xen/common/domain.c
> >> +++ b/xen/common/domain.c
> >> @@ -1484,7 +1484,7 @@ int map_vcpu_info(struct vcpu *v, unsign
> >>      if ( (v != current) && !(v->pause_flags & VPF_down) )
> >>          return -EINVAL;
> >>  
> >> -    page = get_page_from_gfn(d, gfn, NULL, P2M_ALLOC);
> >> +    page = get_page_from_gfn(d, gfn, NULL, P2M_UNSHARE);
> > 
> > I had to go look up that P2M_UNSHARE implies P2M_ALLOC for the users
> > of the parameter, it would be helpful to add a comment in p2m.h that
> > UNSHARE implies ALLOC.
> 
> Same here, plus I needed to further figure out that the same implication
> missing on Arm is okay merely because they ignore the respective argument.

... it's made worse by some callers using P2M_ALLOC | P2M_UNSHARE
which adds to the confusion.

Thanks, Roger.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.