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

1 files 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: