|
| 1 | +From c2b6a8e0c215ad574f49001994ab1ef661df57e1 Mon Sep 17 00:00:00 2001 |
| 2 | +From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= |
| 3 | + <marmarek@invisiblethingslab.com> |
| 4 | +Date: Thu, 9 Oct 2025 14:51:04 +0200 |
| 5 | +Subject: [PATCH] xen/xenbus: better handle backend crash |
| 6 | +MIME-Version: 1.0 |
| 7 | +Content-Type: text/plain; charset=UTF-8 |
| 8 | +Content-Transfer-Encoding: 8bit |
| 9 | + |
| 10 | +When the backend domain crashes, coordinated device cleanup is not |
| 11 | +possible (as it involves waiting for the backend state change). In that |
| 12 | +case, toolstack forcefully removes frontend xenstore entries. |
| 13 | +xenbus_dev_changed() handles this case, and triggers device cleanup. |
| 14 | +It's possible that toolstack manages to connect new device in that |
| 15 | +place, before xenbus_dev_changed() notices the old one is missing. If |
| 16 | +that happens, new one won't be probed and will forever remain in |
| 17 | +XenbusStateInitialising. |
| 18 | + |
| 19 | +Fix this by checking backend-id and if it changes, consider it |
| 20 | +unplug+plug operation. It's important that cleanup on such unplug |
| 21 | +doesn't modify xenstore entries (especially the "state" key) as it |
| 22 | +belong to the new device to be probed - changing it would derail |
| 23 | +establishing connection to the new backend (most likely, closing the |
| 24 | +device before it was even connected). Handle this case by setting new |
| 25 | +xenbus_device->vanished flag to true, and check it before changing state |
| 26 | +entry. |
| 27 | + |
| 28 | +And even if xenbus_dev_changed() correctly detects the device was |
| 29 | +forcefully removed, the cleanup handling is still racy. Since this whole |
| 30 | +handling doesn't happend in a single xenstore transaction, it's possible |
| 31 | +that toolstack might put a new device there already. Avoid re-creating |
| 32 | +the state key (which in the case of loosing the race would actually |
| 33 | +close newly attached device). |
| 34 | + |
| 35 | +The problem does not apply to frontend domain crash, as this case |
| 36 | +involves coordinated cleanup. |
| 37 | + |
| 38 | +Problem originally reported at |
| 39 | +https://lore.kernel.org/xen-devel/aOZvivyZ9YhVWDLN@mail-itl/T/#t, |
| 40 | +including reproduction steps. |
| 41 | + |
| 42 | +Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> |
| 43 | +--- |
| 44 | +I considered re-using one of existing fields instead of a new |
| 45 | +xenbus_device->vanished, but I wasn't sure if that would work better. |
| 46 | +Setting xenbus_device->nodename to NULL would prevent few other places |
| 47 | +using it (including some log messages). Setting xenbus_device->otherend |
| 48 | +might have less unintentional impact, but logically it doesn't feel |
| 49 | +correct. |
| 50 | +--- |
| 51 | + drivers/xen/xenbus/xenbus_client.c | 2 ++ |
| 52 | + drivers/xen/xenbus/xenbus_probe.c | 25 +++++++++++++++++++++++++ |
| 53 | + include/xen/xenbus.h | 1 + |
| 54 | + 3 files changed, 28 insertions(+) |
| 55 | + |
| 56 | +diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c |
| 57 | +index e73ec225d4a61..ce2f49d9aa4ad 100644 |
| 58 | +--- a/drivers/xen/xenbus/xenbus_client.c |
| 59 | ++++ b/drivers/xen/xenbus/xenbus_client.c |
| 60 | +@@ -275,6 +275,8 @@ __xenbus_switch_state(struct xenbus_device *dev, |
| 61 | + */ |
| 62 | + int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state) |
| 63 | + { |
| 64 | ++ if (dev->vanished) |
| 65 | ++ return 0; |
| 66 | + return __xenbus_switch_state(dev, state, 0); |
| 67 | + } |
| 68 | + |
| 69 | +diff --git a/drivers/xen/xenbus/xenbus_probe.c b/drivers/xen/xenbus/xenbus_probe.c |
| 70 | +index 86fe6e7790566..3c3e56b544976 100644 |
| 71 | +--- a/drivers/xen/xenbus/xenbus_probe.c |
| 72 | ++++ b/drivers/xen/xenbus/xenbus_probe.c |
| 73 | +@@ -444,6 +444,9 @@ static void xenbus_cleanup_devices(const char *path, struct bus_type *bus) |
| 74 | + info.dev = NULL; |
| 75 | + bus_for_each_dev(bus, NULL, &info, cleanup_dev); |
| 76 | + if (info.dev) { |
| 77 | ++ dev_warn(&info.dev->dev, |
| 78 | ++ "device forcefully removed from xenstore\n"); |
| 79 | ++ info.dev->vanished = true; |
| 80 | + device_unregister(&info.dev->dev); |
| 81 | + put_device(&info.dev->dev); |
| 82 | + } |
| 83 | +@@ -659,6 +662,28 @@ void xenbus_dev_changed(const char *node, struct xen_bus_type *bus) |
| 84 | + return; |
| 85 | + |
| 86 | + dev = xenbus_device_find(root, &bus->bus); |
| 87 | ++ /* Backend domain crash results in not coordinated frontend removal, |
| 88 | ++ * without going through XenbusStateClosing. Check if the device |
| 89 | ++ * wasn't replaced to point at another backend in the meantime. |
| 90 | ++ */ |
| 91 | ++ if (dev && !strncmp(node, "device/", sizeof("device/")-1)) { |
| 92 | ++ int backend_id; |
| 93 | ++ int err = xenbus_gather(XBT_NIL, root, |
| 94 | ++ "backend-id", "%i", &backend_id, |
| 95 | ++ NULL); |
| 96 | ++ if (!err && backend_id != dev->otherend_id) { |
| 97 | ++ /* It isn't the same device, assume the old one |
| 98 | ++ * vanished and new one needs to be probed. |
| 99 | ++ */ |
| 100 | ++ dev_warn(&dev->dev, |
| 101 | ++ "backend-id mismatch (%d != %d), reconnecting\n", |
| 102 | ++ backend_id, dev->otherend_id); |
| 103 | ++ dev->vanished = true; |
| 104 | ++ device_unregister(&dev->dev); |
| 105 | ++ put_device(&dev->dev); |
| 106 | ++ dev = NULL; |
| 107 | ++ } |
| 108 | ++ } |
| 109 | + if (!dev) |
| 110 | + xenbus_probe_node(bus, type, root); |
| 111 | + else |
| 112 | +diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h |
| 113 | +index 7dab04cf4a36c..43a5335f1d5a3 100644 |
| 114 | +--- a/include/xen/xenbus.h |
| 115 | ++++ b/include/xen/xenbus.h |
| 116 | +@@ -87,6 +87,7 @@ struct xenbus_device { |
| 117 | + struct completion down; |
| 118 | + struct work_struct work; |
| 119 | + struct semaphore reclaim_sem; |
| 120 | ++ bool vanished; |
| 121 | + |
| 122 | + /* Event channel based statistics and settings. */ |
| 123 | + atomic_t event_channels; |
| 124 | +-- |
| 125 | +2.51.0 |
| 126 | + |
0 commit comments