* [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 @ 2021-09-13 6:39 Jan Beulich 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Jan Beulich @ 2021-09-13 6:39 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu I'm prepared for the "how" aspect of the 1st patch here to end up controversial. Since the observed quirk will imo want dealing with, I'd appreciate any objection to the proposed change to be accompanied by an alternative suggestion. An intention of mine was to not further increase the number of arch hooks needed. I further realize that this change conflicts with Oleksandr's "xen/gnttab: Store frame GFN in struct page_info on Arm", at the very least contextually. 1: gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2: memory: XENMEM_add_to_physmap (almost) wrapping checks Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-13 6:39 [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich @ 2021-09-13 6:41 ` Jan Beulich 2021-09-20 10:20 ` Roger Pau Monné ` (2 more replies) 2021-09-13 6:42 ` [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich 2021-09-27 20:41 ` [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Oleksandr 2 siblings, 3 replies; 26+ messages in thread From: Jan Beulich @ 2021-09-13 6:41 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Roger Pau Monné Without holding appropriate locks, attempting to remove a prior mapping of the underlying page is pointless, as the same (or another) mapping could be re-established by a parallel request on another vCPU. Move the code to Arm's gnttab_set_frame_gfn(). Of course this new placement doesn't improve things in any way as far as the security of grant status frame mappings goes (see XSA-379). Proper locking would be needed here to allow status frames to be mapped securely. In turn this then requires replacing the other use in gnttab_unpopulate_status_frames(), which yet in turn requires adjusting x86's gnttab_set_frame_gfn(). Note that with proper locking inside guest_physmap_remove_page() combined with checking the GFN's mapping there against the passed in MFN, there then is no issue with the involved multiple gnttab_set_frame_gfn()-s potentially returning varying values (due to a racing XENMAPSPACE_grant_table request). This, as a side effect, does away with gnttab_map_frame() having a local variable "gfn" which shadows a function parameter of the same name. Together with XSA-379 this points out that XSA-255's addition to gnttab_map_frame() was really useless. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -92,7 +92,7 @@ struct grant_table { struct radix_tree_root maptrack_tree; /* Domain to which this struct grant_table belongs. */ - const struct domain *domain; + struct domain *domain; struct grant_table_arch arch; }; @@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d { int rc = gfn_eq(gfn, INVALID_GFN) ? 0 - : guest_physmap_remove_page(d, gfn, - page_to_mfn(pg), 0); + : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN, + page_to_mfn(pg)); if ( rc ) { @@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d domain_crash(d); return rc; } - gnttab_set_frame_gfn(gt, true, i, INVALID_GFN); } BUG_ON(page_get_owner(pg) != d); @@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u struct grant_table *gt = d->grant_table; bool status = false; + if ( gfn_eq(gfn, INVALID_GFN) ) + return -EINVAL; + grant_write_lock(gt); if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) ) @@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u else rc = gnttab_get_shared_frame_mfn(d, idx, mfn); - if ( !rc && paging_mode_translate(d) ) - { - gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx); - - if ( !gfn_eq(gfn, INVALID_GFN) ) - rc = guest_physmap_remove_page(d, gfn, *mfn, 0); - } - if ( !rc ) { + struct page_info *pg = mfn_to_page(*mfn); + /* * Make sure gnttab_unpopulate_status_frames() won't (successfully) * free the page until our caller has completed its operation. */ - if ( get_page(mfn_to_page(*mfn), d) ) - gnttab_set_frame_gfn(gt, status, idx, gfn); - else + if ( !get_page(pg, d) ) rc = -EBUSY; + else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) ) + put_page(pg); } grant_write_unlock(gt); --- a/xen/include/asm-arm/grant_table.h +++ b/xen/include/asm-arm/grant_table.h @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned XFREE((gt)->arch.status_gfn); \ } while ( 0 ) -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ - do { \ - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ - (gfn); \ - } while ( 0 ) +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ + ({ \ + int rc_ = 0; \ + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ + (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \ + 0)) == 0 ) \ + ((st) ? (gt)->arch.status_gfn \ + : (gt)->arch.shared_gfn)[idx] = (gfn); \ + rc_; \ + }) #define gnttab_get_frame_gfn(gt, st, idx) ({ \ (st) ? gnttab_status_gfn(NULL, gt, idx) \ --- a/xen/include/asm-x86/grant_table.h +++ b/xen/include/asm-x86/grant_table.h @@ -37,7 +37,12 @@ static inline int replace_grant_host_map #define gnttab_init_arch(gt) 0 #define gnttab_destroy_arch(gt) do {} while ( 0 ) -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 ) +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ + (gfn_eq(gfn, INVALID_GFN) \ + ? guest_physmap_remove_page((gt)->domain, \ + gnttab_get_frame_gfn(gt, st, idx), \ + mfn, 0) \ + : 0 /* Handled in add_to_physmap_one(). */) #define gnttab_get_frame_gfn(gt, st, idx) ({ \ mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx) \ : gnttab_shared_mfn(gt, idx); \ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich @ 2021-09-20 10:20 ` Roger Pau Monné 2021-09-20 15:27 ` Jan Beulich 2021-11-25 16:37 ` Julien Grall 2021-12-02 9:09 ` Julien Grall 2 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monné @ 2021-09-20 10:20 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: > Without holding appropriate locks, attempting to remove a prior mapping > of the underlying page is pointless, as the same (or another) mapping > could be re-established by a parallel request on another vCPU. Move the > code to Arm's gnttab_set_frame_gfn(). Of course this new placement > doesn't improve things in any way as far as the security of grant status > frame mappings goes (see XSA-379). Proper locking would be needed here > to allow status frames to be mapped securely. > > In turn this then requires replacing the other use in > gnttab_unpopulate_status_frames(), which yet in turn requires adjusting > x86's gnttab_set_frame_gfn(). Note that with proper locking inside > guest_physmap_remove_page() combined with checking the GFN's mapping > there against the passed in MFN, there then is no issue with the > involved multiple gnttab_set_frame_gfn()-s potentially returning varying > values (due to a racing XENMAPSPACE_grant_table request). > > This, as a side effect, does away with gnttab_map_frame() having a local > variable "gfn" which shadows a function parameter of the same name. > > Together with XSA-379 this points out that XSA-255's addition to > gnttab_map_frame() was really useless. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -92,7 +92,7 @@ struct grant_table { > struct radix_tree_root maptrack_tree; > > /* Domain to which this struct grant_table belongs. */ > - const struct domain *domain; > + struct domain *domain; > > struct grant_table_arch arch; > }; > @@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d > { > int rc = gfn_eq(gfn, INVALID_GFN) > ? 0 > - : guest_physmap_remove_page(d, gfn, > - page_to_mfn(pg), 0); > + : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN, > + page_to_mfn(pg)); > > if ( rc ) > { > @@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d > domain_crash(d); > return rc; > } > - gnttab_set_frame_gfn(gt, true, i, INVALID_GFN); > } > > BUG_ON(page_get_owner(pg) != d); > @@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u > struct grant_table *gt = d->grant_table; > bool status = false; > > + if ( gfn_eq(gfn, INVALID_GFN) ) > + return -EINVAL; > + > grant_write_lock(gt); > > if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) ) > @@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u > else > rc = gnttab_get_shared_frame_mfn(d, idx, mfn); > > - if ( !rc && paging_mode_translate(d) ) > - { > - gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx); > - > - if ( !gfn_eq(gfn, INVALID_GFN) ) > - rc = guest_physmap_remove_page(d, gfn, *mfn, 0); > - } > - > if ( !rc ) > { > + struct page_info *pg = mfn_to_page(*mfn); > + > /* > * Make sure gnttab_unpopulate_status_frames() won't (successfully) > * free the page until our caller has completed its operation. > */ > - if ( get_page(mfn_to_page(*mfn), d) ) > - gnttab_set_frame_gfn(gt, status, idx, gfn); > - else > + if ( !get_page(pg, d) ) > rc = -EBUSY; > + else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) ) > + put_page(pg); > } > > grant_write_unlock(gt); > --- a/xen/include/asm-arm/grant_table.h > +++ b/xen/include/asm-arm/grant_table.h > @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned > XFREE((gt)->arch.status_gfn); \ > } while ( 0 ) > > -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ > - do { \ > - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ > - (gfn); \ > - } while ( 0 ) > +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ > + ({ \ > + int rc_ = 0; \ > + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ Newline maybe? Not sure whether we decided that macros should also follow coding style regarding variable definition followed by newline. > + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ I'm slightly confused by this checks, don't you need to check for gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call guest_physmap_remove_page? Or assuming that ogfn is not invalid can be used to imply a removal? Also the check for ogfn == gfn is only used on Arm, while I would assume a similar one would be needed on x86 to guarantee the same behavior? Thanks, Roger. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-20 10:20 ` Roger Pau Monné @ 2021-09-20 15:27 ` Jan Beulich 2021-09-21 8:32 ` Roger Pau Monné 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-09-20 15:27 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu On 20.09.2021 12:20, Roger Pau Monné wrote: > On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >> --- a/xen/include/asm-arm/grant_table.h >> +++ b/xen/include/asm-arm/grant_table.h >> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned >> XFREE((gt)->arch.status_gfn); \ >> } while ( 0 ) >> >> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ >> - do { \ >> - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ >> - (gfn); \ >> - } while ( 0 ) >> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ >> + ({ \ >> + int rc_ = 0; \ >> + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ > > Newline maybe? Not sure whether we decided that macros should also > follow coding style regarding variable definition followed by newline. To be honest in macros I'm always on the fence: A long line of all blanks and just a trailing backslash is about as ugly to me as the missing separator. I think normally I opt for the way chosen above, but I'm not going to claim to know I'm consistent with this. >> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ > > I'm slightly confused by this checks, don't you need to check for > gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call > guest_physmap_remove_page? Why? It's ogfn which gets passed to the function. And it indeed is the prior GFN's mapping that we want to remove here. > Or assuming that ogfn is not invalid can be used to imply a removal? That implication can be (and on x86 is) used for the incoming argument, i.e. "gfn". I don't think "ogfn" can serve this purpose. > Also the check for ogfn == gfn is only used on Arm, while I would > assume a similar one would be needed on x86 to guarantee the same > behavior? Things are sufficiently different on Arm and x86. As said above, on x86 I'm indeed using "gfn" being INVALID_GFN as an indication that a removal is requested. This is simply transforming the behavior from prior to this change, with the function invocation moved into the per-arch macros. It may be relevant to note that gnttab_unpopulate_status_frames() calls gnttab_set_frame_gfn() with INVALID_GFN only when gnttab_get_frame_gfn() didn't return INVALID_GFN, i.e. the gnttab_get_frame_gfn() used in gnttab_set_frame_gfn() won't return this value either (we've not dropped any locks in between). And the only other caller of gnttab_set_frame_gfn() guarantees "gfn" not to be INVALID_GFN. A little fragile (towards hypothetical further callers of the macro/ function), yes, but I couldn't think of anything substantially better. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-20 15:27 ` Jan Beulich @ 2021-09-21 8:32 ` Roger Pau Monné 2021-09-21 10:12 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monné @ 2021-09-21 8:32 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: > On 20.09.2021 12:20, Roger Pau Monné wrote: > > On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: > >> --- a/xen/include/asm-arm/grant_table.h > >> +++ b/xen/include/asm-arm/grant_table.h > >> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ > > > > I'm slightly confused by this checks, don't you need to check for > > gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call > > guest_physmap_remove_page? > > Why? It's ogfn which gets passed to the function. And it indeed is the > prior GFN's mapping that we want to remove here. > > > Or assuming that ogfn is not invalid can be used to imply a removal? > > That implication can be (and on x86 is) used for the incoming argument, > i.e. "gfn". I don't think "ogfn" can serve this purpose. I guess I'm confused due to the ogfn checks done on the Arm side that are not performed on x86. So on Arm you always need to explicitly unhook the previous GFN before attempting to setup a new mapping, while on x86 you only need to do this when it's a removal in order to clear the entry? So you are effectively only removing the call to guest_physmap_remove_page in gnttab_map_frame for x86, because Arm will still perform it in gnttab_set_frame_gfn. This seems like a limitation of Arm's guest_physmap_add_entry. Thanks, Roger. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-21 8:32 ` Roger Pau Monné @ 2021-09-21 10:12 ` Jan Beulich 2021-09-22 9:26 ` Roger Pau Monné 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-09-21 10:12 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu On 21.09.2021 10:32, Roger Pau Monné wrote: > On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: >> On 20.09.2021 12:20, Roger Pau Monné wrote: >>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >>>> --- a/xen/include/asm-arm/grant_table.h >>>> +++ b/xen/include/asm-arm/grant_table.h >>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>> >>> I'm slightly confused by this checks, don't you need to check for >>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call >>> guest_physmap_remove_page? >> >> Why? It's ogfn which gets passed to the function. And it indeed is the >> prior GFN's mapping that we want to remove here. >> >>> Or assuming that ogfn is not invalid can be used to imply a removal? >> >> That implication can be (and on x86 is) used for the incoming argument, >> i.e. "gfn". I don't think "ogfn" can serve this purpose. > > I guess I'm confused due to the ogfn checks done on the Arm side that > are not performed on x86. So on Arm you always need to explicitly > unhook the previous GFN before attempting to setup a new mapping, > while on x86 you only need to do this when it's a removal in order to > clear the entry? The difference isn't with guest_physmap_add_entry() (both x86 and Arm only insert a new mapping there), but with xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior mappings. And gnttab_map_frame() gets called only from there. This is effectively what the first paragraph of the description is about. > So you are effectively only removing the call to > guest_physmap_remove_page in gnttab_map_frame for x86, because Arm > will still perform it in gnttab_set_frame_gfn. Yes. > This seems like a limitation of Arm's guest_physmap_add_entry. As per above I'm viewing this as a limitation of Arm's xenmem_add_to_physmap_one(). Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-21 10:12 ` Jan Beulich @ 2021-09-22 9:26 ` Roger Pau Monné 2021-09-22 9:42 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Roger Pau Monné @ 2021-09-22 9:26 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: > On 21.09.2021 10:32, Roger Pau Monné wrote: > > On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: > >> On 20.09.2021 12:20, Roger Pau Monné wrote: > >>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: > >>>> --- a/xen/include/asm-arm/grant_table.h > >>>> +++ b/xen/include/asm-arm/grant_table.h > >>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ > >>> > >>> I'm slightly confused by this checks, don't you need to check for > >>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call > >>> guest_physmap_remove_page? > >> > >> Why? It's ogfn which gets passed to the function. And it indeed is the > >> prior GFN's mapping that we want to remove here. > >> > >>> Or assuming that ogfn is not invalid can be used to imply a removal? > >> > >> That implication can be (and on x86 is) used for the incoming argument, > >> i.e. "gfn". I don't think "ogfn" can serve this purpose. > > > > I guess I'm confused due to the ogfn checks done on the Arm side that > > are not performed on x86. So on Arm you always need to explicitly > > unhook the previous GFN before attempting to setup a new mapping, > > while on x86 you only need to do this when it's a removal in order to > > clear the entry? > > The difference isn't with guest_physmap_add_entry() (both x86 and > Arm only insert a new mapping there), but with > xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior > mappings. And gnttab_map_frame() gets called only from there. This > is effectively what the first paragraph of the description is about. OK, sorry, it wasn't clear to me from the description. Could you explicitly mention in the description that the removal is moved into gnttab_set_frame_gfn on Arm in order to cope with the fact that xenmem_add_to_physmap_one doesn't perform it. TBH I think it would be in our best interest to try to make xenmem_add_to_physmap_one behave as close as possible between arches. This discrepancy between x86 and Arm regarding page removal is just going to bring more trouble in the long term, and hiding the differences inside gnttab_set_frame_gfn just makes it even more obscure. Thanks, Roger. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-22 9:26 ` Roger Pau Monné @ 2021-09-22 9:42 ` Jan Beulich 2021-09-22 10:00 ` Roger Pau Monné 2021-09-22 10:28 ` Julien Grall 0 siblings, 2 replies; 26+ messages in thread From: Jan Beulich @ 2021-09-22 9:42 UTC (permalink / raw) To: Roger Pau Monné, Julien Grall, Stefano Stabellini Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu On 22.09.2021 11:26, Roger Pau Monné wrote: > On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: >> On 21.09.2021 10:32, Roger Pau Monné wrote: >>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: >>>> On 20.09.2021 12:20, Roger Pau Monné wrote: >>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >>>>>> --- a/xen/include/asm-arm/grant_table.h >>>>>> +++ b/xen/include/asm-arm/grant_table.h >>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>>>> >>>>> I'm slightly confused by this checks, don't you need to check for >>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call >>>>> guest_physmap_remove_page? >>>> >>>> Why? It's ogfn which gets passed to the function. And it indeed is the >>>> prior GFN's mapping that we want to remove here. >>>> >>>>> Or assuming that ogfn is not invalid can be used to imply a removal? >>>> >>>> That implication can be (and on x86 is) used for the incoming argument, >>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. >>> >>> I guess I'm confused due to the ogfn checks done on the Arm side that >>> are not performed on x86. So on Arm you always need to explicitly >>> unhook the previous GFN before attempting to setup a new mapping, >>> while on x86 you only need to do this when it's a removal in order to >>> clear the entry? >> >> The difference isn't with guest_physmap_add_entry() (both x86 and >> Arm only insert a new mapping there), but with >> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior >> mappings. And gnttab_map_frame() gets called only from there. This >> is effectively what the first paragraph of the description is about. > > OK, sorry, it wasn't clear to me from the description. Could you > explicitly mention in the description that the removal is moved into > gnttab_set_frame_gfn on Arm in order to cope with the fact that > xenmem_add_to_physmap_one doesn't perform it. Well, it's not really "in order to cope" - that's true for the placement prior to this change as well, so not a justification for the change. Nevertheless I've tried to make this more clear by changing the 1st paragraph to: "Without holding appropriate locks, attempting to remove a prior mapping of the underlying page is pointless, as the same (or another) mapping could be re-established by a parallel request on another vCPU. Move the code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of course this new placement doesn't improve things in any way as far as the security of grant status frame mappings goes (see XSA-379). Proper locking would be needed here to allow status frames to be mapped securely." > TBH I think it would be in our best interest to try to make > xenmem_add_to_physmap_one behave as close as possible between arches. > This discrepancy between x86 and Arm regarding page removal is just > going to bring more trouble in the long term, and hiding the > differences inside gnttab_set_frame_gfn just makes it even more > obscure. Stefano, Julien? Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-22 9:42 ` Jan Beulich @ 2021-09-22 10:00 ` Roger Pau Monné 2021-09-28 13:36 ` Ping: " Jan Beulich 2021-09-22 10:28 ` Julien Grall 1 sibling, 1 reply; 26+ messages in thread From: Roger Pau Monné @ 2021-09-22 10:00 UTC (permalink / raw) To: Jan Beulich Cc: Julien Grall, Stefano Stabellini, xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu On Wed, Sep 22, 2021 at 11:42:30AM +0200, Jan Beulich wrote: > On 22.09.2021 11:26, Roger Pau Monné wrote: > > On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: > >> On 21.09.2021 10:32, Roger Pau Monné wrote: > >>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: > >>>> On 20.09.2021 12:20, Roger Pau Monné wrote: > >>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: > >>>>>> --- a/xen/include/asm-arm/grant_table.h > >>>>>> +++ b/xen/include/asm-arm/grant_table.h > >>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ > >>>>> > >>>>> I'm slightly confused by this checks, don't you need to check for > >>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call > >>>>> guest_physmap_remove_page? > >>>> > >>>> Why? It's ogfn which gets passed to the function. And it indeed is the > >>>> prior GFN's mapping that we want to remove here. > >>>> > >>>>> Or assuming that ogfn is not invalid can be used to imply a removal? > >>>> > >>>> That implication can be (and on x86 is) used for the incoming argument, > >>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. > >>> > >>> I guess I'm confused due to the ogfn checks done on the Arm side that > >>> are not performed on x86. So on Arm you always need to explicitly > >>> unhook the previous GFN before attempting to setup a new mapping, > >>> while on x86 you only need to do this when it's a removal in order to > >>> clear the entry? > >> > >> The difference isn't with guest_physmap_add_entry() (both x86 and > >> Arm only insert a new mapping there), but with > >> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior > >> mappings. And gnttab_map_frame() gets called only from there. This > >> is effectively what the first paragraph of the description is about. > > > > OK, sorry, it wasn't clear to me from the description. Could you > > explicitly mention in the description that the removal is moved into > > gnttab_set_frame_gfn on Arm in order to cope with the fact that > > xenmem_add_to_physmap_one doesn't perform it. > > Well, it's not really "in order to cope" - that's true for the placement > prior to this change as well, so not a justification for the change. > Nevertheless I've tried to make this more clear by changing the 1st > paragraph to: > > "Without holding appropriate locks, attempting to remove a prior mapping > of the underlying page is pointless, as the same (or another) mapping > could be re-established by a parallel request on another vCPU. Move the > code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since > xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of > course this new placement doesn't improve things in any way as far as > the security of grant status frame mappings goes (see XSA-379). Proper > locking would be needed here to allow status frames to be mapped > securely." Thanks, this is indeed much clearer IMO: Acked-by: Roger Pau Monné <roger.pau@citrix.com> Albeit I still think we need to fix Arm side to do the removal in xenmem_add_to_physmap_one (or the x86 side to not do it). Thanks, Roger. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Ping: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-22 10:00 ` Roger Pau Monné @ 2021-09-28 13:36 ` Jan Beulich 0 siblings, 0 replies; 26+ messages in thread From: Jan Beulich @ 2021-09-28 13:36 UTC (permalink / raw) To: Julien Grall, Stefano Stabellini Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné On 22.09.2021 12:00, Roger Pau Monné wrote: > On Wed, Sep 22, 2021 at 11:42:30AM +0200, Jan Beulich wrote: >> On 22.09.2021 11:26, Roger Pau Monné wrote: >>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: >>>> On 21.09.2021 10:32, Roger Pau Monné wrote: >>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: >>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote: >>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >>>>>>>> --- a/xen/include/asm-arm/grant_table.h >>>>>>>> +++ b/xen/include/asm-arm/grant_table.h >>>>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>>>>>> >>>>>>> I'm slightly confused by this checks, don't you need to check for >>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call >>>>>>> guest_physmap_remove_page? >>>>>> >>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the >>>>>> prior GFN's mapping that we want to remove here. >>>>>> >>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal? >>>>>> >>>>>> That implication can be (and on x86 is) used for the incoming argument, >>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. >>>>> >>>>> I guess I'm confused due to the ogfn checks done on the Arm side that >>>>> are not performed on x86. So on Arm you always need to explicitly >>>>> unhook the previous GFN before attempting to setup a new mapping, >>>>> while on x86 you only need to do this when it's a removal in order to >>>>> clear the entry? >>>> >>>> The difference isn't with guest_physmap_add_entry() (both x86 and >>>> Arm only insert a new mapping there), but with >>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior >>>> mappings. And gnttab_map_frame() gets called only from there. This >>>> is effectively what the first paragraph of the description is about. >>> >>> OK, sorry, it wasn't clear to me from the description. Could you >>> explicitly mention in the description that the removal is moved into >>> gnttab_set_frame_gfn on Arm in order to cope with the fact that >>> xenmem_add_to_physmap_one doesn't perform it. >> >> Well, it's not really "in order to cope" - that's true for the placement >> prior to this change as well, so not a justification for the change. >> Nevertheless I've tried to make this more clear by changing the 1st >> paragraph to: >> >> "Without holding appropriate locks, attempting to remove a prior mapping >> of the underlying page is pointless, as the same (or another) mapping >> could be re-established by a parallel request on another vCPU. Move the >> code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since >> xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of >> course this new placement doesn't improve things in any way as far as >> the security of grant status frame mappings goes (see XSA-379). Proper >> locking would be needed here to allow status frames to be mapped >> securely." > > Thanks, this is indeed much clearer IMO: > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Any chance of an Arm ack (or otherwise) here? Thanks, Jan > Albeit I still think we need to fix Arm side to do the removal in > xenmem_add_to_physmap_one (or the x86 side to not do it). > > Thanks, Roger. > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-22 9:42 ` Jan Beulich 2021-09-22 10:00 ` Roger Pau Monné @ 2021-09-22 10:28 ` Julien Grall 2021-09-22 10:47 ` Jan Beulich 1 sibling, 1 reply; 26+ messages in thread From: Julien Grall @ 2021-09-22 10:28 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monné, Stefano Stabellini Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu Hi, On 22/09/2021 14:42, Jan Beulich wrote: > On 22.09.2021 11:26, Roger Pau Monné wrote: >> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: >>> On 21.09.2021 10:32, Roger Pau Monné wrote: >>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: >>>>> On 20.09.2021 12:20, Roger Pau Monné wrote: >>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >>>>>>> --- a/xen/include/asm-arm/grant_table.h >>>>>>> +++ b/xen/include/asm-arm/grant_table.h >>>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>>>>> >>>>>> I'm slightly confused by this checks, don't you need to check for >>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call >>>>>> guest_physmap_remove_page? >>>>> >>>>> Why? It's ogfn which gets passed to the function. And it indeed is the >>>>> prior GFN's mapping that we want to remove here. >>>>> >>>>>> Or assuming that ogfn is not invalid can be used to imply a removal? >>>>> >>>>> That implication can be (and on x86 is) used for the incoming argument, >>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. >>>> >>>> I guess I'm confused due to the ogfn checks done on the Arm side that >>>> are not performed on x86. So on Arm you always need to explicitly >>>> unhook the previous GFN before attempting to setup a new mapping, >>>> while on x86 you only need to do this when it's a removal in order to >>>> clear the entry? >>> >>> The difference isn't with guest_physmap_add_entry() (both x86 and >>> Arm only insert a new mapping there), but with >>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior >>> mappings. And gnttab_map_frame() gets called only from there. This >>> is effectively what the first paragraph of the description is about. >> >> OK, sorry, it wasn't clear to me from the description. Could you >> explicitly mention in the description that the removal is moved into >> gnttab_set_frame_gfn on Arm in order to cope with the fact that >> xenmem_add_to_physmap_one doesn't perform it. > > Well, it's not really "in order to cope" - that's true for the placement > prior to this change as well, so not a justification for the change. > Nevertheless I've tried to make this more clear by changing the 1st > paragraph to: > > "Without holding appropriate locks, attempting to remove a prior mapping > of the underlying page is pointless, as the same (or another) mapping > could be re-established by a parallel request on another vCPU. Move the > code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since > xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of > course this new placement doesn't improve things in any way as far as > the security of grant status frame mappings goes (see XSA-379). Proper > locking would be needed here to allow status frames to be mapped > securely." > >> TBH I think it would be in our best interest to try to make >> xenmem_add_to_physmap_one behave as close as possible between arches. >> This discrepancy between x86 and Arm regarding page removal is just >> going to bring more trouble in the long term, and hiding the >> differences inside gnttab_set_frame_gfn just makes it even more >> obscure. > > Stefano, Julien? This would be ideal as I don't particular like the approach taken in this patch. But AFAICT, this would require us to implement an M2P. Or is there another way to do it? In another context, I saw the suggestion to bring an M2P on Arm. But I am still somewhat split whether this is really worth it for the current use. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-22 10:28 ` Julien Grall @ 2021-09-22 10:47 ` Jan Beulich 2021-09-23 8:40 ` Julien Grall 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-09-22 10:47 UTC (permalink / raw) To: Julien Grall Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné, Stefano Stabellini On 22.09.2021 12:28, Julien Grall wrote: > Hi, > > On 22/09/2021 14:42, Jan Beulich wrote: >> On 22.09.2021 11:26, Roger Pau Monné wrote: >>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: >>>> On 21.09.2021 10:32, Roger Pau Monné wrote: >>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: >>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote: >>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >>>>>>>> --- a/xen/include/asm-arm/grant_table.h >>>>>>>> +++ b/xen/include/asm-arm/grant_table.h >>>>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>>>>>> >>>>>>> I'm slightly confused by this checks, don't you need to check for >>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call >>>>>>> guest_physmap_remove_page? >>>>>> >>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the >>>>>> prior GFN's mapping that we want to remove here. >>>>>> >>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal? >>>>>> >>>>>> That implication can be (and on x86 is) used for the incoming argument, >>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. >>>>> >>>>> I guess I'm confused due to the ogfn checks done on the Arm side that >>>>> are not performed on x86. So on Arm you always need to explicitly >>>>> unhook the previous GFN before attempting to setup a new mapping, >>>>> while on x86 you only need to do this when it's a removal in order to >>>>> clear the entry? >>>> >>>> The difference isn't with guest_physmap_add_entry() (both x86 and >>>> Arm only insert a new mapping there), but with >>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior >>>> mappings. And gnttab_map_frame() gets called only from there. This >>>> is effectively what the first paragraph of the description is about. >>> >>> OK, sorry, it wasn't clear to me from the description. Could you >>> explicitly mention in the description that the removal is moved into >>> gnttab_set_frame_gfn on Arm in order to cope with the fact that >>> xenmem_add_to_physmap_one doesn't perform it. >> >> Well, it's not really "in order to cope" - that's true for the placement >> prior to this change as well, so not a justification for the change. >> Nevertheless I've tried to make this more clear by changing the 1st >> paragraph to: >> >> "Without holding appropriate locks, attempting to remove a prior mapping >> of the underlying page is pointless, as the same (or another) mapping >> could be re-established by a parallel request on another vCPU. Move the >> code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since >> xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of >> course this new placement doesn't improve things in any way as far as >> the security of grant status frame mappings goes (see XSA-379). Proper >> locking would be needed here to allow status frames to be mapped >> securely." >> >>> TBH I think it would be in our best interest to try to make >>> xenmem_add_to_physmap_one behave as close as possible between arches. >>> This discrepancy between x86 and Arm regarding page removal is just >>> going to bring more trouble in the long term, and hiding the >>> differences inside gnttab_set_frame_gfn just makes it even more >>> obscure. >> >> Stefano, Julien? > > This would be ideal as I don't particular like the approach taken in > this patch. But AFAICT, this would require us to implement an M2P. Or is > there another way to do it? For the purpose of just XENMAPSPACE_grant_table the "restricted" M2P (to just grant table pages), as introduced by an on-list patch, would do afaict. That wouldn't remove all the differences between the two implementations, but at least the one affecting the difference in how gnttab_map_frame() needs to behave. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-22 10:47 ` Jan Beulich @ 2021-09-23 8:40 ` Julien Grall 0 siblings, 0 replies; 26+ messages in thread From: Julien Grall @ 2021-09-23 8:40 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, Roger Pau Monné, Stefano Stabellini Hi Jan, On 22/09/2021 15:47, Jan Beulich wrote: > On 22.09.2021 12:28, Julien Grall wrote: >> Hi, >> >> On 22/09/2021 14:42, Jan Beulich wrote: >>> On 22.09.2021 11:26, Roger Pau Monné wrote: >>>> On Tue, Sep 21, 2021 at 12:12:05PM +0200, Jan Beulich wrote: >>>>> On 21.09.2021 10:32, Roger Pau Monné wrote: >>>>>> On Mon, Sep 20, 2021 at 05:27:17PM +0200, Jan Beulich wrote: >>>>>>> On 20.09.2021 12:20, Roger Pau Monné wrote: >>>>>>>> On Mon, Sep 13, 2021 at 08:41:47AM +0200, Jan Beulich wrote: >>>>>>>>> --- a/xen/include/asm-arm/grant_table.h >>>>>>>>> +++ b/xen/include/asm-arm/grant_table.h >>>>>>>>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>>>>>>> >>>>>>>> I'm slightly confused by this checks, don't you need to check for >>>>>>>> gfn_eq(gfn, INVALID_GFN) (not ogfn) in order to call >>>>>>>> guest_physmap_remove_page? >>>>>>> >>>>>>> Why? It's ogfn which gets passed to the function. And it indeed is the >>>>>>> prior GFN's mapping that we want to remove here. >>>>>>> >>>>>>>> Or assuming that ogfn is not invalid can be used to imply a removal? >>>>>>> >>>>>>> That implication can be (and on x86 is) used for the incoming argument, >>>>>>> i.e. "gfn". I don't think "ogfn" can serve this purpose. >>>>>> >>>>>> I guess I'm confused due to the ogfn checks done on the Arm side that >>>>>> are not performed on x86. So on Arm you always need to explicitly >>>>>> unhook the previous GFN before attempting to setup a new mapping, >>>>>> while on x86 you only need to do this when it's a removal in order to >>>>>> clear the entry? >>>>> >>>>> The difference isn't with guest_physmap_add_entry() (both x86 and >>>>> Arm only insert a new mapping there), but with >>>>> xenmem_add_to_physmap_one(): Arm's variant doesn't care about prior >>>>> mappings. And gnttab_map_frame() gets called only from there. This >>>>> is effectively what the first paragraph of the description is about. >>>> >>>> OK, sorry, it wasn't clear to me from the description. Could you >>>> explicitly mention in the description that the removal is moved into >>>> gnttab_set_frame_gfn on Arm in order to cope with the fact that >>>> xenmem_add_to_physmap_one doesn't perform it. >>> >>> Well, it's not really "in order to cope" - that's true for the placement >>> prior to this change as well, so not a justification for the change. >>> Nevertheless I've tried to make this more clear by changing the 1st >>> paragraph to: >>> >>> "Without holding appropriate locks, attempting to remove a prior mapping >>> of the underlying page is pointless, as the same (or another) mapping >>> could be re-established by a parallel request on another vCPU. Move the >>> code to Arm's gnttab_set_frame_gfn(); it cannot be dropped there since >>> xenmem_add_to_physmap_one() doesn't call it either (unlike on x86). Of >>> course this new placement doesn't improve things in any way as far as >>> the security of grant status frame mappings goes (see XSA-379). Proper >>> locking would be needed here to allow status frames to be mapped >>> securely." >>> >>>> TBH I think it would be in our best interest to try to make >>>> xenmem_add_to_physmap_one behave as close as possible between arches. >>>> This discrepancy between x86 and Arm regarding page removal is just >>>> going to bring more trouble in the long term, and hiding the >>>> differences inside gnttab_set_frame_gfn just makes it even more >>>> obscure. >>> >>> Stefano, Julien? >> >> This would be ideal as I don't particular like the approach taken in >> this patch. But AFAICT, this would require us to implement an M2P. Or is >> there another way to do it? > > For the purpose of just XENMAPSPACE_grant_table the "restricted" M2P > (to just grant table pages), as introduced by an on-list patch, would > do afaict. That wouldn't remove all the differences between the two > implementations, but at least the one affecting the difference in how > gnttab_map_frame() needs to behave. This would work. I will add it in my todo list but I am not such when I will have time to look at it. I am happy if someone else wants to look at it. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich 2021-09-20 10:20 ` Roger Pau Monné @ 2021-11-25 16:37 ` Julien Grall 2021-11-26 8:07 ` Jan Beulich 2021-12-02 9:09 ` Julien Grall 2 siblings, 1 reply; 26+ messages in thread From: Julien Grall @ 2021-11-25 16:37 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné Hi Jan, Sorry for the delay. I was waiting for XSA-387 to go out before answering. On 13/09/2021 07:41, Jan Beulich wrote: > Without holding appropriate locks, attempting to remove a prior mapping > of the underlying page is pointless, as the same (or another) mapping > could be re-established by a parallel request on another vCPU. Move the > code to Arm's gnttab_set_frame_gfn(). Of course this new placement > doesn't improve things in any way as far as the security of grant status > frame mappings goes (see XSA-379). Proper locking would be needed here > to allow status frames to be mapped securely. > > In turn this then requires replacing the other use in > gnttab_unpopulate_status_frames(), which yet in turn requires adjusting > x86's gnttab_set_frame_gfn(). Note that with proper locking inside > guest_physmap_remove_page() combined with checking the GFN's mapping > there against the passed in MFN, there then is no issue with the > involved multiple gnttab_set_frame_gfn()-s potentially returning varying Do you mean gnttab_get_frame_gfn()? > values (due to a racing XENMAPSPACE_grant_table request). > > This, as a side effect, does away with gnttab_map_frame() having a local > variable "gfn" which shadows a function parameter of the same name. > > Together with XSA-379 this points out that XSA-255's addition to > gnttab_map_frame() was really useless. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -92,7 +92,7 @@ struct grant_table { > struct radix_tree_root maptrack_tree; > > /* Domain to which this struct grant_table belongs. */ > - const struct domain *domain; > + struct domain *domain; > > struct grant_table_arch arch; > }; > @@ -1795,8 +1795,8 @@ gnttab_unpopulate_status_frames(struct d > { > int rc = gfn_eq(gfn, INVALID_GFN) > ? 0 > - : guest_physmap_remove_page(d, gfn, > - page_to_mfn(pg), 0); > + : gnttab_set_frame_gfn(gt, true, i, INVALID_GFN, > + page_to_mfn(pg)); > > if ( rc ) > { > @@ -1806,7 +1806,6 @@ gnttab_unpopulate_status_frames(struct d > domain_crash(d); > return rc; > } > - gnttab_set_frame_gfn(gt, true, i, INVALID_GFN); > } > > BUG_ON(page_get_owner(pg) != d); > @@ -4132,6 +4131,9 @@ int gnttab_map_frame(struct domain *d, u > struct grant_table *gt = d->grant_table; > bool status = false; > > + if ( gfn_eq(gfn, INVALID_GFN) ) > + return -EINVAL; > + > grant_write_lock(gt); > > if ( evaluate_nospec(gt->gt_version == 2) && (idx & XENMAPIDX_grant_table_status) ) > @@ -4144,24 +4146,18 @@ int gnttab_map_frame(struct domain *d, u > else > rc = gnttab_get_shared_frame_mfn(d, idx, mfn); > > - if ( !rc && paging_mode_translate(d) ) > - { > - gfn_t gfn = gnttab_get_frame_gfn(gt, status, idx); > - > - if ( !gfn_eq(gfn, INVALID_GFN) ) > - rc = guest_physmap_remove_page(d, gfn, *mfn, 0); > - } > - > if ( !rc ) > { > + struct page_info *pg = mfn_to_page(*mfn); > + > /* > * Make sure gnttab_unpopulate_status_frames() won't (successfully) > * free the page until our caller has completed its operation. > */ > - if ( get_page(mfn_to_page(*mfn), d) ) > - gnttab_set_frame_gfn(gt, status, idx, gfn); > - else > + if ( !get_page(pg, d) ) > rc = -EBUSY; > + else if ( (rc = gnttab_set_frame_gfn(gt, status, idx, gfn, *mfn)) ) > + put_page(pg); > } > > grant_write_unlock(gt); > --- a/xen/include/asm-arm/grant_table.h > +++ b/xen/include/asm-arm/grant_table.h > @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned > XFREE((gt)->arch.status_gfn); \ > } while ( 0 ) > > -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ > - do { \ > - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ > - (gfn); \ > - } while ( 0 ) > +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ > + ({ \ > + int rc_ = 0; \ > + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ > + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ > + (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \ > + 0)) == 0 ) \ > + ((st) ? (gt)->arch.status_gfn \ > + : (gt)->arch.shared_gfn)[idx] = (gfn); \ > + rc_; \ > + }) I think using a function would make it a bit easier to understand what it does. However... The naming of the function is now quite confusing. The more on x86... > > #define gnttab_get_frame_gfn(gt, st, idx) ({ \ > (st) ? gnttab_status_gfn(NULL, gt, idx) \ > --- a/xen/include/asm-x86/grant_table.h > +++ b/xen/include/asm-x86/grant_table.h > @@ -37,7 +37,12 @@ static inline int replace_grant_host_map > > #define gnttab_init_arch(gt) 0 > #define gnttab_destroy_arch(gt) do {} while ( 0 ) > -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 ) > +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ > + (gfn_eq(gfn, INVALID_GFN) \ > + ? guest_physmap_remove_page((gt)->domain, \ > + gnttab_get_frame_gfn(gt, st, idx), \ > + mfn, 0) \ > + : 0 /* Handled in add_to_physmap_one(). */) ... it will end up to remove the mapping. I don't have a better name at the moment. However I think this would deserve some documentation in the code so one can understand how to implement it for another arch. > #define gnttab_get_frame_gfn(gt, st, idx) ({ \ > mfn_t mfn_ = (st) ? gnttab_status_mfn(gt, idx) \ > : gnttab_shared_mfn(gt, idx); \ > Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-11-25 16:37 ` Julien Grall @ 2021-11-26 8:07 ` Jan Beulich 2021-12-01 18:19 ` Julien Grall 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-11-26 8:07 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné, xen-devel On 25.11.2021 17:37, Julien Grall wrote: > On 13/09/2021 07:41, Jan Beulich wrote: >> Without holding appropriate locks, attempting to remove a prior mapping >> of the underlying page is pointless, as the same (or another) mapping >> could be re-established by a parallel request on another vCPU. Move the >> code to Arm's gnttab_set_frame_gfn(). Of course this new placement >> doesn't improve things in any way as far as the security of grant status >> frame mappings goes (see XSA-379). Proper locking would be needed here >> to allow status frames to be mapped securely. >> >> In turn this then requires replacing the other use in >> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting >> x86's gnttab_set_frame_gfn(). Note that with proper locking inside >> guest_physmap_remove_page() combined with checking the GFN's mapping >> there against the passed in MFN, there then is no issue with the >> involved multiple gnttab_set_frame_gfn()-s potentially returning varying > > Do you mean gnttab_get_frame_gfn()? No, I don't think so; I do mean gnttab_set_frame_gfn(). Its return value directs the caller to do (or not) certain things. >> --- a/xen/include/asm-arm/grant_table.h >> +++ b/xen/include/asm-arm/grant_table.h >> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned >> XFREE((gt)->arch.status_gfn); \ >> } while ( 0 ) >> >> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ >> - do { \ >> - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ >> - (gfn); \ >> - } while ( 0 ) >> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ >> + ({ \ >> + int rc_ = 0; \ >> + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ >> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >> + (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \ >> + 0)) == 0 ) \ >> + ((st) ? (gt)->arch.status_gfn \ >> + : (gt)->arch.shared_gfn)[idx] = (gfn); \ >> + rc_; \ >> + }) > > I think using a function would make it a bit easier to understand what > it does. I can convert it, but it's not very likely that it would be possible to make it an inline one. Not sure whether that's then still desirable. > However... The naming of the function is now quite confusing. The more > on x86... > >> >> #define gnttab_get_frame_gfn(gt, st, idx) ({ \ >> (st) ? gnttab_status_gfn(NULL, gt, idx) \ >> --- a/xen/include/asm-x86/grant_table.h >> +++ b/xen/include/asm-x86/grant_table.h >> @@ -37,7 +37,12 @@ static inline int replace_grant_host_map >> >> #define gnttab_init_arch(gt) 0 >> #define gnttab_destroy_arch(gt) do {} while ( 0 ) >> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 ) >> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ >> + (gfn_eq(gfn, INVALID_GFN) \ >> + ? guest_physmap_remove_page((gt)->domain, \ >> + gnttab_get_frame_gfn(gt, st, idx), \ >> + mfn, 0) \ >> + : 0 /* Handled in add_to_physmap_one(). */) > > ... it will end up to remove the mapping. I don't have a better name at > the moment. However I think this would deserve some documentation in the > code so one can understand how to implement it for another arch. Hmm, perhaps. But wouldn't we better document the final behavior (i.e. once the remaining Arm issue got addressed)? That may then also lead to overall simpler code, ideally with more suitable names (if the present ones are deemed unsuitable, which I'm not convinced of). Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-11-26 8:07 ` Jan Beulich @ 2021-12-01 18:19 ` Julien Grall 0 siblings, 0 replies; 26+ messages in thread From: Julien Grall @ 2021-12-01 18:19 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné, xen-devel Hi Jan, On 26/11/2021 08:07, Jan Beulich wrote: > On 25.11.2021 17:37, Julien Grall wrote: >> On 13/09/2021 07:41, Jan Beulich wrote: >>> Without holding appropriate locks, attempting to remove a prior mapping >>> of the underlying page is pointless, as the same (or another) mapping >>> could be re-established by a parallel request on another vCPU. Move the >>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement >>> doesn't improve things in any way as far as the security of grant status >>> frame mappings goes (see XSA-379). Proper locking would be needed here >>> to allow status frames to be mapped securely. >>> >>> In turn this then requires replacing the other use in >>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting >>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside >>> guest_physmap_remove_page() combined with checking the GFN's mapping >>> there against the passed in MFN, there then is no issue with the >>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying >> >> Do you mean gnttab_get_frame_gfn()? > > No, I don't think so; I do mean gnttab_set_frame_gfn(). Its return value > directs the caller to do (or not) certain things. Hmmm ok. I misundertood that comment then. Thanks for the clarification! > >>> --- a/xen/include/asm-arm/grant_table.h >>> +++ b/xen/include/asm-arm/grant_table.h >>> @@ -71,11 +71,17 @@ int replace_grant_host_mapping(unsigned >>> XFREE((gt)->arch.status_gfn); \ >>> } while ( 0 ) >>> >>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) \ >>> - do { \ >>> - ((st) ? (gt)->arch.status_gfn : (gt)->arch.shared_gfn)[idx] = \ >>> - (gfn); \ >>> - } while ( 0 ) >>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ >>> + ({ \ >>> + int rc_ = 0; \ >>> + gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ >>> + if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) || \ >>> + (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn, \ >>> + 0)) == 0 ) \ >>> + ((st) ? (gt)->arch.status_gfn \ >>> + : (gt)->arch.shared_gfn)[idx] = (gfn); \ >>> + rc_; \ >>> + }) >> >> I think using a function would make it a bit easier to understand what >> it does. > > I can convert it, but it's not very likely that it would be possible > to make it an inline one. Not sure whether that's then still desirable. So looking at Oleksandr series, I think we should be able to have the Arm helper matching the x86 one. At which point, I could deal with this version for now. > >> However... The naming of the function is now quite confusing. The more >> on x86... >> >>> >>> #define gnttab_get_frame_gfn(gt, st, idx) ({ \ >>> (st) ? gnttab_status_gfn(NULL, gt, idx) \ >>> --- a/xen/include/asm-x86/grant_table.h >>> +++ b/xen/include/asm-x86/grant_table.h >>> @@ -37,7 +37,12 @@ static inline int replace_grant_host_map >>> >>> #define gnttab_init_arch(gt) 0 >>> #define gnttab_destroy_arch(gt) do {} while ( 0 ) >>> -#define gnttab_set_frame_gfn(gt, st, idx, gfn) do {} while ( 0 ) >>> +#define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ >>> + (gfn_eq(gfn, INVALID_GFN) \ >>> + ? guest_physmap_remove_page((gt)->domain, \ >>> + gnttab_get_frame_gfn(gt, st, idx), \ >>> + mfn, 0) \ >>> + : 0 /* Handled in add_to_physmap_one(). */) >> >> ... it will end up to remove the mapping. I don't have a better name at >> the moment. However I think this would deserve some documentation in the >> code so one can understand how to implement it for another arch. > > Hmm, perhaps. But wouldn't we better document the final behavior (i.e. > once the remaining Arm issue got addressed)? Fair point. I don't expect Oleksandr's series to be committed long after yours. > That may then also lead > to overall simpler code, ideally with more suitable names (if the > present ones are deemed unsuitable, which I'm not convinced of). I don't have a better name so far. However, without any documentation this is very difficult to figure out what it is meant to do (I am not only referring to someone implementing it for another arch here). Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich 2021-09-20 10:20 ` Roger Pau Monné 2021-11-25 16:37 ` Julien Grall @ 2021-12-02 9:09 ` Julien Grall 2021-12-02 10:12 ` Jan Beulich 2021-12-02 16:12 ` Oleksandr 2 siblings, 2 replies; 26+ messages in thread From: Julien Grall @ 2021-12-02 9:09 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné Hi Jan, On 13/09/2021 07:41, Jan Beulich wrote: > Without holding appropriate locks, attempting to remove a prior mapping > of the underlying page is pointless, as the same (or another) mapping > could be re-established by a parallel request on another vCPU. Move the > code to Arm's gnttab_set_frame_gfn(). Of course this new placement > doesn't improve things in any way as far as the security of grant status > frame mappings goes (see XSA-379). Proper locking would be needed here > to allow status frames to be mapped securely. > > In turn this then requires replacing the other use in > gnttab_unpopulate_status_frames(), which yet in turn requires adjusting > x86's gnttab_set_frame_gfn(). Note that with proper locking inside > guest_physmap_remove_page() combined with checking the GFN's mapping > there against the passed in MFN, there then is no issue with the > involved multiple gnttab_set_frame_gfn()-s potentially returning varying > values (due to a racing XENMAPSPACE_grant_table request). > > This, as a side effect, does away with gnttab_map_frame() having a local > variable "gfn" which shadows a function parameter of the same name. > > Together with XSA-379 this points out that XSA-255's addition to > gnttab_map_frame() was really useless. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> As discussed in the thread, I expect the Arm part to be simplified after Oleksandr's series. So for the Arm part: Acked-by: Julien Grall <jgrall@amazon.com> Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-12-02 9:09 ` Julien Grall @ 2021-12-02 10:12 ` Jan Beulich 2021-12-02 11:05 ` Julien Grall 2021-12-02 16:12 ` Oleksandr 1 sibling, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-12-02 10:12 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné, xen-devel On 02.12.2021 10:09, Julien Grall wrote: > Hi Jan, > > On 13/09/2021 07:41, Jan Beulich wrote: >> Without holding appropriate locks, attempting to remove a prior mapping >> of the underlying page is pointless, as the same (or another) mapping >> could be re-established by a parallel request on another vCPU. Move the >> code to Arm's gnttab_set_frame_gfn(). Of course this new placement >> doesn't improve things in any way as far as the security of grant status >> frame mappings goes (see XSA-379). Proper locking would be needed here >> to allow status frames to be mapped securely. >> >> In turn this then requires replacing the other use in >> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting >> x86's gnttab_set_frame_gfn(). Note that with proper locking inside >> guest_physmap_remove_page() combined with checking the GFN's mapping >> there against the passed in MFN, there then is no issue with the >> involved multiple gnttab_set_frame_gfn()-s potentially returning varying >> values (due to a racing XENMAPSPACE_grant_table request). >> >> This, as a side effect, does away with gnttab_map_frame() having a local >> variable "gfn" which shadows a function parameter of the same name. >> >> Together with XSA-379 this points out that XSA-255's addition to >> gnttab_map_frame() was really useless. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > As discussed in the thread, I expect the Arm part to be simplified after > Oleksandr's series. So for the Arm part: > > Acked-by: Julien Grall <jgrall@amazon.com> Thanks, but let me ask for a clarification: Explicitly just the Arm part, or also the common code change? I ask because from the x86 side I already have an ack by Roger, but strictly speaking that doesn't cover common code. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-12-02 10:12 ` Jan Beulich @ 2021-12-02 11:05 ` Julien Grall 0 siblings, 0 replies; 26+ messages in thread From: Julien Grall @ 2021-12-02 11:05 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné, xen-devel Hi Jan, On 02/12/2021 10:12, Jan Beulich wrote: > On 02.12.2021 10:09, Julien Grall wrote: >> Hi Jan, >> >> On 13/09/2021 07:41, Jan Beulich wrote: >>> Without holding appropriate locks, attempting to remove a prior mapping >>> of the underlying page is pointless, as the same (or another) mapping >>> could be re-established by a parallel request on another vCPU. Move the >>> code to Arm's gnttab_set_frame_gfn(). Of course this new placement >>> doesn't improve things in any way as far as the security of grant status >>> frame mappings goes (see XSA-379). Proper locking would be needed here >>> to allow status frames to be mapped securely. >>> >>> In turn this then requires replacing the other use in >>> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting >>> x86's gnttab_set_frame_gfn(). Note that with proper locking inside >>> guest_physmap_remove_page() combined with checking the GFN's mapping >>> there against the passed in MFN, there then is no issue with the >>> involved multiple gnttab_set_frame_gfn()-s potentially returning varying >>> values (due to a racing XENMAPSPACE_grant_table request). >>> >>> This, as a side effect, does away with gnttab_map_frame() having a local >>> variable "gfn" which shadows a function parameter of the same name. >>> >>> Together with XSA-379 this points out that XSA-255's addition to >>> gnttab_map_frame() was really useless. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> As discussed in the thread, I expect the Arm part to be simplified after >> Oleksandr's series. So for the Arm part: >> >> Acked-by: Julien Grall <jgrall@amazon.com> > > Thanks, but let me ask for a clarification: Explicitly just the Arm part, > or also the common code change? I ask because from the x86 side I already > have an ack by Roger, but strictly speaking that doesn't cover common > code. Ah, I thought Roger gave a reviewed-by and therefore from the check-in policy be sufficient. The common parts look also fine to me. So feel free to extend my acked-by to include the common code. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() 2021-12-02 9:09 ` Julien Grall 2021-12-02 10:12 ` Jan Beulich @ 2021-12-02 16:12 ` Oleksandr 1 sibling, 0 replies; 26+ messages in thread From: Oleksandr @ 2021-12-02 16:12 UTC (permalink / raw) To: Julien Grall, Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, Roger Pau Monné On 02.12.21 11:09, Julien Grall wrote: Hi Julien, Jan > Hi Jan, > > On 13/09/2021 07:41, Jan Beulich wrote: >> Without holding appropriate locks, attempting to remove a prior mapping >> of the underlying page is pointless, as the same (or another) mapping >> could be re-established by a parallel request on another vCPU. Move the >> code to Arm's gnttab_set_frame_gfn(). Of course this new placement >> doesn't improve things in any way as far as the security of grant status >> frame mappings goes (see XSA-379). Proper locking would be needed here >> to allow status frames to be mapped securely. >> >> In turn this then requires replacing the other use in >> gnttab_unpopulate_status_frames(), which yet in turn requires adjusting >> x86's gnttab_set_frame_gfn(). Note that with proper locking inside >> guest_physmap_remove_page() combined with checking the GFN's mapping >> there against the passed in MFN, there then is no issue with the >> involved multiple gnttab_set_frame_gfn()-s potentially returning varying >> values (due to a racing XENMAPSPACE_grant_table request). >> >> This, as a side effect, does away with gnttab_map_frame() having a local >> variable "gfn" which shadows a function parameter of the same name. >> >> Together with XSA-379 this points out that XSA-255's addition to >> gnttab_map_frame() was really useless. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > As discussed in the thread, I expect the Arm part to be simplified > after Oleksandr's series. I assume, current patch is going to be committed soon(?), so I am in the process of preparing a rebased version of my patch as both touch Arm's gnttab_set_frame_gfn at least (I did a rebase sometime, but I lost these changes somehow). Anyway, according to the recent suggestion how to eliminate the possible lock inversion introduced by my patch and taking into the account changes in current patch, the Arm's gnttab_set_frame_gfn, I think, needs to be updated to the following form: #define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn) \ ({ \ gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx); \ (!gfn_eq(ogfn, INVALID_GFN) && !gfn_eq(ogfn, gfn)) \ ? guest_physmap_remove_page((gt)->domain, ogfn, mfn, 0) \ : 0; \ }) > So for the Arm part: > > Acked-by: Julien Grall <jgrall@amazon.com> > > Cheers, > -- Regards, Oleksandr Tyshchenko ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks 2021-09-13 6:39 [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich @ 2021-09-13 6:42 ` Jan Beulich 2021-10-14 11:29 ` Julien Grall 2021-09-27 20:41 ` [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Oleksandr 2 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-09-13 6:42 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu Determining that behavior is correct (i.e. results in failure) for a passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a bit more obvious by checking input in generic code - both for singular requests to not match the value and for range ones to not pass / wrap through it. For Arm similarly make more obvious that no wrapping of MFNs passed for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur: Drop the "nr" parameter of the function to avoid future callers appearing which might not themselves check for wrapping. Otherwise the respective ASSERT() in rangeset_contains_range() could trigger. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I find it odd that map_dev_mmio_region() returns success upon iomem_access_permitted() indicating failure - is this really intended? As per commit 102984bb1987 introducing it this also was added for ACPI only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI builds? I'd be happy to take suggestions towards avoiding the need to #define _gfn() around the BUILD_BUG_ON() being added. I couldn't think of anything prettier. --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one( break; } case XENMAPSPACE_dev_mmio: - rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx)); + rc = map_dev_mmio_region(d, gfn, _mfn(idx)); return rc; default: --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1360,19 +1360,18 @@ int unmap_mmio_regions(struct domain *d, int map_dev_mmio_region(struct domain *d, gfn_t gfn, - unsigned long nr, mfn_t mfn) { int res; - if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) ) + if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) ) return 0; - res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c); + res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c); if ( res < 0 ) { - printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n", - mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id); + printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n", + mfn_x(mfn), d); return res; } --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -4132,7 +4132,10 @@ int gnttab_map_frame(struct domain *d, u bool status = false; if ( gfn_eq(gfn, INVALID_GFN) ) + { + ASSERT_UNREACHABLE(); return -EINVAL; + } grant_write_lock(gt); --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -831,6 +831,9 @@ int xenmem_add_to_physmap(struct domain return -EACCES; } + if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) ) + return -EINVAL; + if ( xatp->space == XENMAPSPACE_gmfn_foreign ) extra.foreign_domid = DOMID_INVALID; @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain if ( xatp->size < start ) return -EILSEQ; + if ( xatp->gpfn + xatp->size < xatp->gpfn || + xatp->idx + xatp->size < xatp->idx ) + { +#define _gfn(x) (x) + BUILD_BUG_ON(INVALID_GFN + 1); +#undef _gfn + return -EOVERFLOW; + } + xatp->idx += start; xatp->gpfn += start; xatp->size -= start; @@ -961,6 +973,9 @@ static int xenmem_add_to_physmap_batch(s extent, 1)) ) return -EFAULT; + if ( gfn_eq(_gfn(gpfn), INVALID_GFN) ) + return -EINVAL; + rc = xenmem_add_to_physmap_one(d, xatpb->space, extra, idx, _gfn(gpfn)); --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d, int map_dev_mmio_region(struct domain *d, gfn_t gfn, - unsigned long nr, mfn_t mfn); int guest_physmap_add_entry(struct domain *d, ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks 2021-09-13 6:42 ` [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich @ 2021-10-14 11:29 ` Julien Grall 2021-10-14 14:10 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Julien Grall @ 2021-10-14 11:29 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu Hi Jan, On 13/09/2021 07:42, Jan Beulich wrote: > Determining that behavior is correct (i.e. results in failure) for a > passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a > bit more obvious by checking input in generic code - both for singular > requests to not match the value and for range ones to not pass / wrap > through it. > > For Arm similarly make more obvious that no wrapping of MFNs passed > for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur: > Drop the "nr" parameter of the function to avoid future callers > appearing which might not themselves check for wrapping. Otherwise > the respective ASSERT() in rangeset_contains_range() could trigger. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > I find it odd that map_dev_mmio_region() returns success upon > iomem_access_permitted() indicating failure - is this really intended? AFAIR yes. The hypercall is not used as "Map the region" but instead "Make sure the region is mapped if the IOMEM region is accessible". It is necessary to return 0 because dom0 OS cannot distinguished between emulated and non-emulated. So we may report error when there is none. > As per commit 102984bb1987 introducing it this also was added for ACPI > only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI > builds? There is nothing specific to ACPI in the implementation. So I don't really see the reason to restrict to CONFIG_ACPI. However, it is still possible to boot using DT when Xen is built with CONFIG_ACPI. So if the restriction was desirable, then I think it should be using !acpi_disabled. > > I'd be happy to take suggestions towards avoiding the need to #define > _gfn() around the BUILD_BUG_ON() being added. I couldn't think of > anything prettier. > > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one( > break; > } > case XENMAPSPACE_dev_mmio: > - rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx)); > + rc = map_dev_mmio_region(d, gfn, _mfn(idx)); > return rc; > > default: > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1360,19 +1360,18 @@ int unmap_mmio_regions(struct domain *d, > > int map_dev_mmio_region(struct domain *d, > gfn_t gfn, > - unsigned long nr, > mfn_t mfn) > { > int res; > > - if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) ) > + if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) ) > return 0; > > - res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c); > + res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c); > if ( res < 0 ) > { > - printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in Dom%d\n", > - mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id); > + printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n", > + mfn_x(mfn), d); > return res; > } > > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -4132,7 +4132,10 @@ int gnttab_map_frame(struct domain *d, u > bool status = false; > > if ( gfn_eq(gfn, INVALID_GFN) ) > + { > + ASSERT_UNREACHABLE(); > return -EINVAL; > + } > > grant_write_lock(gt); > > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -831,6 +831,9 @@ int xenmem_add_to_physmap(struct domain > return -EACCES; > } > > + if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) ) > + return -EINVAL; > + > if ( xatp->space == XENMAPSPACE_gmfn_foreign ) > extra.foreign_domid = DOMID_INVALID; > > @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain > if ( xatp->size < start ) > return -EILSEQ; > > + if ( xatp->gpfn + xatp->size < xatp->gpfn || > + xatp->idx + xatp->size < xatp->idx ) > + { > +#define _gfn(x) (x) AFAICT, _gfn() will already be defined. So some compiler may complain because will be defined differently on debug build. However... > + BUILD_BUG_ON(INVALID_GFN + 1); ... I might be missing something... but why can't use gfn_x(INVALID_GFN) + 1 here? In fact, I am not entirely sure what's the purpose of this BUILD_BUG_ON(). Could you give more details? > +#undef _gfn > + return -EOVERFLOW; > + } > + > xatp->idx += start; > xatp->gpfn += start; > xatp->size -= start; > @@ -961,6 +973,9 @@ static int xenmem_add_to_physmap_batch(s > extent, 1)) ) > return -EFAULT; > > + if ( gfn_eq(_gfn(gpfn), INVALID_GFN) ) > + return -EINVAL; > + > rc = xenmem_add_to_physmap_one(d, xatpb->space, extra, > idx, _gfn(gpfn)); > > --- a/xen/include/asm-arm/p2m.h > +++ b/xen/include/asm-arm/p2m.h > @@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d, > > int map_dev_mmio_region(struct domain *d, > gfn_t gfn, > - unsigned long nr, > mfn_t mfn); > > int guest_physmap_add_entry(struct domain *d, > Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks 2021-10-14 11:29 ` Julien Grall @ 2021-10-14 14:10 ` Jan Beulich 2021-10-15 9:26 ` Julien Grall 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2021-10-14 14:10 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel On 14.10.2021 13:29, Julien Grall wrote: > On 13/09/2021 07:42, Jan Beulich wrote: >> Determining that behavior is correct (i.e. results in failure) for a >> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a >> bit more obvious by checking input in generic code - both for singular >> requests to not match the value and for range ones to not pass / wrap >> through it. >> >> For Arm similarly make more obvious that no wrapping of MFNs passed >> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur: >> Drop the "nr" parameter of the function to avoid future callers >> appearing which might not themselves check for wrapping. Otherwise >> the respective ASSERT() in rangeset_contains_range() could trigger. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> I find it odd that map_dev_mmio_region() returns success upon >> iomem_access_permitted() indicating failure - is this really intended? > > AFAIR yes. The hypercall is not used as "Map the region" but instead > "Make sure the region is mapped if the IOMEM region is accessible". > > It is necessary to return 0 because dom0 OS cannot distinguished between > emulated and non-emulated. So we may report error when there is none. Odd, but I clearly don't understand all the aspects here. >> As per commit 102984bb1987 introducing it this also was added for ACPI >> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI >> builds? > > There is nothing specific to ACPI in the implementation. So I don't > really see the reason to restrict to CONFIG_ACPI. > > However, it is still possible to boot using DT when Xen is built with > CONFIG_ACPI. So if the restriction was desirable, then I think it should > be using !acpi_disabled. My point was rather about this potentially being dead code in non-ACPI builds (i.e. in particular uniformly on 32-bit). >> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain >> if ( xatp->size < start ) >> return -EILSEQ; >> >> + if ( xatp->gpfn + xatp->size < xatp->gpfn || >> + xatp->idx + xatp->size < xatp->idx ) >> + { >> +#define _gfn(x) (x) > > AFAICT, _gfn() will already be defined. So some compiler may complain > because will be defined differently on debug build. No - _gfn() is an inline function as per typesafe.h. (Or else it wouldn't be just "some" compiler, but gcc at least would have complained to me.) > However... > >> + BUILD_BUG_ON(INVALID_GFN + 1); > > ... I might be missing something... but why can't use gfn_x(INVALID_GFN) > + 1 here? Because gfn_x() also is an inline function, and that's not suitable for a compile-time constant expression. > In fact, I am not entirely sure what's the purpose of this > BUILD_BUG_ON(). Could you give more details? The expression in the surrounding if() relies on INVALID_GFN being the largest representable value, i.e. this ensures that INVALID_GFN doesn't sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size). Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks 2021-10-14 14:10 ` Jan Beulich @ 2021-10-15 9:26 ` Julien Grall 2021-10-18 13:25 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Julien Grall @ 2021-10-15 9:26 UTC (permalink / raw) To: Jan Beulich Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel Hi, On 14/10/2021 15:10, Jan Beulich wrote: > On 14.10.2021 13:29, Julien Grall wrote: >> On 13/09/2021 07:42, Jan Beulich wrote: >>> Determining that behavior is correct (i.e. results in failure) for a >>> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a >>> bit more obvious by checking input in generic code - both for singular >>> requests to not match the value and for range ones to not pass / wrap >>> through it. >>> >>> For Arm similarly make more obvious that no wrapping of MFNs passed >>> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur: >>> Drop the "nr" parameter of the function to avoid future callers >>> appearing which might not themselves check for wrapping. Otherwise >>> the respective ASSERT() in rangeset_contains_range() could trigger. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> I find it odd that map_dev_mmio_region() returns success upon >>> iomem_access_permitted() indicating failure - is this really intended? >> >> AFAIR yes. The hypercall is not used as "Map the region" but instead >> "Make sure the region is mapped if the IOMEM region is accessible". >> >> It is necessary to return 0 because dom0 OS cannot distinguished between >> emulated and non-emulated. So we may report error when there is none. > > Odd, but I clearly don't understand all the aspects here. > >>> As per commit 102984bb1987 introducing it this also was added for ACPI >>> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI >>> builds? >> >> There is nothing specific to ACPI in the implementation. So I don't >> really see the reason to restrict to CONFIG_ACPI. >> >> However, it is still possible to boot using DT when Xen is built with >> CONFIG_ACPI. So if the restriction was desirable, then I think it should >> be using !acpi_disabled. > > My point was rather about this potentially being dead code in non-ACPI > builds (i.e. in particular uniformly on 32-bit). The hypercall is already wired and a dom0 OS can use it today even on non-ACPI. Whether a dom0 OS will use it is a different question. I know that Linux will limit it to ACPI. It is likely not used by other OS, but I can't guarantee it. In this case, the hypercall is only a few lines and already restricted to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is not worth it. >>> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain >>> if ( xatp->size < start ) >>> return -EILSEQ; >>> >>> + if ( xatp->gpfn + xatp->size < xatp->gpfn || >>> + xatp->idx + xatp->size < xatp->idx ) >>> + { >>> +#define _gfn(x) (x) >> >> AFAICT, _gfn() will already be defined. So some compiler may complain >> because will be defined differently on debug build. > > No - _gfn() is an inline function as per typesafe.h. (Or else it > wouldn't be just "some" compiler, but gcc at least would have > complained to me.) Ah. somehow I thought it was a macro. But looking at the implementation, it makes sense to be an inline funciton. Sorry for the noise. > >> However... >> >>> + BUILD_BUG_ON(INVALID_GFN + 1); >> >> ... I might be missing something... but why can't use gfn_x(INVALID_GFN) >> + 1 here? > > Because gfn_x() also is an inline function, and that's not suitable > for a compile-time constant expression. Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could also be used to replace the open-code value in INVALID_GFN and INVALID_GFN_INITIALIZER? > >> In fact, I am not entirely sure what's the purpose of this >> BUILD_BUG_ON(). Could you give more details? > > The expression in the surrounding if() relies on INVALID_GFN being the > largest representable value, i.e. this ensures that INVALID_GFN doesn't > sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size). Thanks the explanation. Can you add the rationale in a comment on top of BUILD_BUG_ON()? Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks 2021-10-15 9:26 ` Julien Grall @ 2021-10-18 13:25 ` Jan Beulich 0 siblings, 0 replies; 26+ messages in thread From: Jan Beulich @ 2021-10-18 13:25 UTC (permalink / raw) To: Julien Grall Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel On 15.10.2021 11:26, Julien Grall wrote: > On 14/10/2021 15:10, Jan Beulich wrote: >> On 14.10.2021 13:29, Julien Grall wrote: >>> On 13/09/2021 07:42, Jan Beulich wrote: >>>> Determining that behavior is correct (i.e. results in failure) for a >>>> passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a >>>> bit more obvious by checking input in generic code - both for singular >>>> requests to not match the value and for range ones to not pass / wrap >>>> through it. >>>> >>>> For Arm similarly make more obvious that no wrapping of MFNs passed >>>> for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur: >>>> Drop the "nr" parameter of the function to avoid future callers >>>> appearing which might not themselves check for wrapping. Otherwise >>>> the respective ASSERT() in rangeset_contains_range() could trigger. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> I find it odd that map_dev_mmio_region() returns success upon >>>> iomem_access_permitted() indicating failure - is this really intended? >>> >>> AFAIR yes. The hypercall is not used as "Map the region" but instead >>> "Make sure the region is mapped if the IOMEM region is accessible". >>> >>> It is necessary to return 0 because dom0 OS cannot distinguished between >>> emulated and non-emulated. So we may report error when there is none. >> >> Odd, but I clearly don't understand all the aspects here. >> >>>> As per commit 102984bb1987 introducing it this also was added for ACPI >>>> only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI >>>> builds? >>> >>> There is nothing specific to ACPI in the implementation. So I don't >>> really see the reason to restrict to CONFIG_ACPI. >>> >>> However, it is still possible to boot using DT when Xen is built with >>> CONFIG_ACPI. So if the restriction was desirable, then I think it should >>> be using !acpi_disabled. >> >> My point was rather about this potentially being dead code in non-ACPI >> builds (i.e. in particular uniformly on 32-bit). > > The hypercall is already wired and a dom0 OS can use it today even on > non-ACPI. Whether a dom0 OS will use it is a different question. I know > that Linux will limit it to ACPI. It is likely not used by other OS, but > I can't guarantee it. > > In this case, the hypercall is only a few lines and already restricted > to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is > not worth it. Well, okay then - I've removed that remark. >>>> @@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain >>>> if ( xatp->size < start ) >>>> return -EILSEQ; >>>> >>>> + if ( xatp->gpfn + xatp->size < xatp->gpfn || >>>> + xatp->idx + xatp->size < xatp->idx ) >>>> + { >>>> +#define _gfn(x) (x) >>> >>> AFAICT, _gfn() will already be defined. So some compiler may complain >>> because will be defined differently on debug build. >> >> No - _gfn() is an inline function as per typesafe.h. (Or else it >> wouldn't be just "some" compiler, but gcc at least would have >> complained to me.) > > Ah. somehow I thought it was a macro. But looking at the implementation, > it makes sense to be an inline funciton. > > Sorry for the noise. > >> >>> However... >>> >>>> + BUILD_BUG_ON(INVALID_GFN + 1); >>> >>> ... I might be missing something... but why can't use gfn_x(INVALID_GFN) >>> + 1 here? >> >> Because gfn_x() also is an inline function, and that's not suitable >> for a compile-time constant expression. > > Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could > also be used to replace the open-code value in INVALID_GFN and > INVALID_GFN_INITIALIZER? Can do, but that'll be a prereq patch then also taking care of INVALID_MFN. >>> In fact, I am not entirely sure what's the purpose of this >>> BUILD_BUG_ON(). Could you give more details? >> >> The expression in the surrounding if() relies on INVALID_GFN being the >> largest representable value, i.e. this ensures that INVALID_GFN doesn't >> sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size). > > Thanks the explanation. Can you add the rationale in a comment on top of > BUILD_BUG_ON()? Sure, done. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 2021-09-13 6:39 [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich 2021-09-13 6:42 ` [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich @ 2021-09-27 20:41 ` Oleksandr 2 siblings, 0 replies; 26+ messages in thread From: Oleksandr @ 2021-09-27 20:41 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu On 13.09.21 09:39, Jan Beulich wrote: Hi Jan > I'm prepared for the "how" aspect of the 1st patch here to end up > controversial. Since the observed quirk will imo want dealing with, > I'd appreciate any objection to the proposed change to be accompanied > by an alternative suggestion. An intention of mine was to not further > increase the number of arch hooks needed. I further realize that this > change conflicts with Oleksandr's "xen/gnttab: Store frame GFN in > struct page_info on Arm", at the very least contextually. FYI, I have a rebased version of my patch on top of your patch #1 locally. I preliminary checked that combination on my setup (Arm64) and didn't see any issues. > > 1: gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() > 2: memory: XENMEM_add_to_physmap (almost) wrapping checks > > Jan > > -- Regards, Oleksandr Tyshchenko ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-12-02 16:12 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-13 6:39 [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Jan Beulich 2021-09-13 6:41 ` [PATCH 1/2] gnttab: remove guest_physmap_remove_page() call from gnttab_map_frame() Jan Beulich 2021-09-20 10:20 ` Roger Pau Monné 2021-09-20 15:27 ` Jan Beulich 2021-09-21 8:32 ` Roger Pau Monné 2021-09-21 10:12 ` Jan Beulich 2021-09-22 9:26 ` Roger Pau Monné 2021-09-22 9:42 ` Jan Beulich 2021-09-22 10:00 ` Roger Pau Monné 2021-09-28 13:36 ` Ping: " Jan Beulich 2021-09-22 10:28 ` Julien Grall 2021-09-22 10:47 ` Jan Beulich 2021-09-23 8:40 ` Julien Grall 2021-11-25 16:37 ` Julien Grall 2021-11-26 8:07 ` Jan Beulich 2021-12-01 18:19 ` Julien Grall 2021-12-02 9:09 ` Julien Grall 2021-12-02 10:12 ` Jan Beulich 2021-12-02 11:05 ` Julien Grall 2021-12-02 16:12 ` Oleksandr 2021-09-13 6:42 ` [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks Jan Beulich 2021-10-14 11:29 ` Julien Grall 2021-10-14 14:10 ` Jan Beulich 2021-10-15 9:26 ` Julien Grall 2021-10-18 13:25 ` Jan Beulich 2021-09-27 20:41 ` [PATCH 0/2] grant table and add-to-physmap adjustments on top of XSAs 379 and 384 Oleksandr
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.