From: Paul Durrant <Paul.Durrant@citrix.com>
To: Paul Durrant <Paul.Durrant@citrix.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Anthony Perard <anthony.perard@citrix.com>,
Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler
Date: Wed, 11 Sep 2019 16:15:34 +0000 [thread overview]
Message-ID: <91a39e7bc56c4916b956b0752e0aa9dd@AMSPEX02CL03.citrite.net> (raw)
In-Reply-To: <20190911143618.23477-4-paul.durrant@citrix.com>
> -----Original Message-----
> From: Paul Durrant <paul.durrant@citrix.com>
> Sent: 11 September 2019 15:36
> To: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Anthony
> Perard <anthony.perard@citrix.com>
> Subject: [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler
>
> 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>
> ---
> hw/xen/trace-events | 2 +
> hw/xen/xen-bus.c | 91 +++++++++++++++++++++++++++++-----------
> include/hw/xen/xen-bus.h | 2 +
> 3 files changed, 70 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..055beb7260 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,29 @@ 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)) {
> + 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);
I realize now that I should have a check here to make sure the above does not happen twice. I'll send a v2.
Paul
>
> - 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
prev parent reply other threads:[~2019-09-11 16:20 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-11 14:36 [Qemu-devel] [PATCH 0/3] xen: fix a potential crash in xen-bus Paul Durrant
2019-09-11 14:36 ` [Qemu-devel] [PATCH 1/3] xen / notify: introduce a new XenWatchList abstraction Paul Durrant
2019-09-12 10:16 ` Anthony PERARD
2019-09-12 11:40 ` Paul Durrant
2019-09-11 14:36 ` [Qemu-devel] [PATCH 2/3] xen: introduce separate XenWatchList for XenDevice objects Paul Durrant
2019-09-12 11:27 ` Anthony PERARD
2019-09-11 14:36 ` [Qemu-devel] [PATCH 3/3] xen: perform XenDevice clean-up in XenBus watch handler Paul Durrant
2019-09-11 16:15 ` Paul Durrant [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=91a39e7bc56c4916b956b0752e0aa9dd@AMSPEX02CL03.citrite.net \
--to=paul.durrant@citrix.com \
--cc=anthony.perard@citrix.com \
--cc=qemu-devel@nongnu.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).