| Message ID | 1ccb1524a08c3db2f59b7dae4d8377e1c98903c9.1579628566.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | VM forking | expand |
On 21.01.2020 18:49, Tamas K Lengyel wrote: > When plugging a hole in the target physmap don't use the access permission > returned by __get_gfn_type_access as it can be non-sensical, "can be" is too vague for my taste - it suggests there may also be cases where a sensible value is returned, and hence it should be used. Could you clarify this please? (The code change itself of course is simple and mechanical enough to look okay.) Jan > leading to > spurious vm_events being sent out for access violations at unexpected > locations. Make use of p2m->default_access instead. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > --- > xen/arch/x86/mm/mem_sharing.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c > index eac8047c07..e3ddb63b4f 100644 > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -1071,11 +1071,10 @@ int add_to_physmap(struct domain *sd, unsigned long sgfn, shr_handle_t sh, > p2m_type_t smfn_type, cmfn_type; > struct gfn_info *gfn_info; > struct p2m_domain *p2m = p2m_get_hostp2m(cd); > - p2m_access_t a; > struct two_gfns tg; > > get_two_gfns(sd, _gfn(sgfn), &smfn_type, NULL, &smfn, > - cd, _gfn(cgfn), &cmfn_type, &a, &cmfn, 0, &tg, lock); > + cd, _gfn(cgfn), &cmfn_type, NULL, &cmfn, 0, &tg, lock); > > /* Get the source shared page, check and lock */ > ret = XENMEM_SHARING_OP_S_HANDLE_INVALID; > @@ -1110,7 +1109,7 @@ int add_to_physmap(struct domain *sd, unsigned long sgfn, shr_handle_t sh, > } > > ret = p2m_set_entry(p2m, _gfn(cgfn), smfn, PAGE_ORDER_4K, > - p2m_ram_shared, a); > + p2m_ram_shared, p2m->default_access); > > /* Tempted to turn this into an assert */ > if ( ret ) >
On Wed, Jan 22, 2020 at 8:35 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 21.01.2020 18:49, Tamas K Lengyel wrote: > > When plugging a hole in the target physmap don't use the access permission > > returned by __get_gfn_type_access as it can be non-sensical, > > "can be" is too vague for my taste - it suggests there may also be cases > where a sensible value is returned, and hence it should be used. Could > you clarify this please? (The code change itself of course is simple and > mechanical enough to look okay.) Well, I can only speak of what I observed. The case seems to be that most of the time the function actually returns p2m_access_rwx (which is sensible), but occasionally something else. I didn't investigate where that value actually comes from, but when populating a physmap like this only the default_access is sane. Tamas
On 1/22/20 5:08 PM, Tamas K Lengyel wrote: > On Wed, Jan 22, 2020 at 8:35 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 21.01.2020 18:49, Tamas K Lengyel wrote: >>> When plugging a hole in the target physmap don't use the access permission >>> returned by __get_gfn_type_access as it can be non-sensical, >> >> "can be" is too vague for my taste - it suggests there may also be cases >> where a sensible value is returned, and hence it should be used. Could >> you clarify this please? (The code change itself of course is simple and >> mechanical enough to look okay.) > > Well, I can only speak of what I observed. The case seems to be that > most of the time the function actually returns p2m_access_rwx (which > is sensible), but occasionally something else. I didn't investigate > where that value actually comes from, but when populating a physmap > like this only the default_access is sane. It would be good to get to the bottom of this. Is it possible that your dom0 agent (or whatever it's called) is calling add_to_physmap() on gfns that have already been populated? Is that something you want to catch? -George
On Thu, Jan 23, 2020 at 9:44 AM George Dunlap <george.dunlap@citrix.com> wrote: > > On 1/22/20 5:08 PM, Tamas K Lengyel wrote: > > On Wed, Jan 22, 2020 at 8:35 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 21.01.2020 18:49, Tamas K Lengyel wrote: > >>> When plugging a hole in the target physmap don't use the access permission > >>> returned by __get_gfn_type_access as it can be non-sensical, > >> > >> "can be" is too vague for my taste - it suggests there may also be cases > >> where a sensible value is returned, and hence it should be used. Could > >> you clarify this please? (The code change itself of course is simple and > >> mechanical enough to look okay.) > > > > Well, I can only speak of what I observed. The case seems to be that > > most of the time the function actually returns p2m_access_rwx (which > > is sensible), but occasionally something else. I didn't investigate > > where that value actually comes from, but when populating a physmap > > like this only the default_access is sane. > > It would be good to get to the bottom of this. Is it possible that your > dom0 agent (or whatever it's called) is calling add_to_physmap() on gfns > that have already been populated? Is that something you want to catch? > OK, I went back and deciphered why sometimes I saw different permissions here. In the context I ran into this issue there is no dom0 agent calling add_to_physmap. We wind up in this path with a VM fork where the MMU faulted. The fault handler is trying to determine whether the page needs to be forked from its parent: populated with a shared entry if it's R/X access, or deduplicated if it's a W. This forking only actually happens if the page type that is returned by ept_get_entry is of a hole type. When it's a hole type, ept_get_entry always returns p2m_access_n as the access permission. Copying that access permission to the newly populated entry is bad - that's what this patch fixes. But this path also gets hit when the MMU faults for other reasons. In those cases will get permissions other then p2m_access_n since the type is not a hole. But when it's not a hole, this function bails as that's a clear signal that the page doesn't need forking. So I was seeing p2m_access_rwx permission for page accesses that triggered the MMU fault for reasons other then mem_access. For example, when a previously shared entry needs unsharing. So there is no mystery after all, I was just printing my debug lines with the mem_access permissions irrespective of the page type before the path bails due to the type check. Tamas
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index eac8047c07..e3ddb63b4f 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1071,11 +1071,10 @@ int add_to_physmap(struct domain *sd, unsigned long sgfn, shr_handle_t sh, p2m_type_t smfn_type, cmfn_type; struct gfn_info *gfn_info; struct p2m_domain *p2m = p2m_get_hostp2m(cd); - p2m_access_t a; struct two_gfns tg; get_two_gfns(sd, _gfn(sgfn), &smfn_type, NULL, &smfn, - cd, _gfn(cgfn), &cmfn_type, &a, &cmfn, 0, &tg, lock); + cd, _gfn(cgfn), &cmfn_type, NULL, &cmfn, 0, &tg, lock); /* Get the source shared page, check and lock */ ret = XENMEM_SHARING_OP_S_HANDLE_INVALID; @@ -1110,7 +1109,7 @@ int add_to_physmap(struct domain *sd, unsigned long sgfn, shr_handle_t sh, } ret = p2m_set_entry(p2m, _gfn(cgfn), smfn, PAGE_ORDER_4K, - p2m_ram_shared, a); + p2m_ram_shared, p2m->default_access); /* Tempted to turn this into an assert */ if ( ret )
When plugging a hole in the target physmap don't use the access permission returned by __get_gfn_type_access as it can be non-sensical, leading to spurious vm_events being sent out for access violations at unexpected locations. Make use of p2m->default_access instead. Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> --- xen/arch/x86/mm/mem_sharing.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)