Date: Tue, 7 Jun 2022 10:30:01 GMT From: =?utf-8?Q?Roger Pau Monn=C3=A9?= <royger@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: f3d54ded282c - main - xenbus: improve device tracking Message-ID: <202206071030.257AU1C4096859@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by royger: URL: https://cgit.FreeBSD.org/src/commit/?id=f3d54ded282ce992d168052d2c428e9df2a41e28 commit f3d54ded282ce992d168052d2c428e9df2a41e28 Author: Roger Pau Monné <royger@FreeBSD.org> AuthorDate: 2022-03-21 11:47:20 +0000 Commit: Roger Pau Monné <royger@FreeBSD.org> CommitDate: 2022-06-07 10:29:53 +0000 xenbus: improve device tracking xenbus needs to keep track of the devices exposed on xenstore, so that it can trigger frontend and backend device creation. Removal of backend devices is currently detected by checking the existence of the device (backend) xenstore directory, but that's prone to races as the device driver would usually add entries to such directory itself, so under certain circumstances it's possible for a driver to add node to the directory after the toolstack has removed it. This leads to devices not removed, which can eventually exhaust the memory of FreeBSD. Fix this by checking for the existence of the 'state' node instead of the directory, as such node will always be present when a device is active, and will be removed by the toolstack when the device is shut down. In order to avoid any races with the updating of the 'state' node by FreeBSD and the toolstack removing it use a transaction in xenbusb_write_ivar() for that purpose. Reported by: Ze Dupsys <zedupsys@gmail.com> Sponsored by: Citrix Systems R&D --- sys/xen/xenbus/xenbusb.c | 67 +++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/sys/xen/xenbus/xenbusb.c b/sys/xen/xenbus/xenbusb.c index e026f8203ea1..bd6ff552c9a6 100644 --- a/sys/xen/xenbus/xenbusb.c +++ b/sys/xen/xenbus/xenbusb.c @@ -254,7 +254,7 @@ xenbusb_delete_child(device_t dev, device_t child) static void xenbusb_verify_device(device_t dev, device_t child) { - if (xs_exists(XST_NIL, xenbus_get_node(child), "") == 0) { + if (xs_exists(XST_NIL, xenbus_get_node(child), "state") == 0) { /* * Device tree has been removed from Xenbus. * Tear down the device. @@ -907,6 +907,7 @@ xenbusb_write_ivar(device_t dev, device_t child, int index, uintptr_t value) case XENBUS_IVAR_STATE: { int error; + struct xs_transaction xst; newstate = (enum xenbus_state)value; sx_xlock(&ivars->xd_lock); @@ -915,31 +916,39 @@ xenbusb_write_ivar(device_t dev, device_t child, int index, uintptr_t value) goto out; } - error = xs_scanf(XST_NIL, ivars->xd_node, "state", - NULL, "%d", &currstate); - if (error) - goto out; - do { - error = xs_printf(XST_NIL, ivars->xd_node, "state", - "%d", newstate); - } while (error == EAGAIN); - if (error) { - /* - * Avoid looping through xenbus_dev_fatal() - * which calls xenbus_write_ivar to set the - * state to closing. - */ - if (newstate != XenbusStateClosing) - xenbus_dev_fatal(dev, error, - "writing new state"); - goto out; - } + error = xs_transaction_start(&xst); + if (error != 0) + goto out; + + do { + error = xs_scanf(xst, ivars->xd_node, "state", + NULL, "%d", &currstate); + } while (error == EAGAIN); + if (error) + goto out; + + do { + error = xs_printf(xst, ivars->xd_node, "state", + "%d", newstate); + } while (error == EAGAIN); + if (error) { + /* + * Avoid looping through xenbus_dev_fatal() + * which calls xenbus_write_ivar to set the + * state to closing. + */ + if (newstate != XenbusStateClosing) + xenbus_dev_fatal(dev, error, + "writing new state"); + goto out; + } + } while (xs_transaction_end(xst, 0)); ivars->xd_state = newstate; - if ((ivars->xd_flags & XDF_CONNECTING) != 0 - && (newstate == XenbusStateClosed - || newstate == XenbusStateConnected)) { + if ((ivars->xd_flags & XDF_CONNECTING) != 0 && + (newstate == XenbusStateClosed || + newstate == XenbusStateConnected)) { struct xenbusb_softc *xbs; ivars->xd_flags &= ~XDF_CONNECTING; @@ -949,8 +958,18 @@ xenbusb_write_ivar(device_t dev, device_t child, int index, uintptr_t value) wakeup(&ivars->xd_state); out: + if (error != 0) + xs_transaction_end(xst, 1); sx_xunlock(&ivars->xd_lock); - return (error); + /* + * Shallow ENOENT errors, as returning an error here will + * trigger a panic. ENOENT is fine to ignore, because it means + * the toolstack has removed the state node as part of + * destroying the device, and so we have to shut down the + * device without recreating it or else the node would be + * leaked. + */ + return (error == ENOENT ? 0 : error); } case XENBUS_IVAR_NODE:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202206071030.257AU1C4096859>