| Message ID | 20190912121646.29148-4-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: fix a potential crash in xen-bus | expand |
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,
> -----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
> -----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
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 {
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(-)