Re: [PATCH v1] tools/hotplug: systemd: Make dependency on Xen device nodes

["Erik Schilling" <erik.schilling@xxxxxxxxxx>]

On Fri Aug 25, 2023 at 5:36 AM CEST, Leo Yan wrote:
> When system booting up, the kernel module xen_gntdev.ko is loaded and
> the device node '/dev/xen/gntdev' is created; later the xenstored
> service in systemd launches daemon to open this device node.
>
> This flow has a race condition between creating the device node in the
> kernel module and using the device node in the systemd service.  It's
> possible that the xenstored service fails to open the device node due
> to the delay of creating the device node.  In the end, xenbus cannot be
> used between the Dom0 kernel and the Xen hypervisor.
>
> To resolve this issue, we need to synchronize between udev and systemd
> for the device node.  There have an extra change in the udev rules for
> tagging 'systemd' for Xen device nodes, which notifies device node
> creating to systemd; besides udev change, this patch adds dependency in
> systemd service for waiting the device node.
>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
>
>  The udev rules change is on github:
>  
> https://github.com/systemd/systemd/pull/28962/commits/520340dfea3b6cf9fe7a24c9238313b1a5fe8539

Let's see what they think, but I fear systemd may not be the correct
upstream to carry this... Skimming through the rules that they have
there, it mostly looks like they only carry rules that are relevant for
systemd-related services directly.

>  tools/hotplug/Linux/systemd/xenstored.service.in | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in 
> b/tools/hotplug/Linux/systemd/xenstored.service.in
> index 261077dc92..6e48cdb0e7 100644
> --- a/tools/hotplug/Linux/systemd/xenstored.service.in
> +++ b/tools/hotplug/Linux/systemd/xenstored.service.in
> @@ -1,7 +1,7 @@
>  [Unit]
>  Description=The Xen xenstore
> -Requires=proc-xen.mount
> -After=proc-xen.mount
> +Requires=proc-xen.mount dev-xen-gntdev.device
> +After=proc-xen.mount dev-xen-gntdev.device

I must admit that I am a bit confused why this is enough... During our
discussion on Slack, when you quoted from your rule it included
`ENV{SYSTEMD_WANTS}="xenstored.service"`. Did you drop that during later
development? Was there additional reasearch involved in dropping it?

My understanding was that if devices do not exist when systemd builds
its transaction model, dependencies on them will just get tossed out[1].
So I would have expected that there should still be a race if
the .device does not pull in the service.

Did you hit the race frequently enough to be sure that this fixes it in
entirety? Is there a place somewhere in the Xen or kernel code where one
could add an excessive sleep in order to trigger the race more reliably?

[1] "citation-needed": But I vaguely remember dealing with a similar
    issue and getting told this on #systemd IRC

- Erik