[Xen-devel] [PATCH v2 2/2] libxl/devd: correctly manipulate the dguest list

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

Current code in backend_watch_callback has two issues when manipulating the
dguest list:

1. backend_watch_callback forgets to remove a libxl__ddomain_guest from the
list of tracked domains when the related data is freed, causing dereferences
later on when the list is traversed. Make sure that a domain is always removed
from the list when freed.

2. A spurious device state change can cause a dguest to be freed, with active
devices and without being removed from the list. Fix this by always checking if
a dguest has active devices before freeing and removing it.

Let me know if you want me to resend the patch or if you will fix the message
while committing.

Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
Reported-by: Reinis Martinsons <admin@xxxxxx>
Suggested-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>
---
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>

Changes since v1:
 - Fix commit message
---
 tools/libxl/libxl_device.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index cd4ad05a6f..8417198081 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1602,7 +1602,7 @@ static void backend_watch_callback(libxl__egc *egc, 
libxl__ev_xswatch *watch,
     STATE_AO_GC(nested_ao);
     char *p, *path;
     const char *sstate, *sonline;
-    int state, online, rc, num_devs;
+    int state, online, rc;
     libxl__device *dev;
     libxl__ddomain_device *ddev = NULL;
     libxl__ddomain_guest *dguest = NULL;
@@ -1684,21 +1684,9 @@ static void backend_watch_callback(libxl__egc *egc, 
libxl__ev_xswatch *watch,
              path);
         rc = remove_device(egc, nested_ao, dguest, ddev);
         if (rc > 0)
-            free_ao = true;
+            libxl__nested_ao_free(nested_ao);
 
-        free(ddev->dev);
-        free(ddev);
-        /* If this was the last device in the domain, remove it from the list 
*/
-        num_devs = dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks;
-        if (num_devs == 0) {
-            LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
-                               next);
-            LOGD(DEBUG, dguest->domid, "Removed domain from the list of active 
guests");
-            /* Clear any leftovers in libxl/<domid> */
-            libxl__xs_rm_checked(gc, XBT_NULL,
-                                 GCSPRINTF("libxl/%u", dguest->domid));
-            free(dguest);
-        }
+        goto clean;
     }
 
     if (free_ao)
@@ -1708,10 +1696,20 @@ static void backend_watch_callback(libxl__egc *egc, 
libxl__ev_xswatch *watch,
 
 skip:
     libxl__nested_ao_free(nested_ao);
+clean:
     if (ddev)
         free(ddev->dev);
     free(ddev);
-    free(dguest);
+    if (dguest != NULL &&
+        dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks == 0) {
+        LIBXL_SLIST_REMOVE(&ddomain->guests, dguest, libxl__ddomain_guest,
+                           next);
+        LOGD(DEBUG, dguest->domid, "Removed domain from the list of active 
guests");
+        /* Clear any leftovers in libxl/<domid> */
+        libxl__xs_rm_checked(gc, XBT_NULL,
+                             GCSPRINTF("libxl/%u", dguest->domid));
+        free(dguest);
+    }
     return;
 }
 
-- 
2.11.0 (Apple Git-81)


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel