Skip to content

Commit

Permalink
xen: perform XenDevice clean-up in XenBus watch handler
Browse files Browse the repository at this point in the history 
Cleaning up offline 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 'inactive_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 'inactive_devices' and perform the necessary actions.
For safety 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 includes 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>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Message-Id: <20190913082159.31338-4-paul.durrant@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
  • Loading branch information
@anthonyper-ctx
Paul Durrant authored and anthonyper-ctx committed Sep 24, 2019
1 parent d198b71 commit 3809f75
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 25 deletions.
2 changes: 2 additions & 0 deletions hw/xen/trace-events
View file
Expand Up @@ -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"
Expand Down
94 changes: 69 additions & 25 deletions hw/xen/xen-bus.c
View file
Expand Up @@ -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,
Expand All @@ -365,9 +370,8 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const char *type)
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;

Expand All @@ -385,6 +389,45 @@ 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->inactive_devices, list, next) {
g_assert(xendev->inactive);
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);
Expand Down Expand Up @@ -433,7 +476,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,
Expand Down Expand Up @@ -555,9 +598,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:
Expand Down Expand Up @@ -594,30 +637,31 @@ 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 'inactive' 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) &&
!xendev->inactive) {
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));

if (!xen_backend_try_device_destroy(xendev, &local_err)) {
object_unparent(OBJECT(xendev));
}
xendev->inactive = true;
QLIST_INSERT_HEAD(&xenbus->inactive_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);
}
}

Expand Down
3 changes: 3 additions & 0 deletions include/hw/xen/xen-bus.h
View file
Expand Up @@ -32,7 +32,9 @@ typedef struct XenDevice {
XenWatch *backend_online_watch;
xengnttab_handle *xgth;
bool feature_grant_copy;
bool inactive;
QLIST_HEAD(, XenEventChannel) event_channels;
QLIST_ENTRY(XenDevice) list;
} XenDevice;

typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
Expand Down Expand Up @@ -68,6 +70,7 @@ typedef struct XenBus {
struct xs_handle *xsh;
XenWatchList *watch_list;
XenWatch *backend_watch;
QLIST_HEAD(, XenDevice) inactive_devices;
} XenBus;

typedef struct XenBusClass {
Expand Down

0 comments on commit 3809f75

Please sign in to comment.