[Xen-changelog] [xen master] xen/gnttab: Reduce complexity when reading grant_entry_header_t

[patchbot@xxxxxxx]

commit 8dc0b31dc471b6dd45b58a3df225c07b6c08da5f
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Wed Nov 21 18:38:41 2018 +0000
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Fri Jun 28 13:17:53 2019 +0100

    xen/gnttab: Reduce complexity when reading grant_entry_header_t
    
    _set_status_v{1,2}() and gnttab_prepare_for_transfer() read the shared 
header
    by always casting to u32.  Despite grant_entry_header_t only having an
    alignment of 2, this is actually safe because of the grant table ABI itself.
    
    Switch to using an explicit uint32_t *, which removes all subsequent 
casting.
    
    Furthermore, switch to using ACCESS_ONCE() for the read.  There is nothing 
in
    the _set_status_v1() and gnttab_prepare_for_transfer() which prevents the
    compiler from issuing multiple memory reads and creating a TOCTOU race 
around
    the sanity checks, although the worst that can happen is Xen stamping a 
status
    flag over a bad grant entry if the guest is misbehaving.
    
    _set_status_v2() does use barrier() to try avoid multiple reads, but this is
    overkill.  All that matters is that the shared header gets read in one go, 
and
    this allows the compiler more room to optimise.
    
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
 xen/common/grant_table.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 2bbde5cf31..e5d585fcfb 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -679,6 +679,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
                           domid_t  ldomid)
 {
     int rc = GNTST_okay;
+    uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo, prev_scombo, new_scombo;
     uint16_t mask = GTF_type_mask;
 
@@ -697,7 +698,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
     if ( mapflag )
         mask |= GTF_sub_page;
 
-    scombo.word = *(u32 *)shah;
+    scombo.word = ACCESS_ONCE(*raw_shah);
 
     /*
      * This loop attempts to set the access (reading/writing) flags
@@ -728,7 +729,7 @@ static int _set_status_v1(const grant_entry_header_t *shah,
                          "Attempt to write-pin a r/o grant entry\n");
         }
 
-        prev_scombo.word = guest_cmpxchg(rd, (u32 *)shah,
+        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
                                          scombo.word, new_scombo.word);
         if ( likely(prev_scombo.word == scombo.word) )
             break;
@@ -753,17 +754,13 @@ static int _set_status_v2(const grant_entry_header_t 
*shah,
                           domid_t  ldomid)
 {
     int      rc    = GNTST_okay;
+    uint32_t *raw_shah = (uint32_t *)shah;
     union grant_combo scombo;
     uint16_t flags = shah->flags;
     domid_t  id    = shah->domid;
     uint16_t mask  = GTF_type_mask;
 
-    /* we read flags and domid in a single memory access.
-       this avoids the need for another memory barrier to
-       ensure access to these fields are not reordered */
-    scombo.word = *(u32 *)shah;
-    barrier(); /* but we still need to stop the compiler from turning
-                  it back into two reads */
+    scombo.word = ACCESS_ONCE(*raw_shah);
     flags = scombo.shorts.flags;
     id = scombo.shorts.domid;
 
@@ -797,8 +794,7 @@ static int _set_status_v2(const grant_entry_header_t *shah,
        still valid */
     smp_mb();
 
-    scombo.word = *(u32 *)shah;
-    barrier();
+    scombo.word = ACCESS_ONCE(*raw_shah);
     flags = scombo.shorts.flags;
     id = scombo.shorts.domid;
 
@@ -2041,7 +2037,7 @@ gnttab_prepare_for_transfer(
     struct domain *rd, struct domain *ld, grant_ref_t ref)
 {
     struct grant_table *rgt = rd->grant_table;
-    grant_entry_header_t *sha;
+    uint32_t *raw_shah;
     union grant_combo   scombo, prev_scombo, new_scombo;
     int                 retries = 0;
 
@@ -2055,9 +2051,8 @@ gnttab_prepare_for_transfer(
         goto fail;
     }
 
-    sha = shared_entry_header(rgt, ref);
-
-    scombo.word = *(u32 *)&sha->flags;
+    raw_shah = (uint32_t *)shared_entry_header(rgt, ref);
+    scombo.word = ACCESS_ONCE(*raw_shah);
 
     for ( ; ; )
     {
@@ -2074,7 +2069,7 @@ gnttab_prepare_for_transfer(
         new_scombo = scombo;
         new_scombo.shorts.flags |= GTF_transfer_committed;
 
-        prev_scombo.word = guest_cmpxchg(rd, (u32 *)&sha->flags,
+        prev_scombo.word = guest_cmpxchg(rd, raw_shah,
                                          scombo.word, new_scombo.word);
         if ( likely(prev_scombo.word == scombo.word) )
             break;
--
generated by git-patchbot for /home/xen/git/xen.git#master

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/xen-changelog