Re: [Xen-devel] [systemd-devel] [PATCH v1] core: mount xenfs, ignore proc-xen.mount (#6442, #6662)

[Lennart Poettering <lennart@xxxxxxxxxxxxxx>]

On Mi, 29.11.17 16:38, Olaf Hering (olaf@xxxxxxxxx) wrote:

> The detection of ConditionVirtualisation= relies on the presence of
> /proc/xen/capabilities. If the file exists and contains the string
> "control_d", the running system is a dom0 and VIRTUALIZATION_NONE should
> be set. In case /proc/xen exists, or some sysfs files indicate "xen",
> VIRTUALIZATION_XEN should be set to indicate the system is a domU.
> 
> With an (old) xenlinux based kernel, /proc/xen/capabilities is always
> available and the detection described above works always. But with a
> pvops based kernel, xenfs must be mounted on /proc/xen to get
> "capabilities". Up to now this was done by a proc-xen.mount unit, which
> is part of xen.git. Since the mounting happens "late", other units may
> be scheduled before "proc-xen.mount". If these other units make use of
> "ConditionVirtualisation=", the virtualization detection returns
> incorect results. detect_vm() will set VIRTUALIZATION_XEN because "xen"
> is found in sysfs. This value will be cached. Once xenfs is mounted,
> the next process that runs detect_vm() will get VIRTUALIZATION_NONE.
> 
> This misdetection can be fixed by turning xenfs into an "API
> filesystem", which is done by adding it to mount_table[]. That way xenfs
> will be mounted early, and detect_vm() returns correct results.
> 
> But since the "proc-xen.mount" unit still exists, the startup of that
> unit fails because it is rejected due to a conflict with an "API
> filesystem" mount. This is done in mount_verify(). The unit gets into
> state FAILED. Other Xen toolstack related .service files rely on the
> existance of the proc-xen.mount unit. If proc-xen.mount fails, the
> toolstack fails too.
> 
> To stay compatible with existing and upcoming releases of Xen, add
> "/proc/xen" to the list of ignored paths and adjust the check for API
> mounts.
> 
> Now "proc-xen.mount" is silently accepted, it gets into state "start
> condition failed" because "ConditionPathExists=!/proc/xen/capabilities
> was not met". But all units depending on "proc-xen.mount" start anyway.
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>

Could you please submit this through github? we much prefer doing
reviews through github these days.

Also S-o-b is a kernel thing, we don't do that on systemd.

> ---
> 
> I'm not 100% sure if the logic change in mount_verify() has undesired effects.
> 
>  src/core/mount-setup.c | 3 +++
>  src/core/mount.c       | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c
> index d5c0fcddc..df23e71b5 100644
> --- a/src/core/mount-setup.c
> +++ b/src/core/mount-setup.c
> @@ -114,6 +114,8 @@ static const MountPoint mount_table[] = {
>            cg_is_legacy_wanted, MNT_FATAL|MNT_IN_CONTAINER },
>          { "pstore",      "/sys/fs/pstore",            "pstore",     NULL,    
>                   MS_NOSUID|MS_NOEXEC|MS_NODEV,
>            NULL,          MNT_NONE                   },
> +        { "xenfs",       "/proc/xen",                 "xenfs",      NULL,    
>                   MS_NOSUID|MS_NOEXEC,
> +          NULL,          MNT_NONE                   },
>  #if ENABLE_EFI
>          { "efivarfs",    "/sys/firmware/efi/efivars", "efivarfs",   NULL,    
>                   MS_NOSUID|MS_NOEXEC|MS_NODEV,
>            is_efi_boot,   MNT_NONE                   },
> @@ -126,6 +128,7 @@ static const MountPoint mount_table[] = {
>  static const char ignore_paths[] =
>          /* SELinux file systems */
>          "/sys/fs/selinux\0"
> +        "/proc/xen\0"
>          /* Container bind mounts */
>          "/proc/sys\0"
>          "/dev/console\0"
> diff --git a/src/core/mount.c b/src/core/mount.c
> index b25bb9cb4..5afe7fcd7 100644
> --- a/src/core/mount.c
> +++ b/src/core/mount.c
> @@ -526,7 +526,7 @@ static int mount_verify(Mount *m) {
>                  return -EINVAL;
>          }
>  
> -        if (mount_point_is_api(m->where) || mount_point_ignore(m->where)) {
> +        if (mount_point_is_api(m->where) && !mount_point_ignore(m->where)) {
>                  log_unit_error(UNIT(m), "Cannot create mount unit for API 
> file system %s. Refusing.", m->where);
>                  return -EINVAL;
>          }
> _______________________________________________
> systemd-devel mailing list
> systemd-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/systemd-devel


Lennart

-- 
Lennart Poettering, Red Hat

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel