Bug 77271 - systemd-detect-virt and directive ConditionVirtualization wrongly return vm for xen dom0
Summary: systemd-detect-virt and directive ConditionVirtualization wrongly return vm f...
Status: RESOLVED FIXED
Alias: None
Product: systemd
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: medium normal
Assignee: Thomas Blume
QA Contact: systemd-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-10 15:30 UTC by Thomas Blume
Modified: 2014-07-17 12:27 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
avoid detecting xen dom0 as virtual machine (1.38 KB, text/plain)
2014-04-10 15:30 UTC, Thomas Blume
Details
avoid detecting xen dom0 as virtual machine if possible (1.58 KB, patch)
2014-04-23 14:50 UTC, Thomas Blume
Details | Splinter Review
updated patch (2.71 KB, text/plain)
2014-06-05 07:55 UTC, Thomas Blume
Details
new updated patch (2.63 KB, patch)
2014-06-06 08:40 UTC, Thomas Blume
Details | Splinter Review
patch update 3 (2.46 KB, patch)
2014-06-10 06:48 UTC, Thomas Blume
Details | Splinter Review
fix 0004-systemd-detect-virt-only-discover-Xen-domU.patch (1.06 KB, patch)
2014-07-17 09:37 UTC, Thomas Blume
Details | Splinter Review

Description Thomas Blume 2014-04-10 15:30:36 UTC
Created attachment 97166 [details]
avoid detecting xen dom0 as virtual machine

The current vm detection for xen was introduced in Bug 61491.
This implementation lacks the distinction between Xen dom0 and Xen domU.
Both, dom0 and domU are running inside the hypervisor.
Therefore systemd-detect-virt and the ConditionVirtualization directive detect dom0 as a virtual machine.

However, dom0 is not using virtual devices but is accessing the real hardware.
Therefore dom0 should be considered the virtualisation host and not a virtual machine.

Problems from this arise for example when using ConditionVirtualization in a services file for smartd. It won't check the harddisks on dom0 because of the vm detection.

Attached patch solves this.
Comment 1 Zbigniew Jedrzejewski-Szmek 2014-04-21 02:01:22 UTC
Andy,
you proposed the original change. Would this one work fine for your configuration?
Comment 2 Andy Melnikov 2014-04-21 07:49:29 UTC
On my Xen 3.4.1 PV DomU /proc/xen/capabilities doesn't exist. /proc/xen exists but it's empty.

On another Xen 3.4.2 DomU box (which was HVM one day but now it's either PV or PVHVM and I cannot tell which) /proc/xen/capabilities exists but it's empty.
Comment 3 Thomas Blume 2014-04-22 07:09:04 UTC
Hm, I've found that using /proc/xen/capabilities is widely used to identifiy dom0 versus domU.
If this is not general enough, there are other possibilities listet at:

http://wiki.xenproject.org/wiki/Xen_FAQ_Design_and_in_Depth

below:

"How can I tell if my OS is running on a physical machine or a virtual machine ?"

which one would work for you?
Comment 4 Andy Melnikov 2014-04-22 07:42:40 UTC
That page seems to be wrong/outdated/incomplete: dmidecode won't work on Xen PV systems as there's no DMI emulation there, /proc/xen/capabilities doesn't always exist, and kernel suffix is unreliable as one can put anything there.

I think we should leave old Xen detection code using /sys/hypervisor/type and only add a distinction between xen dom0 and domU using the method proposed in the new patch. In pseudocode:

if (/sys/hypervisor/type == 'xen')
{
   # we run Xen, and only need to distinguish between dom0 and domU
   if (exists /proc/xen/capabilities && 
         /proc/xen/capabilities contains 'control_d')
   {
      return 'xen-dom0'     
   }
   else
   {
      return 'xen-domU'
   }
}
Comment 5 Thomas Blume 2014-04-22 07:49:55 UTC
Ok, agreed.
Will you provide the patch?
Comment 6 Andy Melnikov 2014-04-22 08:27:16 UTC
I don't have build environment set up. I'm an end user, not a C developer. It should be much easier for you to amend your patch.
Comment 7 Thomas Blume 2014-04-23 14:50:27 UTC
Created attachment 97822 [details] [review]
avoid detecting xen dom0 as virtual machine if possible

Updated patch
Comment 8 Thomas Blume 2014-04-23 14:59:25 UTC
Ok, attached the new patch, tested as good as I could.
In domU:

# cat /proc/xen/capabilities
# systemd-detect-virt 
xen-domU
# echo $?
0

# mkdir /tmp/void
# mount -o bind /tmp/void/ /proc/xen/
# cat /proc/xen/capabilities
cat: /proc/xen/capabilities: No such file or directory
# systemd-detect-virt 
xen
# echo $?
0


in dom0:

# cat /proc/xen/capabilities
control_d
# systemd-detect-virt 
xen-dom0
# echo $?
1

# mkdir /tmp/void
# mount -o bind /tmp/void /proc/xen/
# cat /proc/xen/capabilities
cat: /proc/xen/capabilities: No such file or directory
# systemd-detect-virt 
xen
# echo $?
0
Comment 9 Andy Melnikov 2014-04-23 19:23:44 UTC
My case where /proc/xen/capabilities doesn't exist is xen-domU too. 

Unless someone has dom0 with missing /proc/xen/capabilities I think it's better not to create confusion and name both cases "xen-domU" instead of  "xen-domU" and "xen" as proposed by the updated patch.
Comment 10 Thomas Blume 2014-04-24 07:50:06 UTC
Hm, should we really return xen-domU when we don't know wheter it is domU or dom0?
I did the distinction by purpose to signal that it cannot be determined wheter it is domU or dom0.
However, it is only a string and if it fits better for cases where /proc/xen/capabilities doesn't exit, we could surely return xen-domU.
We just need to be aware that this output will be wrong in a dom0.
Comment 11 Andy Melnikov 2014-04-24 10:59:05 UTC
I agree that if we are not sure we shouldn't output "xen-dom0". 

My hypothesis was that /proc/xen/capabilities always exists in dom0. If the hypothesis holds then we do know that it's "xen-domU" and there's no need for separate "xen".

If there's a chance for /proc/xen/capabilities not to exist in dom0 then we can leave your patch as is.
Comment 12 Thomas Blume 2014-04-24 13:54:57 UTC
In SUSE kernels /proc/xen/capabilities is created in privcmd.c (see https://gitorious.org/opensuse/kernel/source/3f3ae894fe6c750ed2185cfdcc30a20d4becca88:drivers/xen/privcmd/privcmd.c).

But it seems that other kernel flavours don't have this.
(See for example: http://lists.xen.org/archives/html/xen-devel/2012-09/txtV0mkHOtzj_.txt)
In older kernels it was not created automatically, but had to be mounted via xenfs. (See: http://old-list-archives.xenproject.org/xen-users/2009-07/msg00490.html). Not sure wheter some flavours still use this.

So, I guess we cannot really rely on the fact that /proc/xen/capabilities is always present in dom0.
Comment 13 Andy Melnikov 2014-04-24 19:07:08 UTC
ok, then the patch looks good to me.
Comment 14 Lennart Poettering 2014-05-24 07:05:30 UTC
Please provide a git-format-patch formatted patch.

Also, unless > 0 is returned from the call you shouldn't set *id.

Please keep variable declarations local to the block you need them in.

Checking for the feature with a simple strstr() sounds wrong, you need to check for separators before/behind it too.

I see no reason to distuingish "xen" from "xen-domU". We probably shouldn't consider xen-dom0 virtualization at all, and everything else should just be "xen".
Comment 15 Thomas Blume 2014-06-05 07:55:15 UTC
Created attachment 100447 [details]
updated patch

Here is the updated patch.
Is it ok like that?
Comment 16 Lennart Poettering 2014-06-05 19:27:54 UTC
(In reply to comment #15)
> Created attachment 100447 [details]
> updated patch
> 
> Here is the updated patch.
> Is it ok like that?

Please follow the usual coding style: no redundant {} in single-line if blocks. Only spaces, no tabs.

The strsep loop looks needlessly complex. Why not just use "while ((cap = strsep(...)) { .."?

Otherwise looks OK.
Comment 17 Thomas Blume 2014-06-06 08:40:41 UTC
Created attachment 100513 [details] [review]
new updated patch

Thanks for the hints.
Here is the new updated patch.
Comment 18 Andy Melnikov 2014-06-06 11:10:04 UTC
Comment on attachment 100513 [details] [review]
new updated patch

Review of attachment 100513 [details] [review]:
-----------------------------------------------------------------

::: src/shared/virt.c
@@ +170,2 @@
>          if (r >= 0) {
> +                while (cap = strsep(&domcap, ",")) {

Shouldn't char * cap = NULL be moved here inside if?

@@ +170,3 @@
>          if (r >= 0) {
> +                while (cap = strsep(&domcap, ",")) {
> +                        if (streq(cap, "control_d"))

Can this be moved above into while condition?

while (cap = strsep(&domcap, ",") && !streq(cap, "control_d")) {
Comment 19 Lennart Poettering 2014-06-06 11:58:14 UTC
(In reply to comment #17)
> Created attachment 100513 [details] [review] [review]
> new updated patch
> 
> Thanks for the hints.
> Here is the new updated patch.

The loop actually skips over every second word now... You want to invoke strsep() only once in each iteration...
Comment 20 Thomas Blume 2014-06-10 06:48:24 UTC
Created attachment 100789 [details] [review]
patch update 3

Thanks for the hints.
Here is the updated patch.
Comment 21 Lennart Poettering 2014-06-10 16:18:45 UTC
To cut this short I merged this now, fixed the memory leak and whitespace problems, simplified things a bit. Can't test this though, as I don't have any Xen machines around. Please check whether I didn't break antyhing. Thanks.
Comment 22 Thomas Blume 2014-07-17 09:37:46 UTC
Created attachment 102974 [details] [review]
fix 0004-systemd-detect-virt-only-discover-Xen-domU.patch

The conditional for detection xen virtualization contained a little mistake.
It is checking for i to be empty: 'if (!i)  {', but it must check for cap instead,
because: 'cap = strsep(&i, ",")' will set cap to the discovered value and i to
the next value after the separator.
Hence, i would be empty, if there is only control_d in domcap, leading to a wrong
domU detection.
Comment 23 Zbigniew Jedrzejewski-Szmek 2014-07-17 12:27:12 UTC
Applied in http://cgit.freedesktop.org/systemd/systemd/commit/?id=a71516dfd1.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.