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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9179 bytes --]

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

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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]