Re: [PATCH] xen/xenbus: better handle backend crash

[Jürgen Groß <jgross@xxxxxxxx>]

On 06.02.26 17:57, Marek Marczykowski-Górecki wrote:
> On Thu, Jan 29, 2026 at 08:02:35AM +0100, Jürgen Groß wrote:
> > 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 <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > > > 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?
> Thanks for the patch!
>
> I ran it through relevant tests, and I got inconsistent results.
> Specifically, sometimes, the domU hangs (actually, just one vCPU spins
> inside xenwatch thread). Last console messages are:
>
>      systemd[626]: Starting dconf.service - User preferences database...
>      gnome-keyring-daemon[975]: ␛[0;1;39mcouldn't access control socket: 
> /run/user/1000/keyring/control: No such file or directory␛[0m
>      gnome-keyring-daemon[975]: ␛[0;1;38:5:185mdiscover_other_daemon: 0␛[0m
>      xen vif-0: xenbus: state reset occurred, reconnecting
>      gnome-keyring-daemon[974]: ␛[0;1;39mcouldn't access control socket: 
> /run/user/1000/keyring/control: No such file or directory␛[0m
>      gnome-keyring-daemon[976]: ␛[0;1;39mcouldn't access control socket: 
> /run/user/1000/keyring/control: No such file or directory␛[0m
>      gnome-keyring-daemon[976]: ␛[0;1;38:5:185mdiscover_other_daemon: 0␛[0m
>      gnome-keyring-daemon[974]: ␛[0;1;38:5:185mdiscover_other_daemon: 0␛[0m
>      xen vif-0: xenbus: state reset occurred, reconnecting
>      systemd[626]: Started dconf.service - User preferences database.
>      xen_netfront: Initialising Xen virtual ethernet driver
>      vif vif-0: xenbus: state reset occurred, reconnecting
>
> And the call trace of the spinning xenwatch thread is:
>
>      task:xenwatch        state:D stack:0     pid:64    tgid:64    ppid:2      
> task_flags:0x288040 flags:0x00080000
>      Call Trace:
>       <TASK>
>       __schedule+0x2f3/0x780
>       schedule+0x27/0x80
>       xs_wait_for_reply+0xab/0x1f0
>       ? __pfx_autoremove_wake_function+0x10/0x10
>       xs_talkv+0xec/0x200
>       xs_single+0x4a/0x70
>       xenbus_gather+0xe4/0x1a0
>       xenbus_read_driver_state+0x42/0x70
>       xennet_bus_close+0x113/0x2c0 [xen_netfront]
>       ? __pfx_autoremove_wake_function+0x10/0x10
>       xennet_remove+0x16/0x80 [xen_netfront]
>       xenbus_dev_remove+0x71/0xf0
>       device_release_driver_internal+0x19c/0x200
>       bus_remove_device+0xc6/0x130
>       device_del+0x160/0x3e0
>       device_unregister+0x17/0x60
>       xenbus_dev_changed.cold+0x5e/0x6b
>       ? __pfx_xenwatch_thread+0x10/0x10
>       xenwatch_thread+0x92/0x1c0
>       ? __pfx_autoremove_wake_function+0x10/0x10
>       kthread+0xfc/0x240
>       ? __pfx_kthread+0x10/0x10
>       ret_from_fork+0xf5/0x110
>       ? __pfx_kthread+0x10/0x10
>       ret_from_fork_asm+0x1a/0x30
>       </TASK>
>      task:xenbus          state:S stack:0     pid:63    tgid:63    ppid:2      
> task_flags:0x208040 flags:0x00080000
>      Call Trace:
>       <TASK>
>       __schedule+0x2f3/0x780
>       ? __pfx_xenbus_thread+0x10/0x10
>       schedule+0x27/0x80
>       xenbus_thread+0x1a8/0x200
>       ? __pfx_autoremove_wake_function+0x10/0x10
>       kthread+0xfc/0x240
>       ? __pfx_kthread+0x10/0x10
>       ret_from_fork+0xf5/0x110
>       ? __pfx_kthread+0x10/0x10
>       ret_from_fork_asm+0x1a/0x30
>       </TASK>
>
> (technically, `top` says it's the xenbus thread spinning, but it looks
> like the actual issue is in xenwatch one)
>
> Note that other xenwatch actions in this domU are not executed, for
> example `xl sysrq` does nothing. Not surprising, given xenwatch thread
> is busy... Fortunately, it blocks only a single vCPU, so I'm able to
> interact with the domU over console (to get the above traces).
>
> It isn't a reliable failure, in this test run it failed once, out of 4
> related tests.
>
> The specific test is: 
> https://github.com/QubesOS/qubes-core-admin/blob/main/qubes/tests/integ/network.py#L234
> In short:
> 1. Start a domU
> 2. Pause it
> 3. Attach network (backend is != dom0)
> 4. Unpause
>
> TBH, I'm not sure why the "state reset occurred" message is triggered at
> all, I think it shouldn't be in this case...
Thanks for the test.

I guess the hang happens due to xennet_bus_close() waiting for a state
change which won't happen at all, as it is already XenbusStateInitialising.

The right thing to do would be to add the xenbus_device pointer to the
parameters of xenbus_read_driver_state(), in order to be able to return
XenbusStateUnknown in case the device has vanished.

I'll add a patch for that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature