[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[xen master] gnttab: adjust pin count overflow checks



commit 8cec5d065c243e54cae97aaa14b0de447883c278
Author:     Jan Beulich <jbeulich@xxxxxxxx>
AuthorDate: Mon Jan 18 12:13:42 2021 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Mon Jan 18 12:13:42 2021 +0100

    gnttab: adjust pin count overflow checks
    
    It's at least odd to check counters which aren't going to be
    incremented, resulting in failure just because prior operations may have
    reached the refcount limit. And it's also not helpful to use open-coded
    literal numbers in these checks.
    
    Calculate the increment values first and derive from them the mask to
    use in the checks.
    
    Also move the pin count checks ahead of the calculation of the status
    (and for copy also sha2) pointers: They're not needed in the failure
    cases, and this way the compiler may also have an easier time keeping
    the variables at least transiently in registers for the subsequent uses.
    
    Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
    Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
 xen/common/grant_table.c | 85 ++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 35 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a5d3ed8bda..3c75116933 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -322,23 +322,38 @@ shared_entry_header(struct grant_table *t, grant_ref_t 
ref)
 
 /* Active grant entry - used for shadowing GTF_permit_access grants. */
 struct active_grant_entry {
-    uint32_t      pin;    /* Reference count information:             */
+/*
+ * 4x byte-wide reference counts, for {host,device}{read,write} mappings,
+ * implemented as a single 32-bit (presumably to optimise checking for any
+ * reference).
+ */
+    uint32_t      pin;
+                          /* Width of the individual counter fields.  */
+#define GNTPIN_cntr_width    8
+#define GNTPIN_cntr_mask     ((1U << GNTPIN_cntr_width) - 1)
                           /* Count of writable host-CPU mappings.     */
 #define GNTPIN_hstw_shift    0
 #define GNTPIN_hstw_inc      (1U << GNTPIN_hstw_shift)
-#define GNTPIN_hstw_mask     (0xFFU << GNTPIN_hstw_shift)
+#define GNTPIN_hstw_mask     (GNTPIN_cntr_mask << GNTPIN_hstw_shift)
                           /* Count of read-only host-CPU mappings.    */
-#define GNTPIN_hstr_shift    8
+#define GNTPIN_hstr_shift    (GNTPIN_hstw_shift + GNTPIN_cntr_width)
 #define GNTPIN_hstr_inc      (1U << GNTPIN_hstr_shift)
-#define GNTPIN_hstr_mask     (0xFFU << GNTPIN_hstr_shift)
+#define GNTPIN_hstr_mask     (GNTPIN_cntr_mask << GNTPIN_hstr_shift)
                           /* Count of writable device-bus mappings.   */
-#define GNTPIN_devw_shift    16
+#define GNTPIN_devw_shift    (GNTPIN_hstr_shift + GNTPIN_cntr_width)
 #define GNTPIN_devw_inc      (1U << GNTPIN_devw_shift)
-#define GNTPIN_devw_mask     (0xFFU << GNTPIN_devw_shift)
+#define GNTPIN_devw_mask     (GNTPIN_cntr_mask << GNTPIN_devw_shift)
                           /* Count of read-only device-bus mappings.  */
-#define GNTPIN_devr_shift    24
+#define GNTPIN_devr_shift    (GNTPIN_devw_shift + GNTPIN_cntr_width)
 #define GNTPIN_devr_inc      (1U << GNTPIN_devr_shift)
-#define GNTPIN_devr_mask     (0xFFU << GNTPIN_devr_shift)
+#define GNTPIN_devr_mask     (GNTPIN_cntr_mask << GNTPIN_devr_shift)
+
+/* Convert a combination of GNTPIN_*_inc to an overflow checking mask. */
+#define GNTPIN_incr2oflow_mask(x) ({                       \
+    ASSERT(!((x) & ~(GNTPIN_hstw_inc | GNTPIN_hstr_inc |   \
+                     GNTPIN_devw_inc | GNTPIN_devr_inc))); \
+    (x) << (GNTPIN_cntr_width - 1);                        \
+})
 
     domid_t       domid;  /* Domain being granted access.             */
     unsigned int  start:15; /* For sub-page grants, the start offset
@@ -988,7 +1003,8 @@ map_grant_ref(
     mfn_t mfn;
     struct page_info *pg = NULL;
     int            rc = GNTST_okay;
-    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0;
+    unsigned int   cache_flags, clear_flags = 0, refcnt = 0, typecnt = 0,
+                   pin_incr = 0;
     bool           host_map_created = false;
     struct active_grant_entry *act = NULL;
     struct grant_mapping *mt;
@@ -998,7 +1014,14 @@ map_grant_ref(
 
     ld = current->domain;
 
-    if ( unlikely((op->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0) )
+    if ( op->flags & GNTMAP_device_map )
+        pin_incr += (op->flags & GNTMAP_readonly) ? GNTPIN_devr_inc
+                                                  : GNTPIN_devw_inc;
+    if ( op->flags & GNTMAP_host_map )
+        pin_incr += (op->flags & GNTMAP_readonly) ? GNTPIN_hstr_inc
+                                                  : GNTPIN_hstw_inc;
+
+    if ( unlikely(!pin_incr) )
     {
         gdprintk(XENLOG_INFO, "Bad flags in grant map op: %x\n", op->flags);
         op->status = GNTST_bad_gntref;
@@ -1052,19 +1075,19 @@ map_grant_ref(
     shah = shared_entry_header(rgt, ref);
     act = active_entry_acquire(rgt, ref);
 
-    /* Make sure we do not access memory speculatively */
-    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
-                                                 : &status_entry(rgt, ref);
-
     /* If already pinned, check the active domid and avoid refcnt overflow. */
     if ( act->pin &&
          ((act->domid != ld->domain_id) ||
-          (act->pin & 0x80808080U) != 0 ||
+          (act->pin & GNTPIN_incr2oflow_mask(pin_incr)) ||
           (act->is_sub_page)) )
         PIN_FAIL(act_release_out, GNTST_general_error,
                  "Bad domain (%d != %d), or risk of counter overflow %08x, or 
subpage %d\n",
                  act->domid, ld->domain_id, act->pin, act->is_sub_page);
 
+    /* Make sure we do not access memory speculatively */
+    status = evaluate_nospec(rgt->gt_version == 1) ? &shah->flags
+                                                   : &status_entry(rgt, ref);
+
     if ( !act->pin ||
          (!(op->flags & GNTMAP_readonly) &&
           !(act->pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask))) )
@@ -1095,12 +1118,7 @@ map_grant_ref(
         }
     }
 
-    if ( op->flags & GNTMAP_device_map )
-        act->pin += (op->flags & GNTMAP_readonly) ?
-            GNTPIN_devr_inc : GNTPIN_devw_inc;
-    if ( op->flags & GNTMAP_host_map )
-        act->pin += (op->flags & GNTMAP_readonly) ?
-            GNTPIN_hstr_inc : GNTPIN_hstw_inc;
+    act->pin += pin_incr;
 
     mfn = act->mfn;
 
@@ -1275,13 +1293,7 @@ map_grant_ref(
     grant_read_lock(rgt);
 
     act = active_entry_acquire(rgt, op->ref);
-
-    if ( op->flags & GNTMAP_device_map )
-        act->pin -= (op->flags & GNTMAP_readonly) ?
-            GNTPIN_devr_inc : GNTPIN_devw_inc;
-    if ( op->flags & GNTMAP_host_map )
-        act->pin -= (op->flags & GNTMAP_readonly) ?
-            GNTPIN_hstr_inc : GNTPIN_hstw_inc;
+    act->pin -= pin_incr;
 
  unlock_out_clear:
     if ( !(op->flags & GNTMAP_readonly) &&
@@ -2516,6 +2528,7 @@ acquire_grant_for_copy(
     uint16_t trans_length;
     bool is_sub_page;
     s16 rc = GNTST_okay;
+    unsigned int pin_incr = readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
     unsigned int clear_flags = 0;
 
     *page = NULL;
@@ -2530,6 +2543,14 @@ acquire_grant_for_copy(
     shah = shared_entry_header(rgt, gref);
     act = active_entry_acquire(rgt, gref);
 
+    /* If already pinned, check the active domid and avoid refcnt overflow. */
+    if ( act->pin &&
+         ((act->domid != ldom) ||
+          (act->pin & GNTPIN_incr2oflow_mask(pin_incr))) )
+        PIN_FAIL(unlock_out, GNTST_general_error,
+                 "Bad domain (%d != %d), or risk of counter overflow %08x\n",
+                 act->domid, ldom, act->pin);
+
     if ( evaluate_nospec(rgt->gt_version == 1) )
     {
         sha2 = NULL;
@@ -2541,12 +2562,6 @@ acquire_grant_for_copy(
         status = &status_entry(rgt, gref);
     }
 
-    /* If already pinned, check the active domid and avoid refcnt overflow. */
-    if ( act->pin && ((act->domid != ldom) || (act->pin & 0x80808080U) != 0) )
-        PIN_FAIL(unlock_out, GNTST_general_error,
-                 "Bad domain (%d != %d), or risk of counter overflow %08x\n",
-                 act->domid, ldom, act->pin);
-
     old_pin = act->pin;
     if ( sha2 && (shah->flags & GTF_type_mask) == GTF_transitive )
     {
@@ -2728,7 +2743,7 @@ acquire_grant_for_copy(
         }
     }
 
-    act->pin += readonly ? GNTPIN_hstr_inc : GNTPIN_hstw_inc;
+    act->pin += pin_incr;
 
     *page_off = act->start;
     *length = act->length;
--
generated by git-patchbot for /home/xen/git/xen.git#master



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.