| Message ID | ae2142231342bfc6fb9731303130a2c0fa381576.1580148227.git.tamas.lengyel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 27.01.2020 19:06, 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, leading to > spurious vm_events being sent out for access violations at unexpected > locations. Make use of p2m->default_access instead. As before, to me "can be non-sensical" is insufficient as a reason here. If it can also be a "good" value, it still remains unclear why in that case p2m->default_access is nevertheless the right value to use. Jan
On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 27.01.2020 19:06, 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, leading to > > spurious vm_events being sent out for access violations at unexpected > > locations. Make use of p2m->default_access instead. > > As before, to me "can be non-sensical" is insufficient as a reason > here. If it can also be a "good" value, it still remains unclear > why in that case p2m->default_access is nevertheless the right > value to use. I have already explained in the previous version of the patch why I said "can be". Forgot to change the commit message from "can be" to "is". Tamas
On 28.01.2020 18:02, Tamas K Lengyel wrote: > On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 27.01.2020 19:06, 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, leading to >>> spurious vm_events being sent out for access violations at unexpected >>> locations. Make use of p2m->default_access instead. >> >> As before, to me "can be non-sensical" is insufficient as a reason >> here. If it can also be a "good" value, it still remains unclear >> why in that case p2m->default_access is nevertheless the right >> value to use. > > I have already explained in the previous version of the patch why I > said "can be". Forgot to change the commit message from "can be" to > "is". Changing just the commit message would be easy while committing. But even with the change I would ask why this is. Looking at ept_get_entry() (and assuming p2m_pt_get_entry() will work similarly, minus the p2m_access_t which can't come out of the PTE just yet), I see if ( is_epte_valid(ept_entry) ) { *t = p2m_recalc_type(recalc || ept_entry->recalc, ept_entry->sa_p2mt, p2m, gfn); *a = ept_entry->access; near its end. Which means even a hole can have its access field set. So it's still not clear to me from the description why p2m->default_access is uniformly the value to use. Wouldn't you rather want to override the original value only if it's p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not paged-out pages)? Of course then the question is whether there wouldn't be an ambiguity with p2m_access_n having got set explicitly on the page. But maybe this is impossible for p2m_invalid / p2m_mmio_dm? Jan
On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 28.01.2020 18:02, Tamas K Lengyel wrote: > > On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 27.01.2020 19:06, 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, leading to > >>> spurious vm_events being sent out for access violations at unexpected > >>> locations. Make use of p2m->default_access instead. > >> > >> As before, to me "can be non-sensical" is insufficient as a reason > >> here. If it can also be a "good" value, it still remains unclear > >> why in that case p2m->default_access is nevertheless the right > >> value to use. > > > > I have already explained in the previous version of the patch why I > > said "can be". Forgot to change the commit message from "can be" to > > "is". > > Changing just the commit message would be easy while committing. > But even with the change I would ask why this is. Looking at > ept_get_entry() (and assuming p2m_pt_get_entry() will work > similarly, minus the p2m_access_t which can't come out of the > PTE just yet), I see > > if ( is_epte_valid(ept_entry) ) > { > *t = p2m_recalc_type(recalc || ept_entry->recalc, > ept_entry->sa_p2mt, p2m, gfn); > *a = ept_entry->access; > > near its end. Which means even a hole can have its access field > set. So it's still not clear to me from the description why > p2m->default_access is uniformly the value to use. Wouldn't you > rather want to override the original value only if it's > p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not > paged-out pages)? At this point I would just rather state that add_to_physmap only works on actual holes, not with paged-out pages. In fact, I would like to see mem_paging being dropped from the codebase entirely since it's been abandoned for years and noone expressing any interest in keeping it. In the interim I would rather not spend unnecessary cycles on speculating about potential corner-cases of mem_paging when noone actually uses it. > Of course then the question is whether there > wouldn't be an ambiguity with p2m_access_n having got set > explicitly on the page. But maybe this is impossible for > p2m_invalid / p2m_mmio_dm? As far as mem_access permissions go, I don't know of any usecase that would set mem_access permission on a hole even if by looks of it it is technically possible. At this point I would rather just put this corner-case's description in a comment. Tamas
On Wed, Jan 29, 2020 at 7:05 AM Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote: > > On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@suse.com> wrote: > > > > On 28.01.2020 18:02, Tamas K Lengyel wrote: > > > On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: > > >> > > >> On 27.01.2020 19:06, 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, leading to > > >>> spurious vm_events being sent out for access violations at unexpected > > >>> locations. Make use of p2m->default_access instead. > > >> > > >> As before, to me "can be non-sensical" is insufficient as a reason > > >> here. If it can also be a "good" value, it still remains unclear > > >> why in that case p2m->default_access is nevertheless the right > > >> value to use. > > > > > > I have already explained in the previous version of the patch why I > > > said "can be". Forgot to change the commit message from "can be" to > > > "is". > > > > Changing just the commit message would be easy while committing. > > But even with the change I would ask why this is. Looking at > > ept_get_entry() (and assuming p2m_pt_get_entry() will work > > similarly, minus the p2m_access_t which can't come out of the > > PTE just yet), I see > > > > if ( is_epte_valid(ept_entry) ) > > { > > *t = p2m_recalc_type(recalc || ept_entry->recalc, > > ept_entry->sa_p2mt, p2m, gfn); > > *a = ept_entry->access; > > > > near its end. Which means even a hole can have its access field > > set. So it's still not clear to me from the description why > > p2m->default_access is uniformly the value to use. Wouldn't you > > rather want to override the original value only if it's > > p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not > > paged-out pages)? > > At this point I would just rather state that add_to_physmap only works > on actual holes, not with paged-out pages. In fact, I would like to > see mem_paging being dropped from the codebase entirely since it's > been abandoned for years and noone expressing any interest in keeping > it. In the interim I would rather not spend unnecessary cycles on > speculating about potential corner-cases of mem_paging when noone > actually uses it. > > > Of course then the question is whether there > > wouldn't be an ambiguity with p2m_access_n having got set > > explicitly on the page. But maybe this is impossible for > > p2m_invalid / p2m_mmio_dm? > > As far as mem_access permissions go, I don't know of any usecase that > would set mem_access permission on a hole even if by looks of it it is > technically possible. At this point I would rather just put this > corner-case's description in a comment. A potential solution for this - if one would need it in the future - would be to add another VM event type that Xen can use to alert the toolstack that a pre-existing mem_access permission is being overwritten by forking. That would allow the toolstack to reset the permission if it wants to. But honestly, I think it's a lot of code for a situation that I don't expect anyone will ever run into. Let's just document it and move on. Tamas
On 29.01.2020 15:05, Tamas K Lengyel wrote: > On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 28.01.2020 18:02, Tamas K Lengyel wrote: >>> On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 27.01.2020 19:06, 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, leading to >>>>> spurious vm_events being sent out for access violations at unexpected >>>>> locations. Make use of p2m->default_access instead. >>>> >>>> As before, to me "can be non-sensical" is insufficient as a reason >>>> here. If it can also be a "good" value, it still remains unclear >>>> why in that case p2m->default_access is nevertheless the right >>>> value to use. >>> >>> I have already explained in the previous version of the patch why I >>> said "can be". Forgot to change the commit message from "can be" to >>> "is". >> >> Changing just the commit message would be easy while committing. >> But even with the change I would ask why this is. Looking at >> ept_get_entry() (and assuming p2m_pt_get_entry() will work >> similarly, minus the p2m_access_t which can't come out of the >> PTE just yet), I see >> >> if ( is_epte_valid(ept_entry) ) >> { >> *t = p2m_recalc_type(recalc || ept_entry->recalc, >> ept_entry->sa_p2mt, p2m, gfn); >> *a = ept_entry->access; >> >> near its end. Which means even a hole can have its access field >> set. So it's still not clear to me from the description why >> p2m->default_access is uniformly the value to use. Wouldn't you >> rather want to override the original value only if it's >> p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not >> paged-out pages)? > > At this point I would just rather state that add_to_physmap only works > on actual holes, not with paged-out pages. In fact, I would like to > see mem_paging being dropped from the codebase entirely since it's > been abandoned for years and noone expressing any interest in keeping > it. In the interim I would rather not spend unnecessary cycles on > speculating about potential corner-cases of mem_paging when noone > actually uses it. > >> Of course then the question is whether there >> wouldn't be an ambiguity with p2m_access_n having got set >> explicitly on the page. But maybe this is impossible for >> p2m_invalid / p2m_mmio_dm? > > As far as mem_access permissions go, I don't know of any usecase that > would set mem_access permission on a hole even if by looks of it it is > technically possible. At this point I would rather just put this > corner-case's description in a comment. I think I would ack a revised patch having this properly explained. Jan
On Wed, Jan 29, 2020 at 7:44 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 29.01.2020 15:05, Tamas K Lengyel wrote: > > On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@suse.com> wrote: > >> > >> On 28.01.2020 18:02, Tamas K Lengyel wrote: > >>> On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: > >>>> > >>>> On 27.01.2020 19:06, 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, leading to > >>>>> spurious vm_events being sent out for access violations at unexpected > >>>>> locations. Make use of p2m->default_access instead. > >>>> > >>>> As before, to me "can be non-sensical" is insufficient as a reason > >>>> here. If it can also be a "good" value, it still remains unclear > >>>> why in that case p2m->default_access is nevertheless the right > >>>> value to use. > >>> > >>> I have already explained in the previous version of the patch why I > >>> said "can be". Forgot to change the commit message from "can be" to > >>> "is". > >> > >> Changing just the commit message would be easy while committing. > >> But even with the change I would ask why this is. Looking at > >> ept_get_entry() (and assuming p2m_pt_get_entry() will work > >> similarly, minus the p2m_access_t which can't come out of the > >> PTE just yet), I see > >> > >> if ( is_epte_valid(ept_entry) ) > >> { > >> *t = p2m_recalc_type(recalc || ept_entry->recalc, > >> ept_entry->sa_p2mt, p2m, gfn); > >> *a = ept_entry->access; > >> > >> near its end. Which means even a hole can have its access field > >> set. So it's still not clear to me from the description why > >> p2m->default_access is uniformly the value to use. Wouldn't you > >> rather want to override the original value only if it's > >> p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not > >> paged-out pages)? > > > > At this point I would just rather state that add_to_physmap only works > > on actual holes, not with paged-out pages. In fact, I would like to > > see mem_paging being dropped from the codebase entirely since it's > > been abandoned for years and noone expressing any interest in keeping > > it. In the interim I would rather not spend unnecessary cycles on > > speculating about potential corner-cases of mem_paging when noone > > actually uses it. > > > >> Of course then the question is whether there > >> wouldn't be an ambiguity with p2m_access_n having got set > >> explicitly on the page. But maybe this is impossible for > >> p2m_invalid / p2m_mmio_dm? > > > > As far as mem_access permissions go, I don't know of any usecase that > > would set mem_access permission on a hole even if by looks of it it is > > technically possible. At this point I would rather just put this > > corner-case's description in a comment. > > I think I would ack a revised patch having this properly explained. That's fine, I'll add some comments to this effect and reword the commit message. Tamas
On Wed, Jan 29, 2020 at 7:56 AM Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote: > > On Wed, Jan 29, 2020 at 7:44 AM Jan Beulich <jbeulich@suse.com> wrote: > > > > On 29.01.2020 15:05, Tamas K Lengyel wrote: > > > On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@suse.com> wrote: > > >> > > >> On 28.01.2020 18:02, Tamas K Lengyel wrote: > > >>> On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: > > >>>> > > >>>> On 27.01.2020 19:06, 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, leading to > > >>>>> spurious vm_events being sent out for access violations at unexpected > > >>>>> locations. Make use of p2m->default_access instead. > > >>>> > > >>>> As before, to me "can be non-sensical" is insufficient as a reason > > >>>> here. If it can also be a "good" value, it still remains unclear > > >>>> why in that case p2m->default_access is nevertheless the right > > >>>> value to use. > > >>> > > >>> I have already explained in the previous version of the patch why I > > >>> said "can be". Forgot to change the commit message from "can be" to > > >>> "is". > > >> > > >> Changing just the commit message would be easy while committing. > > >> But even with the change I would ask why this is. Looking at > > >> ept_get_entry() (and assuming p2m_pt_get_entry() will work > > >> similarly, minus the p2m_access_t which can't come out of the > > >> PTE just yet), I see > > >> > > >> if ( is_epte_valid(ept_entry) ) > > >> { > > >> *t = p2m_recalc_type(recalc || ept_entry->recalc, > > >> ept_entry->sa_p2mt, p2m, gfn); > > >> *a = ept_entry->access; > > >> > > >> near its end. Which means even a hole can have its access field > > >> set. So it's still not clear to me from the description why > > >> p2m->default_access is uniformly the value to use. Wouldn't you > > >> rather want to override the original value only if it's > > >> p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not > > >> paged-out pages)? > > > > > > At this point I would just rather state that add_to_physmap only works > > > on actual holes, not with paged-out pages. In fact, I would like to > > > see mem_paging being dropped from the codebase entirely since it's > > > been abandoned for years and noone expressing any interest in keeping > > > it. In the interim I would rather not spend unnecessary cycles on > > > speculating about potential corner-cases of mem_paging when noone > > > actually uses it. > > > > > >> Of course then the question is whether there > > >> wouldn't be an ambiguity with p2m_access_n having got set > > >> explicitly on the page. But maybe this is impossible for > > >> p2m_invalid / p2m_mmio_dm? > > > > > > As far as mem_access permissions go, I don't know of any usecase that > > > would set mem_access permission on a hole even if by looks of it it is > > > technically possible. At this point I would rather just put this > > > corner-case's description in a comment. > > > > I think I would ack a revised patch having this properly explained. > > That's fine, I'll add some comments to this effect and reword the > commit message. > Actually, looking at the implementation of p2m_set_mem_access it's not clear to me whether we can even have a hole with a mem_access permission set. Can you have a "hole" type with a valid gfn? If not, this is a non-issue since p2m_set_mem_access only sets mem_access permissions that pass !gfn_eq(gfn, INVALID_GFN). Tamas
On Wed, Jan 29, 2020 at 9:09 AM Tamas K Lengyel <tamas.k.lengyel@gmail.com> wrote: > > On Wed, Jan 29, 2020 at 7:56 AM Tamas K Lengyel > <tamas.k.lengyel@gmail.com> wrote: > > > > On Wed, Jan 29, 2020 at 7:44 AM Jan Beulich <jbeulich@suse.com> wrote: > > > > > > On 29.01.2020 15:05, Tamas K Lengyel wrote: > > > > On Wed, Jan 29, 2020 at 6:27 AM Jan Beulich <jbeulich@suse.com> wrote: > > > >> > > > >> On 28.01.2020 18:02, Tamas K Lengyel wrote: > > > >>> On Tue, Jan 28, 2020 at 9:56 AM Jan Beulich <jbeulich@suse.com> wrote: > > > >>>> > > > >>>> On 27.01.2020 19:06, 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, leading to > > > >>>>> spurious vm_events being sent out for access violations at unexpected > > > >>>>> locations. Make use of p2m->default_access instead. > > > >>>> > > > >>>> As before, to me "can be non-sensical" is insufficient as a reason > > > >>>> here. If it can also be a "good" value, it still remains unclear > > > >>>> why in that case p2m->default_access is nevertheless the right > > > >>>> value to use. > > > >>> > > > >>> I have already explained in the previous version of the patch why I > > > >>> said "can be". Forgot to change the commit message from "can be" to > > > >>> "is". > > > >> > > > >> Changing just the commit message would be easy while committing. > > > >> But even with the change I would ask why this is. Looking at > > > >> ept_get_entry() (and assuming p2m_pt_get_entry() will work > > > >> similarly, minus the p2m_access_t which can't come out of the > > > >> PTE just yet), I see > > > >> > > > >> if ( is_epte_valid(ept_entry) ) > > > >> { > > > >> *t = p2m_recalc_type(recalc || ept_entry->recalc, > > > >> ept_entry->sa_p2mt, p2m, gfn); > > > >> *a = ept_entry->access; > > > >> > > > >> near its end. Which means even a hole can have its access field > > > >> set. So it's still not clear to me from the description why > > > >> p2m->default_access is uniformly the value to use. Wouldn't you > > > >> rather want to override the original value only if it's > > > >> p2m_access_n together with p2m_invalid or p2m_mmio_dm (but not > > > >> paged-out pages)? > > > > > > > > At this point I would just rather state that add_to_physmap only works > > > > on actual holes, not with paged-out pages. In fact, I would like to > > > > see mem_paging being dropped from the codebase entirely since it's > > > > been abandoned for years and noone expressing any interest in keeping > > > > it. In the interim I would rather not spend unnecessary cycles on > > > > speculating about potential corner-cases of mem_paging when noone > > > > actually uses it. > > > > > > > >> Of course then the question is whether there > > > >> wouldn't be an ambiguity with p2m_access_n having got set > > > >> explicitly on the page. But maybe this is impossible for > > > >> p2m_invalid / p2m_mmio_dm? > > > > > > > > As far as mem_access permissions go, I don't know of any usecase that > > > > would set mem_access permission on a hole even if by looks of it it is > > > > technically possible. At this point I would rather just put this > > > > corner-case's description in a comment. > > > > > > I think I would ack a revised patch having this properly explained. > > > > That's fine, I'll add some comments to this effect and reword the > > commit message. > > > > Actually, looking at the implementation of p2m_set_mem_access it's not > clear to me whether we can even have a hole with a mem_access > permission set. Can you have a "hole" type with a valid gfn? If not, > this is a non-issue since p2m_set_mem_access only sets mem_access > permissions that pass !gfn_eq(gfn, INVALID_GFN). Never mind, of course gfn can be anything (ie valid) since it's not tied to whether the entry actually exists or not. Tamas
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index 2b3be5b125..52b139c1bb 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(-)