On 26.01.26 08:08, Jürgen Groß wrote:
On 17.11.25 12:06, Jürgen Groß wrote:On 02.11.25 04:20, Marek Marczykowski-Górecki wrote:When the backend domain crashes, coordinated device cleanup is not possible (as it involves waiting for the backend state change). In that case, toolstack forcefully removes frontend xenstore entries. xenbus_dev_changed() handles this case, and triggers device cleanup. It's possible that toolstack manages to connect new device in that place, before xenbus_dev_changed() notices the old one is missing. If that happens, new one won't be probed and will forever remain in XenbusStateInitialising.Fix this by checking backend-id and if it changes, consider it unplug+plug operation. It's important that cleanup on such unplug doesn't modify xenstore entries (especially the "state" key) as it belong to the new device to be probed - changing it would derail establishing connection to the new backend (most likely, closing the device before it was even connected). Handle this case by setting new xenbus_device->vanished flag to true, and check it before changing state entry. And even if xenbus_dev_changed() correctly detects the device was forcefully removed, the cleanup handling is still racy. Since this whole handling doesn't happend in a single xenstore transaction, it's possible that toolstack might put a new device there already. Avoid re-creating the state key (which in the case of loosing the race would actually close newly attached device). The problem does not apply to frontend domain crash, as this case involves coordinated cleanup. Problem originally reported at https://lore.kernel.org/xen-devel/aOZvivyZ9YhVWDLN@mail-itl/T/#t, including reproduction steps. Signed-off-by: Marek Marczykowski-Górecki <[email protected]>Sorry I didn't get earlier to this. My main problem with this patch is that it is basically just papering over a more general problem. You are just making the problem much more improbable, but not impossible to occur again. In case the new driver domain has the same domid as the old one you can still have the same race. The clean way to handle that would be to add a unique Id in Xenstore to each device on the backend side, which can be tested on the frontend side to match. In case it doesn't match, an old device with the same kind and devid can be cleaned up. The unique Id would obviously need to be set by the Xen tools inside the transaction writing the initial backend Xenstore nodes, as doing that from the backend would add another potential ambiguity by the driver domain choosing the same unique id as the previous one did. The question is whether something like your patch should be used as a fallback in case there is no unique Id on the backend side of the device due to a too old Xen version.I think I have found a solution which is much more simple, as it doesn't need any change of the protocol or any addition of new identifiers. When creating a new PV device, Xen tools will always write all generic frontend- and backend-nodes. This includes the frontend state, which is initialized as XenbusStateInitialising. The Linux kernel's xenbus driver is already storing the last known state of a xenbus device in struct xenbus_device. When changing the state, the xenbus driver is even reading the state from Xenstore (even if only for making sure the path is still existing). So all what is needed is to check, whether the read current state is matching the locally saved state. If it is not matching AND the read state is XenbusStateInitialising, you can be sure that the backend has been replaced. Handling this will need to check the return value of xenbus_switch_state() in all related drivers, but this is just a more or less mechanical change. I'll prepare a patch series for that.
In the end the result is more like your patch, avoiding the need to modify all drivers. I just added my idea to your patch and modified some of your code to be more simple. I _think_ I have covered all possible scenarios now, resulting in the need to keep the backend id check in case the backend died during the early init phase of the device. Could you please verify the attached patch is working for you? Juergen
From 3a7a8257613141edb03833c5d277b14ee28697dc Mon Sep 17 00:00:00 2001 From: Juergen Gross <[email protected]> Date: Mon, 26 Jan 2026 08:45:17 +0100 Subject: [PATCH] xen/xenbus: better handle backend crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the backend domain crashes, coordinated device cleanup is not possible (as it involves waiting for the backend state change). In that case, toolstack forcefully removes frontend xenstore entries. xenbus_dev_changed() handles this case, and triggers device cleanup. It's possible that toolstack manages to connect new device in that place, before xenbus_dev_changed() notices the old one is missing. If that happens, new one won't be probed and will forever remain in XenbusStateInitialising. Fix this by checking the frontend's state in Xenstore. In case it has been reset to XenbusStateInitialising by Xen tools, consider this being the result of an unplug+plug operation. It's important that cleanup on such unplug doesn't modify Xenstore entries (especially the "state" key) as it belong to the new device to be probed - changing it would derail establishing connection to the new backend (most likely, closing the device before it was even connected). Handle this case by setting new xenbus_device->vanished flag to true, and check it before changing state entry. And even if xenbus_dev_changed() correctly detects the device was forcefully removed, the cleanup handling is still racy. Since this whole handling doesn't happened in a single Xenstore transaction, it's possible that toolstack might put a new device there already. Avoid re-creating the state key (which in the case of loosing the race would actually close newly attached device). The problem does not apply to frontend domain crash, as this case involves coordinated cleanup. Problem originally reported at https://lore.kernel.org/xen-devel/aOZvivyZ9YhVWDLN@mail-itl/T/#t, including reproduction steps. Based-on-patch-by: Marek Marczykowski-Górecki <[email protected]> Signed-off-by: Juergen Gross <[email protected]> --- drivers/xen/xenbus/xenbus_client.c | 9 ++++++-- drivers/xen/xenbus/xenbus_probe.c | 34 ++++++++++++++++++++++++++++++ include/xen/xenbus.h | 1 + 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c index 2dc874fb5506..0d99a1f60df4 100644 --- a/drivers/xen/xenbus/xenbus_client.c +++ b/drivers/xen/xenbus/xenbus_client.c @@ -226,8 +226,9 @@ __xenbus_switch_state(struct xenbus_device *dev, struct xenbus_transaction xbt; int current_state; int err, abort; + bool vanished = false; - if (state == dev->state) + if (state == dev->state || dev->vanished) return 0; again: @@ -242,6 +243,10 @@ __xenbus_switch_state(struct xenbus_device *dev, err = xenbus_scanf(xbt, dev->nodename, "state", "%d", ¤t_state); if (err != 1) goto abort; + if (current_state != dev->state && current_state == XenbusStateInitialising) { + vanished = true; + goto abort; + } err = xenbus_printf(xbt, dev->nodename, "state", "%d", state); if (err) { @@ -256,7 +261,7 @@ __xenbus_switch_state(struct xenbus_device *dev, if (err == -EAGAIN && !abort) goto again; xenbus_switch_fatal(dev, depth, err, "ending transaction"); - } else + } else if (!vanished) dev->state = state; return 0; diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c index 86fe6e779056..fcd59a2109c3 100644 --- a/drivers/xen/xenbus/xenbus_probe.c +++ b/drivers/xen/xenbus/xenbus_probe.c @@ -444,6 +444,9 @@ static void xenbus_cleanup_devices(const char *path, struct bus_type *bus) info.dev = NULL; bus_for_each_dev(bus, NULL, &info, cleanup_dev); if (info.dev) { + dev_warn(&info.dev->dev, + "device forcefully removed from xenstore\n"); + info.dev->vanished = true; device_unregister(&info.dev->dev); put_device(&info.dev->dev); } @@ -659,6 +662,37 @@ void xenbus_dev_changed(const char *node, struct xen_bus_type *bus) return; dev = xenbus_device_find(root, &bus->bus); + /* + * Backend domain crash results in not coordinated frontend removal, + * without going through XenbusStateClosing. If this is a new instance + * of the same device Xen tools will have reset the state to + * XenbusStateInitializing. + * It might be that the backend crashed early during the init phase of + * device setup, in which case the known state would have been + * XenbusStateInitializing. So test the backend domid to match the + * saved one. In case the new backend happens to have the same domid as + * the old one, we can just carry on, as there is no inconsistency + * resulting in this case. + */ + if (dev && !strcmp(bus->root, "device")) { + enum xenbus_state state = xenbus_read_driver_state(dev->nodename); + unsigned int backend = xenbus_read_unsigned(root, "backend-id", + dev->otherend_id); + + if (state == XenbusStateInitialising && + (state != dev->state || backend != dev->otherend_id)) { + /* + * State has been reset, assume the old one vanished + * and new one needs to be probed. + */ + dev_warn(&dev->dev, + "state reset occurred, reconnecting\n"); + dev->vanished = true; + device_unregister(&dev->dev); + put_device(&dev->dev); + dev = NULL; + } + } if (!dev) xenbus_probe_node(bus, type, root); else diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h index c94caf852aea..064cb6db5e9f 100644 --- a/include/xen/xenbus.h +++ b/include/xen/xenbus.h @@ -80,6 +80,7 @@ struct xenbus_device { const char *devicetype; const char *nodename; const char *otherend; + bool vanished; int otherend_id; struct xenbus_watch otherend_watch; struct device dev; -- 2.52.0
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
