[Qemu-devel] [PATCH v2 0/3] xen: fix a potential crash in xen-bus

[Qemu-devel] [PATCH v2 0/3] xen: fix a potential crash in xen-bus

[Paul Durrant]

This series fixes a potential segfault caused by NotifierList corruption
in xen-bus. The first two patches lay the groundwork and the third
actually fixes the problem.

Paul Durrant (3):
  xen / notify: introduce a new XenWatchList abstraction
  xen: introduce separate XenWatchList for XenDevice objects
  xen: perform XenDevice clean-up in XenBus watch handler

 hw/xen/trace-events      |   9 +-
 hw/xen/xen-bus.c         | 267 ++++++++++++++++++++++++++++-----------
 include/hw/xen/xen-bus.h |   7 +-
 include/qemu/notify.h    |   2 +
 util/notify.c            |   5 +
 5 files changed, 213 insertions(+), 77 deletions(-)
---
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
-- 
2.20.1.2.gb21ebb6

[Qemu-devel] [PATCH v2 1/3] xen / notify: introduce a new XenWatchList abstraction

[Paul Durrant]

Xenstore watch call-backs are already abstracted away from XenBus using
the XenWatch data structure but the associated NotifierList manipulation
and file handle registration is still open coded in various xen_bus_...()
functions.
This patch creates a new XenWatchList data structure to allow these
interactions to be abstracted away from XenBus as well. This is in
preparation for a subsequent patch which will introduce separate watch lists
for XenBus and XenDevice objects.

NOTE: This patch also introduces a new notifier_list_empty() helper function
      for the purposes of adding an assertion that a XenWatchList is not
      freed whilst its associated NotifierList is still occupied.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Anthony Perard <anthony.perard@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
 hw/xen/trace-events      |   5 +-
 hw/xen/xen-bus.c         | 117 +++++++++++++++++++++++++--------------
 include/hw/xen/xen-bus.h |   3 +-
 include/qemu/notify.h    |   2 +
 util/notify.c            |   5 ++
 5 files changed, 87 insertions(+), 45 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index bc82ecb1a5..ac8d9c20d2 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -19,9 +19,8 @@ xen_bus_unrealize(void) ""
 xen_bus_enumerate(void) ""
 xen_bus_type_enumerate(const char *type) "type: %s"
 xen_bus_backend_create(const char *type, const char *path) "type: %s path: %s"
-xen_bus_add_watch(const char *node, const char *key, char *token) "node: %s key: %s token: %s"
-xen_bus_remove_watch(const char *node, const char *key, char *token) "node: %s key: %s token: %s"
-xen_bus_watch(const char *token) "token: %s"
+xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
+xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
 xen_device_realize(const char *type, char *name) "type: %s name: %s"
 xen_device_unrealize(const char *type, char *name) "type: %s name: %s"
 xen_device_backend_state(const char *type, char *name, const char *state) "type: %s name: %s -> %s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 025df5e59f..28efaccff2 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -157,18 +157,60 @@ static void free_watch(XenWatch *watch)
     g_free(watch);
 }
 
-static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
-                                   const char *key, XenWatchHandler handler,
-                                   void *opaque, Error **errp)
+struct XenWatchList {
+    struct xs_handle *xsh;
+    NotifierList notifiers;
+};
+
+static void watch_list_event(void *opaque)
+{
+    XenWatchList *watch_list = opaque;
+    char **v;
+    const char *token;
+
+    v = xs_check_watch(watch_list->xsh);
+    if (!v) {
+        return;
+    }
+
+    token = v[XS_WATCH_TOKEN];
+
+    notifier_list_notify(&watch_list->notifiers, (void *)token);
+
+    free(v);
+}
+
+static XenWatchList *watch_list_create(struct xs_handle *xsh)
+{
+    XenWatchList *watch_list = g_new0(XenWatchList, 1);
+
+    g_assert(xsh);
+
+    watch_list->xsh = xsh;
+    notifier_list_init(&watch_list->notifiers);
+    qemu_set_fd_handler(xs_fileno(watch_list->xsh), watch_list_event, NULL,
+                        watch_list);
+
+    return watch_list;
+}
+
+static void watch_list_destroy(XenWatchList *watch_list)
+{
+    g_assert(notifier_list_empty(&watch_list->notifiers));
+    qemu_set_fd_handler(xs_fileno(watch_list->xsh), NULL, NULL, NULL);
+    g_free(watch_list);
+}
+
+static XenWatch *watch_list_add(XenWatchList *watch_list, const char *node,
+                                const char *key, XenWatchHandler handler,
+                                void *opaque, Error **errp)
 {
     XenWatch *watch = new_watch(node, key, handler, opaque);
     Error *local_err = NULL;
 
-    trace_xen_bus_add_watch(watch->node, watch->key, watch->token);
-
-    notifier_list_add(&xenbus->watch_notifiers, &watch->notifier);
+    notifier_list_add(&watch_list->notifiers, &watch->notifier);
 
-    xs_node_watch(xenbus->xsh, node, key, watch->token, &local_err);
+    xs_node_watch(watch_list->xsh, node, key, watch->token, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
 
@@ -181,18 +223,34 @@ static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
     return watch;
 }
 
-static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
-                                 Error **errp)
+static void watch_list_remove(XenWatchList *watch_list, XenWatch *watch,
+                              Error **errp)
 {
-    trace_xen_bus_remove_watch(watch->node, watch->key, watch->token);
-
-    xs_node_unwatch(xenbus->xsh, watch->node, watch->key, watch->token,
+    xs_node_unwatch(watch_list->xsh, watch->node, watch->key, watch->token,
                     errp);
 
     notifier_remove(&watch->notifier);
     free_watch(watch);
 }
 
+static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
+                                   const char *key, XenWatchHandler handler,
+                                   void *opaque, Error **errp)
+{
+    trace_xen_bus_add_watch(node, key);
+
+    return watch_list_add(xenbus->watch_list, node, key, handler, opaque,
+                          errp);
+}
+
+static void xen_bus_remove_watch(XenBus *xenbus, XenWatch *watch,
+                                 Error **errp)
+{
+    trace_xen_bus_remove_watch(watch->node, watch->key);
+
+    watch_list_remove(xenbus->watch_list, watch, errp);
+}
+
 static void xen_bus_backend_create(XenBus *xenbus, const char *type,
                                    const char *name, char *path,
                                    Error **errp)
@@ -338,35 +396,14 @@ static void xen_bus_unrealize(BusState *bus, Error **errp)
         xenbus->backend_watch = NULL;
     }
 
-    if (!xenbus->xsh) {
-        return;
+    if (xenbus->watch_list) {
+        watch_list_destroy(xenbus->watch_list);
+        xenbus->watch_list = NULL;
     }
 
-    qemu_set_fd_handler(xs_fileno(xenbus->xsh), NULL, NULL, NULL);
-
-    xs_close(xenbus->xsh);
-}
-
-static void xen_bus_watch(void *opaque)
-{
-    XenBus *xenbus = opaque;
-    char **v;
-    const char *token;
-
-    g_assert(xenbus->xsh);
-
-    v = xs_check_watch(xenbus->xsh);
-    if (!v) {
-        return;
+    if (xenbus->xsh) {
+        xs_close(xenbus->xsh);
     }
-
-    token = v[XS_WATCH_TOKEN];
-
-    trace_xen_bus_watch(token);
-
-    notifier_list_notify(&xenbus->watch_notifiers, (void *)token);
-
-    free(v);
 }
 
 static void xen_bus_realize(BusState *bus, Error **errp)
@@ -390,9 +427,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
         xenbus->backend_id = 0; /* Assume lack of node means dom0 */
     }
 
-    notifier_list_init(&xenbus->watch_notifiers);
-    qemu_set_fd_handler(xs_fileno(xenbus->xsh), xen_bus_watch, NULL,
-                        xenbus);
+    xenbus->watch_list = watch_list_create(xenbus->xsh);
 
     module_call_init(MODULE_INIT_XEN_BACKEND);
 
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 1c2d9dfdb8..88b84e29bb 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -14,6 +14,7 @@
 
 typedef void (*XenWatchHandler)(void *opaque);
 
+typedef struct XenWatchList XenWatchList;
 typedef struct XenWatch XenWatch;
 typedef struct XenEventChannel XenEventChannel;
 
@@ -63,7 +64,7 @@ typedef struct XenBus {
     BusState qbus;
     domid_t backend_id;
     struct xs_handle *xsh;
-    NotifierList watch_notifiers;
+    XenWatchList *watch_list;
     XenWatch *backend_watch;
 } XenBus;
 
diff --git a/include/qemu/notify.h b/include/qemu/notify.h
index a3d73e4bc7..bcfa70fb2e 100644
--- a/include/qemu/notify.h
+++ b/include/qemu/notify.h
@@ -40,6 +40,8 @@ void notifier_remove(Notifier *notifier);
 
 void notifier_list_notify(NotifierList *list, void *data);
 
+bool notifier_list_empty(NotifierList *list);
+
 /* Same as Notifier but allows .notify() to return errors */
 typedef struct NotifierWithReturn NotifierWithReturn;
 
diff --git a/util/notify.c b/util/notify.c
index aee8d93cb0..76bab212ae 100644
--- a/util/notify.c
+++ b/util/notify.c
@@ -40,6 +40,11 @@ void notifier_list_notify(NotifierList *list, void *data)
     }
 }
 
+bool notifier_list_empty(NotifierList *list)
+{
+    return QLIST_EMPTY(&list->notifiers);
+}
+
 void notifier_with_return_list_init(NotifierWithReturnList *list)
 {
     QLIST_INIT(&list->notifiers);
-- 
2.20.1.2.gb21ebb6

[Qemu-devel] [PATCH v2 2/3] xen: introduce separate XenWatchList for XenDevice objects

[Paul Durrant]

This patch uses the XenWatchList abstraction to add a separate watch list
for each device. This is more scalable than walking a single notifier
list for all watches and is also necessary to implement a bug-fix in a
subsequent patch.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Anthony Perard <anthony.perard@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
 hw/xen/trace-events      |  2 ++
 hw/xen/xen-bus.c         | 72 ++++++++++++++++++++++++++++++++--------
 include/hw/xen/xen-bus.h |  2 ++
 3 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index ac8d9c20d2..80ce3dafad 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -29,6 +29,8 @@ xen_device_backend_changed(const char *type, char *name) "type: %s name: %s"
 xen_device_frontend_state(const char *type, char *name, const char *state) "type: %s name: %s -> %s"
 xen_device_frontend_changed(const char *type, char *name) "type: %s name: %s"
 xen_device_unplug(const char *type, char *name) "type: %s name: %s"
+xen_device_add_watch(const char *type, char *name, const char *node, const char *key) "type: %s name: %s node: %s key: %s"
+xen_device_remove_watch(const char *type, char *name, const char *node, const char *key) "type: %s name: %s node: %s key: %s"
 
 # xen-bus-helper.c
 xs_node_create(const char *node) "%s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 28efaccff2..810a4e2df3 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -235,11 +235,11 @@ static void watch_list_remove(XenWatchList *watch_list, XenWatch *watch,
 
 static XenWatch *xen_bus_add_watch(XenBus *xenbus, const char *node,
                                    const char *key, XenWatchHandler handler,
-                                   void *opaque, Error **errp)
+                                   Error **errp)
 {
     trace_xen_bus_add_watch(node, key);
 
-    return watch_list_add(xenbus->watch_list, node, key, handler, opaque,
+    return watch_list_add(xenbus->watch_list, node, key, handler, xenbus,
                           errp);
 }
 
@@ -433,7 +433,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
 
     xenbus->backend_watch =
         xen_bus_add_watch(xenbus, "", /* domain root node */
-                          "backend", xen_bus_enumerate, xenbus, &local_err);
+                          "backend", xen_bus_enumerate, &local_err);
     if (local_err) {
         /* This need not be treated as a hard error so don't propagate */
         error_reportf_err(local_err,
@@ -621,6 +621,31 @@ static void xen_device_backend_changed(void *opaque)
     }
 }
 
+static XenWatch *xen_device_add_watch(XenDevice *xendev, const char *node,
+                                      const char *key,
+                                      XenWatchHandler handler,
+                                      Error **errp)
+{
+    const char *type = object_get_typename(OBJECT(xendev));
+
+    trace_xen_device_add_watch(type, xendev->name, node, key);
+
+    return watch_list_add(xendev->watch_list, node, key, handler, xendev,
+                          errp);
+}
+
+static void xen_device_remove_watch(XenDevice *xendev, XenWatch *watch,
+                                    Error **errp)
+{
+    const char *type = object_get_typename(OBJECT(xendev));
+
+    trace_xen_device_remove_watch(type, xendev->name, watch->node,
+                                  watch->key);
+
+    watch_list_remove(xendev->watch_list, watch, errp);
+}
+
+
 static void xen_device_backend_create(XenDevice *xendev, Error **errp)
 {
     XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
@@ -645,9 +670,9 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
     }
 
     xendev->backend_state_watch =
-        xen_bus_add_watch(xenbus, xendev->backend_path,
-                          "state", xen_device_backend_changed,
-                          xendev, &local_err);
+        xen_device_add_watch(xendev, xendev->backend_path,
+                             "state", xen_device_backend_changed,
+                             &local_err);
     if (local_err) {
         error_propagate_prepend(errp, local_err,
                                 "failed to watch backend state: ");
@@ -655,9 +680,9 @@ static void xen_device_backend_create(XenDevice *xendev, Error **errp)
     }
 
     xendev->backend_online_watch =
-        xen_bus_add_watch(xenbus, xendev->backend_path,
-                          "online", xen_device_backend_changed,
-                          xendev, &local_err);
+        xen_device_add_watch(xendev, xendev->backend_path,
+                             "online", xen_device_backend_changed,
+                             &local_err);
     if (local_err) {
         error_propagate_prepend(errp, local_err,
                                 "failed to watch backend online: ");
@@ -671,12 +696,12 @@ static void xen_device_backend_destroy(XenDevice *xendev)
     Error *local_err = NULL;
 
     if (xendev->backend_online_watch) {
-        xen_bus_remove_watch(xenbus, xendev->backend_online_watch, NULL);
+        xen_device_remove_watch(xendev, xendev->backend_online_watch, NULL);
         xendev->backend_online_watch = NULL;
     }
 
     if (xendev->backend_state_watch) {
-        xen_bus_remove_watch(xenbus, xendev->backend_state_watch, NULL);
+        xen_device_remove_watch(xendev, xendev->backend_state_watch, NULL);
         xendev->backend_state_watch = NULL;
     }
 
@@ -812,8 +837,8 @@ static void xen_device_frontend_create(XenDevice *xendev, Error **errp)
     }
 
     xendev->frontend_state_watch =
-        xen_bus_add_watch(xenbus, xendev->frontend_path, "state",
-                          xen_device_frontend_changed, xendev, &local_err);
+        xen_device_add_watch(xendev, xendev->frontend_path, "state",
+                             xen_device_frontend_changed, &local_err);
     if (local_err) {
         error_propagate_prepend(errp, local_err,
                                 "failed to watch frontend state: ");
@@ -826,7 +851,8 @@ static void xen_device_frontend_destroy(XenDevice *xendev)
     Error *local_err = NULL;
 
     if (xendev->frontend_state_watch) {
-        xen_bus_remove_watch(xenbus, xendev->frontend_state_watch, NULL);
+        xen_device_remove_watch(xendev, xendev->frontend_state_watch,
+                                NULL);
         xendev->frontend_state_watch = NULL;
     }
 
@@ -1122,6 +1148,16 @@ static void xen_device_unrealize(DeviceState *dev, Error **errp)
         xendev->xgth = NULL;
     }
 
+    if (xendev->watch_list) {
+        watch_list_destroy(xendev->watch_list);
+        xendev->watch_list = NULL;
+    }
+
+    if (xendev->xsh) {
+        xs_close(xendev->xsh);
+        xendev->xsh = NULL;
+    }
+
     g_free(xendev->name);
     xendev->name = NULL;
 }
@@ -1164,6 +1200,14 @@ static void xen_device_realize(DeviceState *dev, Error **errp)
 
     trace_xen_device_realize(type, xendev->name);
 
+    xendev->xsh = xs_open(0);
+    if (!xendev->xsh) {
+        error_setg_errno(errp, errno, "failed xs_open");
+        goto unrealize;
+    }
+
+    xendev->watch_list = watch_list_create(xendev->xsh);
+
     xendev->xgth = xengnttab_open(NULL, 0);
     if (!xendev->xgth) {
         error_setg_errno(errp, errno, "failed xengnttab_open");
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 88b84e29bb..0d198148f6 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -22,6 +22,8 @@ typedef struct XenDevice {
     DeviceState qdev;
     domid_t frontend_id;
     char *name;
+    struct xs_handle *xsh;
+    XenWatchList *watch_list;
     char *backend_path, *frontend_path;
     enum xenbus_state backend_state, frontend_state;
     Notifier exit;
-- 
2.20.1.2.gb21ebb6

[Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler

[Paul Durrant]

Cleaning up offine XenDevice objects directly in
xen_device_backend_changed() is dangerous as xen_device_unrealize() will
modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
used in notifier_list_notify() is insufficient as *two* notifiers (for
the frontend and backend watches) are removed, thus potentially rendering
the 'next' pointer unsafe.

The solution is to use the XenBus backend_watch handler to do the clean-up
instead, as it is invoked whilst walking a separate watch list.

This patch therefore adds a new 'offline_devices' list to XenBus, to which
offline devices are added by xen_device_backend_changed(). The XenBus
backend_watch registration is also changed to not only invoke
xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
walk 'offline_devices' and perform the necessary actions.
For safety a an extra 'online' check is also added to
xen_bus_type_enumerate() to make sure that no attempt is made to create a
new XenDevice object for a backend that is offline.

NOTE: This patch also include some cosmetic changes:
      - substitute the local variable name 'backend_state'
        in xen_bus_type_enumerate() with 'state', since there
        is no ambiguity with any other state in that context.
      - change xen_device_state_is_active() to
        xen_device_frontend_is_active() (and pass a XenDevice directly)
        since the state tests contained therein only apply to a frontend.
      - use 'state' rather then 'xendev->backend_state' in
        xen_device_backend_changed() to shorten the code.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>

v2:
 - Make sure we don't try to add a XenDevice to 'offline_devices' more than
   once
---
 hw/xen/trace-events      |  2 +
 hw/xen/xen-bus.c         | 92 +++++++++++++++++++++++++++++-----------
 include/hw/xen/xen-bus.h |  2 +
 3 files changed, 71 insertions(+), 25 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 80ce3dafad..e6885bc751 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -17,8 +17,10 @@ xen_domid_restrict(int err) "err: %u"
 xen_bus_realize(void) ""
 xen_bus_unrealize(void) ""
 xen_bus_enumerate(void) ""
+xen_bus_cleanup(void) ""
 xen_bus_type_enumerate(const char *type) "type: %s"
 xen_bus_backend_create(const char *type, const char *path) "type: %s path: %s"
+xen_bus_device_cleanup(const char *type, char *name) "type: %s name: %s"
 xen_bus_add_watch(const char *node, const char *key) "node: %s key: %s"
 xen_bus_remove_watch(const char *node, const char *key) "node: %s key: %s"
 xen_device_realize(const char *type, char *name) "type: %s name: %s"
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 810a4e2df3..43e29b8771 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -340,13 +340,18 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
     for (i = 0; i < n; i++) {
         char *backend_path = g_strdup_printf("%s/%s", domain_path,
                                              backend[i]);
-        enum xenbus_state backend_state;
+        enum xenbus_state state;
+        unsigned int online;
 
         if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "state",
-                          NULL, "%u", &backend_state) != 1)
-            backend_state = XenbusStateUnknown;
+                          NULL, "%u", &state) != 1)
+            state = XenbusStateUnknown;
 
-        if (backend_state == XenbusStateInitialising) {
+        if (xs_node_scanf(xenbus->xsh, XBT_NULL, backend_path, "online",
+                          NULL, "%u", &online) != 1)
+            online = 0;
+
+        if (online && state == XenbusStateInitialising) {
             Error *local_err = NULL;
 
             xen_bus_backend_create(xenbus, type, backend[i], backend_path,
@@ -365,9 +370,8 @@ out:
     g_free(domain_path);
 }
 
-static void xen_bus_enumerate(void *opaque)
+static void xen_bus_enumerate(XenBus *xenbus)
 {
-    XenBus *xenbus = opaque;
     char **type;
     unsigned int i, n;
 
@@ -385,6 +389,44 @@ static void xen_bus_enumerate(void *opaque)
     free(type);
 }
 
+static void xen_bus_device_cleanup(XenDevice *xendev)
+{
+    const char *type = object_get_typename(OBJECT(xendev));
+    Error *local_err = NULL;
+
+    trace_xen_bus_device_cleanup(type, xendev->name);
+
+    g_assert(!xendev->backend_online);
+
+    if (!xen_backend_try_device_destroy(xendev, &local_err)) {
+        object_unparent(OBJECT(xendev));
+    }
+
+    if (local_err) {
+        error_report_err(local_err);
+    }
+}
+
+static void xen_bus_cleanup(XenBus *xenbus)
+{
+    XenDevice *xendev, *next;
+
+    trace_xen_bus_cleanup();
+
+    QLIST_FOREACH_SAFE(xendev, &xenbus->offline_devices, list, next) {
+        QLIST_REMOVE(xendev, list);
+        xen_bus_device_cleanup(xendev);
+    }
+}
+
+static void xen_bus_backend_changed(void *opaque)
+{
+    XenBus *xenbus = opaque;
+
+    xen_bus_enumerate(xenbus);
+    xen_bus_cleanup(xenbus);
+}
+
 static void xen_bus_unrealize(BusState *bus, Error **errp)
 {
     XenBus *xenbus = XEN_BUS(bus);
@@ -433,7 +475,7 @@ static void xen_bus_realize(BusState *bus, Error **errp)
 
     xenbus->backend_watch =
         xen_bus_add_watch(xenbus, "", /* domain root node */
-                          "backend", xen_bus_enumerate, &local_err);
+                          "backend", xen_bus_backend_changed, &local_err);
     if (local_err) {
         /* This need not be treated as a hard error so don't propagate */
         error_reportf_err(local_err,
@@ -555,9 +597,9 @@ static void xen_device_backend_set_online(XenDevice *xendev, bool online)
  * Tell from the state whether the frontend is likely alive,
  * i.e. it will react to a change of state of the backend.
  */
-static bool xen_device_state_is_active(enum xenbus_state state)
+static bool xen_device_frontend_is_active(XenDevice *xendev)
 {
-    switch (state) {
+    switch (xendev->frontend_state) {
     case XenbusStateInitWait:
     case XenbusStateInitialised:
     case XenbusStateConnected:
@@ -594,30 +636,30 @@ static void xen_device_backend_changed(void *opaque)
      * state to Closing, but there is no active frontend then set the
      * backend state to Closed.
      */
-    if (xendev->backend_state == XenbusStateClosing &&
-        !xen_device_state_is_active(xendev->frontend_state)) {
+    if (state == XenbusStateClosing &&
+        !xen_device_frontend_is_active(xendev)) {
         xen_device_backend_set_state(xendev, XenbusStateClosed);
     }
 
     /*
      * If a backend is still 'online' then we should leave it alone but,
-     * if a backend is not 'online', then the device should be destroyed
-     * once the state is Closed.
+     * if a backend is not 'online', then the device is a candidate
+     * for destruction. Hence add it to the 'offline' list to be cleaned
+     * by xen_bus_cleanup().
      */
-    if (!xendev->backend_online &&
-        (xendev->backend_state == XenbusStateClosed ||
-         xendev->backend_state == XenbusStateInitialising ||
-         xendev->backend_state == XenbusStateInitWait ||
-         xendev->backend_state == XenbusStateUnknown)) {
-        Error *local_err = NULL;
+    if (!online &&
+        (state == XenbusStateClosed ||  state == XenbusStateInitialising ||
+         state == XenbusStateInitWait || state == XenbusStateUnknown) &&
+        !QLIST_NEXT(xendev, list)) {
+        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
 
-        if (!xen_backend_try_device_destroy(xendev, &local_err)) {
-            object_unparent(OBJECT(xendev));
-        }
+        QLIST_INSERT_HEAD(&xenbus->offline_devices, xendev, list);
 
-        if (local_err) {
-            error_report_err(local_err);
-        }
+        /*
+         * Re-write the state to cause a XenBus backend_watch notification,
+         * resulting in a call to xen_bus_cleanup().
+         */
+        xen_device_backend_printf(xendev, "state", "%u", state);
     }
 }
 
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 0d198148f6..15d71aff70 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -33,6 +33,7 @@ typedef struct XenDevice {
     xengnttab_handle *xgth;
     bool feature_grant_copy;
     QLIST_HEAD(, XenEventChannel) event_channels;
+    QLIST_ENTRY(XenDevice) list;
 } XenDevice;
 
 typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
@@ -68,6 +69,7 @@ typedef struct XenBus {
     struct xs_handle *xsh;
     XenWatchList *watch_list;
     XenWatch *backend_watch;
+    QLIST_HEAD(, XenDevice) offline_devices;
 } XenBus;
 
 typedef struct XenBusClass {
-- 
2.20.1.2.gb21ebb6

Re: [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler

[Anthony PERARD]

On Thu, Sep 12, 2019 at 01:16:46PM +0100, Paul Durrant wrote:
> Cleaning up offine XenDevice objects directly in
              ^ offline

> xen_device_backend_changed() is dangerous as xen_device_unrealize() will
> modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
> used in notifier_list_notify() is insufficient as *two* notifiers (for
> the frontend and backend watches) are removed, thus potentially rendering
> the 'next' pointer unsafe.
> 
> The solution is to use the XenBus backend_watch handler to do the clean-up
> instead, as it is invoked whilst walking a separate watch list.
> 
> This patch therefore adds a new 'offline_devices' list to XenBus, to which
> offline devices are added by xen_device_backend_changed(). The XenBus
> backend_watch registration is also changed to not only invoke
> xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
> walk 'offline_devices' and perform the necessary actions.
> For safety a an extra 'online' check is also added to
             ^ one 'a' too many?

> xen_bus_type_enumerate() to make sure that no attempt is made to create a
> new XenDevice object for a backend that is offline.
> 
> NOTE: This patch also include some cosmetic changes:
>       - substitute the local variable name 'backend_state'
>         in xen_bus_type_enumerate() with 'state', since there
>         is no ambiguity with any other state in that context.
>       - change xen_device_state_is_active() to
>         xen_device_frontend_is_active() (and pass a XenDevice directly)
>         since the state tests contained therein only apply to a frontend.
>       - use 'state' rather then 'xendev->backend_state' in
>         xen_device_backend_changed() to shorten the code.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> 
> v2:
>  - Make sure we don't try to add a XenDevice to 'offline_devices' more than
>    once
> ---
>  
>      /*
>       * If a backend is still 'online' then we should leave it alone but,
> -     * if a backend is not 'online', then the device should be destroyed
> -     * once the state is Closed.
> +     * if a backend is not 'online', then the device is a candidate
> +     * for destruction. Hence add it to the 'offline' list to be cleaned
> +     * by xen_bus_cleanup().
>       */
> -    if (!xendev->backend_online &&
> -        (xendev->backend_state == XenbusStateClosed ||
> -         xendev->backend_state == XenbusStateInitialising ||
> -         xendev->backend_state == XenbusStateInitWait ||
> -         xendev->backend_state == XenbusStateUnknown)) {
> -        Error *local_err = NULL;
> +    if (!online &&
> +        (state == XenbusStateClosed ||  state == XenbusStateInitialising ||
> +         state == XenbusStateInitWait || state == XenbusStateUnknown) &&
> +        !QLIST_NEXT(xendev, list)) {

Could you add a note about this QLIST_NEXT? I don't think it's going to
be obvious enough why we check that there are no next item. I might only
understand it just because of your reply to the v1 of the patch.
(Well the changelog of the patch also point out what it's for.)

> +        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
>  
> -        if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> -            object_unparent(OBJECT(xendev));
> -        }
> +        QLIST_INSERT_HEAD(&xenbus->offline_devices, xendev, list);
>  
> -        if (local_err) {
> -            error_report_err(local_err);
> -        }
> +        /*
> +         * Re-write the state to cause a XenBus backend_watch notification,
> +         * resulting in a call to xen_bus_cleanup().
> +         */
> +        xen_device_backend_printf(xendev, "state", "%u", state);

It kind of feels wrong to rely on xenstore to notify QEMU's xenbus
driver that a device needs cleanup. But that does the job.

With a note about the use of QLIST_NEXT and the few typo fixed, the
patch will be good to go.

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 12 September 2019 16:04
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler
> 
> On Thu, Sep 12, 2019 at 01:16:46PM +0100, Paul Durrant wrote:
> > Cleaning up offine XenDevice objects directly in
>               ^ offline
> 
> > xen_device_backend_changed() is dangerous as xen_device_unrealize() will
> > modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
> > used in notifier_list_notify() is insufficient as *two* notifiers (for
> > the frontend and backend watches) are removed, thus potentially rendering
> > the 'next' pointer unsafe.
> >
> > The solution is to use the XenBus backend_watch handler to do the clean-up
> > instead, as it is invoked whilst walking a separate watch list.
> >
> > This patch therefore adds a new 'offline_devices' list to XenBus, to which
> > offline devices are added by xen_device_backend_changed(). The XenBus
> > backend_watch registration is also changed to not only invoke
> > xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
> > walk 'offline_devices' and perform the necessary actions.
> > For safety a an extra 'online' check is also added to
>              ^ one 'a' too many?
> 
> > xen_bus_type_enumerate() to make sure that no attempt is made to create a
> > new XenDevice object for a backend that is offline.
> >
> > NOTE: This patch also include some cosmetic changes:
> >       - substitute the local variable name 'backend_state'
> >         in xen_bus_type_enumerate() with 'state', since there
> >         is no ambiguity with any other state in that context.
> >       - change xen_device_state_is_active() to
> >         xen_device_frontend_is_active() (and pass a XenDevice directly)
> >         since the state tests contained therein only apply to a frontend.
> >       - use 'state' rather then 'xendev->backend_state' in
> >         xen_device_backend_changed() to shorten the code.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> >
> > v2:
> >  - Make sure we don't try to add a XenDevice to 'offline_devices' more than
> >    once
> > ---
> >
> >      /*
> >       * If a backend is still 'online' then we should leave it alone but,
> > -     * if a backend is not 'online', then the device should be destroyed
> > -     * once the state is Closed.
> > +     * if a backend is not 'online', then the device is a candidate
> > +     * for destruction. Hence add it to the 'offline' list to be cleaned
> > +     * by xen_bus_cleanup().
> >       */
> > -    if (!xendev->backend_online &&
> > -        (xendev->backend_state == XenbusStateClosed ||
> > -         xendev->backend_state == XenbusStateInitialising ||
> > -         xendev->backend_state == XenbusStateInitWait ||
> > -         xendev->backend_state == XenbusStateUnknown)) {
> > -        Error *local_err = NULL;
> > +    if (!online &&
> > +        (state == XenbusStateClosed ||  state == XenbusStateInitialising ||
> > +         state == XenbusStateInitWait || state == XenbusStateUnknown) &&
> > +        !QLIST_NEXT(xendev, list)) {
> 
> Could you add a note about this QLIST_NEXT? I don't think it's going to
> be obvious enough why we check that there are no next item. I might only
> understand it just because of your reply to the v1 of the patch.
> (Well the changelog of the patch also point out what it's for.)
> 

Sure, it's worth a comment.

> > +        XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
> >
> > -        if (!xen_backend_try_device_destroy(xendev, &local_err)) {
> > -            object_unparent(OBJECT(xendev));
> > -        }
> > +        QLIST_INSERT_HEAD(&xenbus->offline_devices, xendev, list);
> >
> > -        if (local_err) {
> > -            error_report_err(local_err);
> > -        }
> > +        /*
> > +         * Re-write the state to cause a XenBus backend_watch notification,
> > +         * resulting in a call to xen_bus_cleanup().
> > +         */
> > +        xen_device_backend_printf(xendev, "state", "%u", state);
> 
> It kind of feels wrong to rely on xenstore to notify QEMU's xenbus
> driver that a device needs cleanup. But that does the job.
> 

I had originally considered setting up an event notifier but that seemed like more fds than were strictly necessary ;-)

> With a note about the use of QLIST_NEXT and the few typo fixed, the
> patch will be good to go.
> 

Cool. I'll tidy up and send a v3.

  Paul

> Thanks,
> 
> --
> Anthony PERARD

Re: [Qemu-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler

[Paul Durrant]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Paul Durrant
> Sent: 12 September 2019 16:16
> To: Anthony Perard <anthony.perard@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini <sstabellini@kernel.org>; qemu-devel@nongnu.org
> Subject: Re: [Xen-devel] [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler
> 
> > -----Original Message-----
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > Sent: 12 September 2019 16:04
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>
> > Subject: Re: [PATCH v2 3/3] xen: perform XenDevice clean-up in XenBus watch handler
> >
> > On Thu, Sep 12, 2019 at 01:16:46PM +0100, Paul Durrant wrote:
> > > Cleaning up offine XenDevice objects directly in
> >               ^ offline
> >
> > > xen_device_backend_changed() is dangerous as xen_device_unrealize() will
> > > modify the watch list that is being walked. Even the QLIST_FOREACH_SAFE()
> > > used in notifier_list_notify() is insufficient as *two* notifiers (for
> > > the frontend and backend watches) are removed, thus potentially rendering
> > > the 'next' pointer unsafe.
> > >
> > > The solution is to use the XenBus backend_watch handler to do the clean-up
> > > instead, as it is invoked whilst walking a separate watch list.
> > >
> > > This patch therefore adds a new 'offline_devices' list to XenBus, to which
> > > offline devices are added by xen_device_backend_changed(). The XenBus
> > > backend_watch registration is also changed to not only invoke
> > > xen_bus_enumerate() but also a new xen_bus_cleanup() function, which will
> > > walk 'offline_devices' and perform the necessary actions.
> > > For safety a an extra 'online' check is also added to
> >              ^ one 'a' too many?
> >
> > > xen_bus_type_enumerate() to make sure that no attempt is made to create a
> > > new XenDevice object for a backend that is offline.
> > >
> > > NOTE: This patch also include some cosmetic changes:
> > >       - substitute the local variable name 'backend_state'
> > >         in xen_bus_type_enumerate() with 'state', since there
> > >         is no ambiguity with any other state in that context.
> > >       - change xen_device_state_is_active() to
> > >         xen_device_frontend_is_active() (and pass a XenDevice directly)
> > >         since the state tests contained therein only apply to a frontend.
> > >       - use 'state' rather then 'xendev->backend_state' in
> > >         xen_device_backend_changed() to shorten the code.
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > ---
> > >
> > > v2:
> > >  - Make sure we don't try to add a XenDevice to 'offline_devices' more than
> > >    once
> > > ---
> > >
> > >      /*
> > >       * If a backend is still 'online' then we should leave it alone but,
> > > -     * if a backend is not 'online', then the device should be destroyed
> > > -     * once the state is Closed.
> > > +     * if a backend is not 'online', then the device is a candidate
> > > +     * for destruction. Hence add it to the 'offline' list to be cleaned
> > > +     * by xen_bus_cleanup().
> > >       */
> > > -    if (!xendev->backend_online &&
> > > -        (xendev->backend_state == XenbusStateClosed ||
> > > -         xendev->backend_state == XenbusStateInitialising ||
> > > -         xendev->backend_state == XenbusStateInitWait ||
> > > -         xendev->backend_state == XenbusStateUnknown)) {
> > > -        Error *local_err = NULL;
> > > +    if (!online &&
> > > +        (state == XenbusStateClosed ||  state == XenbusStateInitialising ||
> > > +         state == XenbusStateInitWait || state == XenbusStateUnknown) &&
> > > +        !QLIST_NEXT(xendev, list)) {
> >
> > Could you add a note about this QLIST_NEXT? I don't think it's going to
> > be obvious enough why we check that there are no next item. I might only
> > understand it just because of your reply to the v1 of the patch.
> > (Well the changelog of the patch also point out what it's for.)
> >
> 
> Sure, it's worth a comment.

Actually, on closer inspection !QLIST_NEXT() is an insufficient checked. I had assumed that QLISTs were full doubly-linked lists but they are not; the last element on a list will still have a NULL next pointer. It will be sufficient, and also clearer to the reader, if I add a boolean to XenDevice which is set when it is added to the list.

  Paul