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

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 8644 bytes --]

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@invisiblethingslab.com>
> > > 
> > > 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...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]