| Message ID | 2bf46266-785d-0de3-5f61-48c3fd191a5c@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gnttab: misc fixes | expand |
On 17/02/2021 10:46, Jan Beulich wrote: > Prior to its introduction, v1 entries weren't checked for this flag, and > the flag also has been meaningless for v1 entries. Therefore it also > shouldn't be checked. (The only consistent alternative would be to also > check for all currently undefined flags to be clear.) We recently had a similar discussion for the stable libs. Whatever we do, an unexpected corner case needs to break. Checking for all undefined flags up front is far cleaner - absolutely nothing good can come for a guest which set GTF_sub_page with v1, and is expecting it to work, and this way, we do all breaking in one go, rather than breaking $N times in the future as new flags get added. ~Andrew
On 18.02.2021 15:22, Andrew Cooper wrote: > On 17/02/2021 10:46, Jan Beulich wrote: >> Prior to its introduction, v1 entries weren't checked for this flag, and >> the flag also has been meaningless for v1 entries. Therefore it also >> shouldn't be checked. (The only consistent alternative would be to also >> check for all currently undefined flags to be clear.) > > We recently had a similar discussion for the stable libs. > > Whatever we do, an unexpected corner case needs to break. Checking for > all undefined flags up front is far cleaner - absolutely nothing good > can come for a guest which set GTF_sub_page with v1, and is expecting it > to work, and this way, we do all breaking in one go, rather than > breaking $N times in the future as new flags get added. Except that there doesn't need to be any breaking: v1 could continue to ignore all originally undefined flags. v1 and v2 could continue to ignore all flags presently undefined. See also Julien's question about GTF_sub_page on a transitive grant entry. That's presently an ignored setting as well (and, as an implementation detail, in fact getting forced to be set, but with a range covering the entire page). Retroactively starting to check (and reject) undefined flags isn't going to be nice; nevertheless I wanted to put this up as an at least theoretical alternative. Perhaps a topic for the next community call, if I don't forget by the time an agenda page appears. Jan
--- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -762,13 +762,11 @@ static int _set_status_v1(const grant_en struct domain *rd, struct active_grant_entry *act, int readonly, - int mapflag, - domid_t ldomid) + domid_t ldomid) { int rc = GNTST_okay; uint32_t *raw_shah = (uint32_t *)shah; union grant_combo scombo; - uint16_t mask = GTF_type_mask; /* * We bound the number of times we retry CMPXCHG on memory locations that @@ -780,11 +778,6 @@ static int _set_status_v1(const grant_en */ int retries = 0; - /* if this is a grant mapping operation we should ensure GTF_sub_page - is not set */ - if ( mapflag ) - mask |= GTF_sub_page; - scombo.raw = ACCESS_ONCE(*raw_shah); /* @@ -798,8 +791,9 @@ static int _set_status_v1(const grant_en union grant_combo prev, new; /* If not already pinned, check the grant domid and type. */ - if ( !act->pin && (((scombo.flags & mask) != GTF_permit_access) || - (scombo.domid != ldomid)) ) + if ( !act->pin && + (((scombo.flags & GTF_type_mask) != GTF_permit_access) || + (scombo.domid != ldomid)) ) PIN_FAIL(done, GNTST_general_error, "Bad flags (%x) or dom (%d); expected d%d\n", scombo.flags, scombo.domid, ldomid); @@ -916,7 +910,7 @@ static int _set_status(const grant_entry { if ( evaluate_nospec(rgt_version == 1) ) - return _set_status_v1(shah, rd, act, readonly, mapflag, ldomid); + return _set_status_v1(shah, rd, act, readonly, ldomid); else return _set_status_v2(shah, status, rd, act, readonly, mapflag, ldomid); } --- a/xen/include/public/grant_table.h +++ b/xen/include/public/grant_table.h @@ -175,7 +175,7 @@ typedef struct grant_entry_v1 grant_entr * mappings of the grant [GST] * GTF_sub_page: Grant access to only a subrange of the page. @domid * will only be allowed to copy from the grant, and not - * map it. [GST] + * map it. [GST, v2] */ #define _GTF_readonly (2) #define GTF_readonly (1U<<_GTF_readonly)
Prior to its introduction, v1 entries weren't checked for this flag, and the flag also has been meaningless for v1 entries. Therefore it also shouldn't be checked. (The only consistent alternative would be to also check for all currently undefined flags to be clear.) Fixes: b545941b6638 ("Implement sub-page grant support") Signed-off-by: Jan Beulich <jbeulich@suse.com>