[v2,2/2] libxl/devd: correctly manipulate the dguest list

Commit Message

Roger Pau Monné May 11, 2017, 10:55 a.m. UTC

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é <[e-mail]>
Reported-by: Reinis Martinsons <[e-mail]>
Suggested-by: Ian Jackson <[e-mail]>
Reviewed-by: Wei Liu <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: Julien Grall <[e-mail]>

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

Comments

Ian Jackson May 11, 2017, 11:13 a.m. UTC | #1

Roger Pau Monne writes ("[PATCH v2 2/2] libxl/devd: correctly manipulate the dguest list"):
> Current code in backend_watch_callback has two issues when manipulating the
> dguest list:
...
>  skip:
>      libxl__nested_ao_free(nested_ao);
> +clean:
>      if (ddev)
>          free(ddev->dev);

This is starting to be quite goto-rich, and the memory ownership rules
become less clear.  Rather than try to analyse this in detail, I
wonder if it would be better to try to rework this so that it fits
CODING_STYLE better.

Wei, what do you think ?

>      free(ddev);
> -    free(dguest);
> +    if (dguest != NULL &&
> +        dguest->num_vifs + dguest->num_vbds + dguest->num_qdisks == 0) {

Perhaps this cleanup functionality could become a function if its own.
check_maybe_free_dguest or something ?

I haven't gone through the code in detail trying to convince myself
it's OK.  Even if there are to be no significant code changes, I would
like to see the memory ownership and lifetime rules here written down
(in comments in the code).  That way readers wouldn't have to
reverse-engineer them, and bugs will be clearer.

Ian.

Wei Liu May 11, 2017, 11:39 a.m. UTC | #2

On Thu, May 11, 2017 at 12:13:00PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH v2 2/2] libxl/devd: correctly manipulate the dguest list"):
> > Current code in backend_watch_callback has two issues when manipulating the
> > dguest list:
> ...
> >  skip:
> >      libxl__nested_ao_free(nested_ao);
> > +clean:
> >      if (ddev)
> >          free(ddev->dev);
> 
> This is starting to be quite goto-rich, and the memory ownership rules
> become less clear.  Rather than try to analyse this in detail, I
> wonder if it would be better to try to rework this so that it fits
> CODING_STYLE better.
> 
> Wei, what do you think ?
> 

No objection from me, of course.

Patch

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;
 }