Add a check reporting if any CPU vulnerabilities have not been mitigated
by the kernel. It further reports whether it is safe to use Intel SMT
for KVM guests or not, as several of the vulnerabilities are dangerous
when combined with SMT and KVM, even if mitigations are in effect.

eg on a host with mitigations, but unsafe SMT still enabled:

  Checking CPU hardware vulnerability mitigation...PASS
  Checking CPU hardware vulnerability SMT safety...FAIL

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 libvirt.spec.in                               |   1 +
 tools/Makefile.am                             |   1 +
 .../rules/linux-cpu-hardware-flaws.yaml       | 165 ++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml

diff --git a/libvirt.spec.in b/libvirt.spec.in
index f336296a08..8aa226798a 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1901,6 +1901,7 @@ exit 0
 %{_datadir}/libvirt/host-validate/linux-acpi.yaml
 %{_datadir}/libvirt/host-validate/linux-cgroups.yaml
 %{_datadir}/libvirt/host-validate/linux-cpu.yaml
+%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml
 %{_datadir}/libvirt/host-validate/linux-devices.yaml
 %{_datadir}/libvirt/host-validate/linux-iommu.yaml
 %{_datadir}/libvirt/host-validate/linux-namespaces.yaml
diff --git a/tools/Makefile.am b/tools/Makefile.am
index 728de475a2..907b0195c2 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \
 	$(srcdir)/host-validate/rules/linux-acpi.yaml \
 	$(srcdir)/host-validate/rules/linux-cgroups.yaml \
 	$(srcdir)/host-validate/rules/linux-cpu.yaml \
+	$(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \
 	$(srcdir)/host-validate/rules/linux-devices.yaml \
 	$(srcdir)/host-validate/rules/linux-iommu.yaml \
 	$(srcdir)/host-validate/rules/linux-namespaces.yaml \
diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
new file mode 100644
index 0000000000..6a243df96d
--- /dev/null
+++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
@@ -0,0 +1,165 @@
+#
+# Define facts related to CPU hardware vulnerabilities
+#
+
+facts:
+- name: cpu.vulnerability.meltdown
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/meltdown
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: (\w+)
+          match: 1
+- name: cpu.vulnerability.spectre_v1
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/spectre_v1
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: (\w+)
+          match: 1
+- name: cpu.vulnerability.spectre_v2
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/spectre_v2
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: (\w+)
+          match: 1
+- name: cpu.vulnerability.spec_store_bypass
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: (\w+)
+          match: 1
+- name: cpu.vulnerability.mds
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/mds
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: (\w+)
+          match: 1
+- name: cpu.vulnerability.mds_smt
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/mds
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: SMT (\w+)
+          match: 1
+- name: cpu.vulnerability.l1tf
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/l1tf
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: (\w+)
+          match: 1
+- name: cpu.vulnerability.l1tf_smt
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  value:
+    file:
+      path: /sys/devices/system/cpu/vulnerabilities/l1tf
+      ignoreMissing: true
+      parse:
+        scalar:
+          regex: SMT (\w+)
+          match: 1
+- name: cpu.vulnerability.unsafe
+  filter:
+    fact:
+      name: os.kernel
+      value: Linux
+  report:
+    message: CPU hardware vulnerability mitigation
+    pass: false
+  value:
+    bool:
+      any:
+        expressions:
+        - fact:
+            name: cpu.vulnerability.meltdown
+            value: Vulnerable
+        - fact:
+            name: cpu.vulnerability.spectre_v1
+            value: Vulnerable
+        - fact:
+            name: cpu.vulnerability.spectre_v2
+            value: Vulnerable
+        - fact:
+            name: cpu.vulnerability.spec_store_bypass
+            value: Vulnerable
+        - fact:
+            name: cpu.vulnerability.mds
+            value: Vulnerable
+        - fact:
+            name: cpu.vulnerability.l1tf
+            value: Vulnerable
+- name: cpu.vulnerability.unsafe_smt
+  filter:
+    all:
+      expressions:
+      - fact:
+          name: os.kernel
+          value: Linux
+      - fact:
+          name: cpu.vendor.intel
+          value: "true"
+      - fact:
+          name: cpu.virt.present
+          value: "true"
+  report:
+    message: CPU hardware vulnerability SMT safety
+    pass: false
+  value:
+    bool:
+      any:
+        expressions:
+        - fact:
+            name: cpu.vulnerability.mds_smt
+            value: vulnerable
+        - fact:
+            name: cpu.vulnerability.l1tf_smt
+            value: vulnerable
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Sep 27, 2019 at 01:52:25PM +0100, Daniel P. Berrangé wrote:
>Add a check reporting if any CPU vulnerabilities have not been mitigated
>by the kernel. It further reports whether it is safe to use Intel SMT
>for KVM guests or not, as several of the vulnerabilities are dangerous
>when combined with SMT and KVM, even if mitigations are in effect.
>
>eg on a host with mitigations, but unsafe SMT still enabled:
>
>  Checking CPU hardware vulnerability mitigation...PASS
>  Checking CPU hardware vulnerability SMT safety...FAIL
>
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> libvirt.spec.in                               |   1 +
> tools/Makefile.am                             |   1 +
> .../rules/linux-cpu-hardware-flaws.yaml       | 165 ++++++++++++++++++
> 3 files changed, 167 insertions(+)
> create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
>
>diff --git a/libvirt.spec.in b/libvirt.spec.in
>index f336296a08..8aa226798a 100644
>--- a/libvirt.spec.in
>+++ b/libvirt.spec.in
>@@ -1901,6 +1901,7 @@ exit 0
> %{_datadir}/libvirt/host-validate/linux-acpi.yaml
> %{_datadir}/libvirt/host-validate/linux-cgroups.yaml
> %{_datadir}/libvirt/host-validate/linux-cpu.yaml
>+%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml
> %{_datadir}/libvirt/host-validate/linux-devices.yaml
> %{_datadir}/libvirt/host-validate/linux-iommu.yaml
> %{_datadir}/libvirt/host-validate/linux-namespaces.yaml
>diff --git a/tools/Makefile.am b/tools/Makefile.am
>index 728de475a2..907b0195c2 100644
>--- a/tools/Makefile.am
>+++ b/tools/Makefile.am
>@@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \
> 	$(srcdir)/host-validate/rules/linux-acpi.yaml \
> 	$(srcdir)/host-validate/rules/linux-cgroups.yaml \
> 	$(srcdir)/host-validate/rules/linux-cpu.yaml \
>+	$(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \
> 	$(srcdir)/host-validate/rules/linux-devices.yaml \
> 	$(srcdir)/host-validate/rules/linux-iommu.yaml \
> 	$(srcdir)/host-validate/rules/linux-namespaces.yaml \
>diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
>new file mode 100644
>index 0000000000..6a243df96d
>--- /dev/null
>+++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
>@@ -0,0 +1,165 @@
>+#
>+# Define facts related to CPU hardware vulnerabilities
>+#
>+
>+facts:
>+- name: cpu.vulnerability.meltdown
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/meltdown
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: (\w+)
>+          match: 1
>+- name: cpu.vulnerability.spectre_v1
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/spectre_v1
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: (\w+)
>+          match: 1
>+- name: cpu.vulnerability.spectre_v2
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/spectre_v2
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: (\w+)
>+          match: 1
>+- name: cpu.vulnerability.spec_store_bypass
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: (\w+)
>+          match: 1
>+- name: cpu.vulnerability.mds
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/mds
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: (\w+)
>+          match: 1
>+- name: cpu.vulnerability.mds_smt
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/mds
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: SMT (\w+)
>+          match: 1
>+- name: cpu.vulnerability.l1tf
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/l1tf
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: (\w+)
>+          match: 1
>+- name: cpu.vulnerability.l1tf_smt
>+  filter:
>+    fact:
>+      name: os.kernel
>+      value: Linux
>+  value:
>+    file:
>+      path: /sys/devices/system/cpu/vulnerabilities/l1tf
>+      ignoreMissing: true
>+      parse:
>+        scalar:
>+          regex: SMT (\w+)
>+          match: 1

Given the fact that most of these could just be virFileReadValueUint() it does
not even make it easier to read or write the code.

Every time someone will want to add a new check or a fact they will need to find
a similar one, copy-paste it, change it and hope for the best.  This introduces
yet another "language" on top of the two you are adding already.  I really do
not see any benefit in this.

If I was to pick a new feature we could benefit from, I would much rather prefer
having an opt-in for report-home of HW features and usage for some very rough
anonymous statistics.

Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
> On Fri, Sep 27, 2019 at 01:52:25PM +0100, Daniel P. Berrangé wrote:
> > Add a check reporting if any CPU vulnerabilities have not been mitigated
> > by the kernel. It further reports whether it is safe to use Intel SMT
> > for KVM guests or not, as several of the vulnerabilities are dangerous
> > when combined with SMT and KVM, even if mitigations are in effect.
> > 
> > eg on a host with mitigations, but unsafe SMT still enabled:
> > 
> >  Checking CPU hardware vulnerability mitigation...PASS
> >  Checking CPU hardware vulnerability SMT safety...FAIL
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > libvirt.spec.in                               |   1 +
> > tools/Makefile.am                             |   1 +
> > .../rules/linux-cpu-hardware-flaws.yaml       | 165 ++++++++++++++++++
> > 3 files changed, 167 insertions(+)
> > create mode 100644 tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
> > 
> > diff --git a/libvirt.spec.in b/libvirt.spec.in
> > index f336296a08..8aa226798a 100644
> > --- a/libvirt.spec.in
> > +++ b/libvirt.spec.in
> > @@ -1901,6 +1901,7 @@ exit 0
> > %{_datadir}/libvirt/host-validate/linux-acpi.yaml
> > %{_datadir}/libvirt/host-validate/linux-cgroups.yaml
> > %{_datadir}/libvirt/host-validate/linux-cpu.yaml
> > +%{_datadir}/libvirt/host-validate/linux-cpu-hardware-flaws.yaml
> > %{_datadir}/libvirt/host-validate/linux-devices.yaml
> > %{_datadir}/libvirt/host-validate/linux-iommu.yaml
> > %{_datadir}/libvirt/host-validate/linux-namespaces.yaml
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 728de475a2..907b0195c2 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -173,6 +173,7 @@ virt_host_validate_rules_DATA = \
> > 	$(srcdir)/host-validate/rules/linux-acpi.yaml \
> > 	$(srcdir)/host-validate/rules/linux-cgroups.yaml \
> > 	$(srcdir)/host-validate/rules/linux-cpu.yaml \
> > +	$(srcdir)/host-validate/rules/linux-cpu-hardware-flaws.yaml \
> > 	$(srcdir)/host-validate/rules/linux-devices.yaml \
> > 	$(srcdir)/host-validate/rules/linux-iommu.yaml \
> > 	$(srcdir)/host-validate/rules/linux-namespaces.yaml \
> > diff --git a/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
> > new file mode 100644
> > index 0000000000..6a243df96d
> > --- /dev/null
> > +++ b/tools/host-validate/rules/linux-cpu-hardware-flaws.yaml
> > @@ -0,0 +1,165 @@
> > +#
> > +# Define facts related to CPU hardware vulnerabilities
> > +#
> > +
> > +facts:
> > +- name: cpu.vulnerability.meltdown
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/meltdown
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.spectre_v1
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/spectre_v1
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.spectre_v2
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/spectre_v2
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.spec_store_bypass
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/spec_store_bypass
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.mds
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/mds
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.mds_smt
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/mds
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: SMT (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.l1tf
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/l1tf
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: (\w+)
> > +          match: 1
> > +- name: cpu.vulnerability.l1tf_smt
> > +  filter:
> > +    fact:
> > +      name: os.kernel
> > +      value: Linux
> > +  value:
> > +    file:
> > +      path: /sys/devices/system/cpu/vulnerabilities/l1tf
> > +      ignoreMissing: true
> > +      parse:
> > +        scalar:
> > +          regex: SMT (\w+)
> > +          match: 1
> 
> Given the fact that most of these could just be virFileReadValueUint() it does
> not even make it easier to read or write the code.

Errr, virFileReadValueUint reads a single integer from a file.

This is extracting a single *word* from a specific field in a
file.

Doing this in code would involve doing a regex match the same as
here.

> Every time someone will want to add a new check or a fact they will need to find
> a similar one, copy-paste it, change it and hope for the best.  This introduces
> yet another "language" on top of the two you are adding already.  I really do
> not see any benefit in this.

Taking this particular example.

  - name: cpu.vulnerability.l1tf_smt
    filter:
      fact:
        name: os.kernel
        value: Linux
    value:
      file:
        path: /sys/devices/system/cpu/vulnerabilities/l1tf
        ignoreMissing: true
        parse:
          scalar:
            regex: SMT (\w+)
            match: 1

It could be approximately equiv to code

        if runtime.GOOS == "Linux" {
	    return "", nil
	}

        data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf")
	if err != nil {
	    if  os.IsNotExist(err) {
	        return "", nil
	    }
	    return "", err
	}

	match, err := regexp.Find(`SMT (\w+)`, data)
	if err != nil {
 	    return "", err
	}
	err = SetFact("cpu.vulnerability.l1tf_smt", match)
	if err != nil {
	    return "", err
	}

The code does look more familiar, but it is certainly not simpler.
Most notabley there are alot of error handling cases here that need
to be dealt with, even when written in Go. Doing this in C with its
string handling support involves even more error handling. This
is a significant maintainence burden that is not well handled in
a consistent manner across the code.

This is a fairly simple check example, case where we want to extract
data from a more more complex file formats result in much more code
complexity.

In the pure code approach, we have mixed up code related to reading
files, running commands, parsing data formats, setting facts, printing
messages, skipping execution when certain other conditions applied.
Hidden in this is the information about what feature we are actually
checking for.

A data driven approach to this problem means that we get clear separation
between distinct parts/jobs.

The engine written in code concerns itself with generic facilities for
reading files, running commands, parsing data, and all the error handling
and data processing.

Thereafter the interesting checks is simply a declarative expression of
what data we wish to extract for each fature, and declarative expression
of dependancies between features.

This engine code has greater complexity than what exists today for sure,
but the key thing is that this is mostly a one time upfront code, and is
the least interesting part of the tool. What is most interest is the set
of features being checked for and the relationships between the features.

Having the declarative part separated from the functional part is where
the value arises. 

 - Changes to the functional code apply consistently across all
   the checks made. For example, an improvement to error handling
   to regex processing was done once and applied to all the checks
   that used regex. Or consistently handling when reading files
   that are potentially missing on systems.

 - New checks can be introduced in a more reliable and consistent
   manner. These CPU vulnerability checks are an example of this
   in practice. We merely have to express what files we're interested
   in and how to match the relevant data in them. No need to worry
   about reading files, compiling & running regexs, making sure the
   error handling was done in the same way as other existing checks.

 - The declarative data can be processed algorithmically. For example
   given the depedancies expressed between the facts, I saw that it
   was not neccessary to define all the facts in the same data file.
   It was possible split them across many files, load those files in
   any order, and algorithmically determine exactly which subset of
   checks need to be executed & in what order.

 - Having separated data from the code it is obviously possible to
   extend this without introducing any code changes. This is possible
   to use from outside libvirt. For example there are usage scenarios
   which may not be supported by certain versions of QEMU. The QEMU
   package can drop in some facts which will be picked up by the
   virt-host-validate tool. Alternatively going up the stack, an
   app like OpenStack which uses libvirt may wish to restrict what
   they use, or wish to check extra virt host features interesting
   to their app's usage of libvirt. Again their package can now
   drop in extra facts.

 - New features can be built ontop of the declarative data. At first
   I just set out to replicate the existing tool's output as is. Once
   that was done, it was obvious that we could trivially add a new
   feature allowing us to print the raw informationm about each
   fact that was checked, as an alternative to the human targetted
   message strings. IOW we got a machine readable output format
   essentially for free by virtue of using declarative data.

 - The implementation can be rewritten again at will. If the original
   C code had this split of data vs processing logic, this conversion
   of langauge would have been even more compelling, as none of othe
   declarative data would have needed changing. I'm not expecting
   another rewrite, but this capability was already valuable, between
   the v1 and v2 posting of this patch I changed from XML to YAML for
   the data format. This conversion was entirely automated, because
   it could be algorithmically processed.

These are all typical & expected benefits that arise from separating
functional logic from declarative data. In the long term this is a
clear win, & as I mentioned I would have done this sooner had it not
been for our use of C. 

> If I was to pick a new feature we could benefit from, I would much rather prefer
> having an opt-in for report-home of HW features and usage for some very rough
> anonymous statistics.

I don't think we want to get involved in reporting user data back to any
server, as it is a data privacy minefield. This tool can be used by other
existing reporting tools though - eg Red Hat distros often include the
'sosreport' tool that bundles stuff up for reporting customer tickets.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, 2019-09-30 at 15:30 +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
> > Given the fact that most of these could just be virFileReadValueUint() it does
> > not even make it easier to read or write the code.
> 
> Errr, virFileReadValueUint reads a single integer from a file.
> 
> This is extracting a single *word* from a specific field in a
> file.
> 
> Doing this in code would involve doing a regex match the same as
> here.
> 
> > Every time someone will want to add a new check or a fact they will need to find
> > a similar one, copy-paste it, change it and hope for the best.  This introduces
> > yet another "language" on top of the two you are adding already.  I really do
> > not see any benefit in this.
> 
> Taking this particular example.
> 
>   - name: cpu.vulnerability.l1tf_smt
>     filter:
>       fact:
>         name: os.kernel
>         value: Linux
>     value:
>       file:
>         path: /sys/devices/system/cpu/vulnerabilities/l1tf
>         ignoreMissing: true
>         parse:
>           scalar:
>             regex: SMT (\w+)
>             match: 1
> 
> It could be approximately equiv to code
> 
>         if runtime.GOOS == "Linux" {
> 	    return "", nil
> 	}
> 
>         data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf")
> 	if err != nil {
> 	    if  os.IsNotExist(err) {
> 	        return "", nil
> 	    }
> 	    return "", err
> 	}
> 
> 	match, err := regexp.Find(`SMT (\w+)`, data)
> 	if err != nil {
>  	    return "", err
> 	}
> 	err = SetFact("cpu.vulnerability.l1tf_smt", match)
> 	if err != nil {
> 	    return "", err
> 	}
> 
> The code does look more familiar, but it is certainly not simpler.
> Most notabley there are alot of error handling cases here that need
> to be dealt with, even when written in Go. Doing this in C with its
> string handling support involves even more error handling. This
> is a significant maintainence burden that is not well handled in
> a consistent manner across the code.
> 
> This is a fairly simple check example, case where we want to extract
> data from a more more complex file formats result in much more code
> complexity.
> 
> In the pure code approach, we have mixed up code related to reading
> files, running commands, parsing data formats, setting facts, printing
> messages, skipping execution when certain other conditions applied.
> Hidden in this is the information about what feature we are actually
> checking for.
> 
> A data driven approach to this problem means that we get clear separation
> between distinct parts/jobs.
> 
> The engine written in code concerns itself with generic facilities for
> reading files, running commands, parsing data, and all the error handling
> and data processing.
> 
> Thereafter the interesting checks is simply a declarative expression of
> what data we wish to extract for each fature, and declarative expression
> of dependancies between features.
> 
> This engine code has greater complexity than what exists today for sure,
> but the key thing is that this is mostly a one time upfront code, and is
> the least interesting part of the tool. What is most interest is the set
> of features being checked for and the relationships between the features.
> 
> Having the declarative part separated from the functional part is where
> the value arises. 
> 
>  - Changes to the functional code apply consistently across all
>    the checks made. For example, an improvement to error handling
>    to regex processing was done once and applied to all the checks
>    that used regex. Or consistently handling when reading files
>    that are potentially missing on systems.
> 
>  - New checks can be introduced in a more reliable and consistent
>    manner. These CPU vulnerability checks are an example of this
>    in practice. We merely have to express what files we're interested
>    in and how to match the relevant data in them. No need to worry
>    about reading files, compiling & running regexs, making sure the
>    error handling was done in the same way as other existing checks.

All of the above mostly sounds like "use functions" to me. Something
like

  value, err := GetIntFactFromFile(
      "/sys/devices/system/cpu/vulnerabilities/l1tf",
      "SMT (\w+)",
      OsLinux,
      IgnoreMissing,
  )
  if err != nil {
      return err
  }
  SetIntFact("cpu.vulnerability.l1tf_smt", value)

requires basically as much upfront knowledge to understand as the
YAML version, but doesn't introduce a new DSL. And hey, it's shorter
than either version! :)

>  - The declarative data can be processed algorithmically. For example
>    given the depedancies expressed between the facts, I saw that it
>    was not neccessary to define all the facts in the same data file.
>    It was possible split them across many files, load those files in
>    any order, and algorithmically determine exactly which subset of
>    checks need to be executed & in what order.
> 
>  - Having separated data from the code it is obviously possible to
>    extend this without introducing any code changes. This is possible
>    to use from outside libvirt. For example there are usage scenarios
>    which may not be supported by certain versions of QEMU. The QEMU
>    package can drop in some facts which will be picked up by the
>    virt-host-validate tool. Alternatively going up the stack, an
>    app like OpenStack which uses libvirt may wish to restrict what
>    they use, or wish to check extra virt host features interesting
>    to their app's usage of libvirt. Again their package can now
>    drop in extra facts.

This last point is the only advantage I can see very clearly. I'm
not sure it's worth introducing a specific format for, though.

>  - New features can be built ontop of the declarative data. At first
>    I just set out to replicate the existing tool's output as is. Once
>    that was done, it was obvious that we could trivially add a new
>    feature allowing us to print the raw informationm about each
>    fact that was checked, as an alternative to the human targetted
>    message strings. IOW we got a machine readable output format
>    essentially for free by virtue of using declarative data.

This is a benefit of storing the information as facts, which as I
said before I think is a good idea; it doesn't mean we have to
obtain said facts by processing YAML files.

>  - The implementation can be rewritten again at will. If the original
>    C code had this split of data vs processing logic, this conversion
>    of langauge would have been even more compelling, as none of othe
>    declarative data would have needed changing. I'm not expecting
>    another rewrite, but this capability was already valuable, between
>    the v1 and v2 posting of this patch I changed from XML to YAML for
>    the data format. This conversion was entirely automated, because
>    it could be algorithmically processed.

If you had used code instead of a pure data format as the source for
information, then you wouldn't have needed to perform that conversion
in the first place O:-)

> These are all typical & expected benefits that arise from separating
> functional logic from declarative data. In the long term this is a
> clear win, & as I mentioned I would have done this sooner had it not
> been for our use of C.

I expect that if you would have posted patches introducing this
machinery to the C version of virt-host-validate you would have seen
more or less the same reactions.

> > If I was to pick a new feature we could benefit from, I would much rather prefer
> > having an opt-in for report-home of HW features and usage for some very rough
> > anonymous statistics.
> 
> I don't think we want to get involved in reporting user data back to any
> server, as it is a data privacy minefield. This tool can be used by other
> existing reporting tools though - eg Red Hat distros often include the
> 'sosreport' tool that bundles stuff up for reporting customer tickets.

Wholeheartedly agreed.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 30, 2019 at 05:10:10PM +0200, Andrea Bolognani wrote:
> On Mon, 2019-09-30 at 15:30 +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 30, 2019 at 10:55:00AM +0200, Martin Kletzander wrote:
> > > Given the fact that most of these could just be virFileReadValueUint() it does
> > > not even make it easier to read or write the code.
> > 
> > Errr, virFileReadValueUint reads a single integer from a file.
> > 
> > This is extracting a single *word* from a specific field in a
> > file.
> > 
> > Doing this in code would involve doing a regex match the same as
> > here.
> > 
> > > Every time someone will want to add a new check or a fact they will need to find
> > > a similar one, copy-paste it, change it and hope for the best.  This introduces
> > > yet another "language" on top of the two you are adding already.  I really do
> > > not see any benefit in this.
> > 
> > Taking this particular example.
> > 
> >   - name: cpu.vulnerability.l1tf_smt
> >     filter:
> >       fact:
> >         name: os.kernel
> >         value: Linux
> >     value:
> >       file:
> >         path: /sys/devices/system/cpu/vulnerabilities/l1tf
> >         ignoreMissing: true
> >         parse:
> >           scalar:
> >             regex: SMT (\w+)
> >             match: 1
> > 
> > It could be approximately equiv to code
> > 
> >         if runtime.GOOS == "Linux" {
> > 	    return "", nil
> > 	}
> > 
> >         data, err = ioutils.ReadFile("/sys/devices/system/cpu/vulnerabilities/l1tf")
> > 	if err != nil {
> > 	    if  os.IsNotExist(err) {
> > 	        return "", nil
> > 	    }
> > 	    return "", err
> > 	}
> > 
> > 	match, err := regexp.Find(`SMT (\w+)`, data)
> > 	if err != nil {
> >  	    return "", err
> > 	}
> > 	err = SetFact("cpu.vulnerability.l1tf_smt", match)
> > 	if err != nil {
> > 	    return "", err
> > 	}
> > 
> > The code does look more familiar, but it is certainly not simpler.
> > Most notabley there are alot of error handling cases here that need
> > to be dealt with, even when written in Go. Doing this in C with its
> > string handling support involves even more error handling. This
> > is a significant maintainence burden that is not well handled in
> > a consistent manner across the code.
> > 
> > This is a fairly simple check example, case where we want to extract
> > data from a more more complex file formats result in much more code
> > complexity.
> > 
> > In the pure code approach, we have mixed up code related to reading
> > files, running commands, parsing data formats, setting facts, printing
> > messages, skipping execution when certain other conditions applied.
> > Hidden in this is the information about what feature we are actually
> > checking for.
> > 
> > A data driven approach to this problem means that we get clear separation
> > between distinct parts/jobs.
> > 
> > The engine written in code concerns itself with generic facilities for
> > reading files, running commands, parsing data, and all the error handling
> > and data processing.
> > 
> > Thereafter the interesting checks is simply a declarative expression of
> > what data we wish to extract for each fature, and declarative expression
> > of dependancies between features.
> > 
> > This engine code has greater complexity than what exists today for sure,
> > but the key thing is that this is mostly a one time upfront code, and is
> > the least interesting part of the tool. What is most interest is the set
> > of features being checked for and the relationships between the features.
> > 
> > Having the declarative part separated from the functional part is where
> > the value arises. 
> > 
> >  - Changes to the functional code apply consistently across all
> >    the checks made. For example, an improvement to error handling
> >    to regex processing was done once and applied to all the checks
> >    that used regex. Or consistently handling when reading files
> >    that are potentially missing on systems.
> > 
> >  - New checks can be introduced in a more reliable and consistent
> >    manner. These CPU vulnerability checks are an example of this
> >    in practice. We merely have to express what files we're interested
> >    in and how to match the relevant data in them. No need to worry
> >    about reading files, compiling & running regexs, making sure the
> >    error handling was done in the same way as other existing checks.
> 
> All of the above mostly sounds like "use functions" to me. Something
> like
> 
>   value, err := GetIntFactFromFile(
>       "/sys/devices/system/cpu/vulnerabilities/l1tf",
>       "SMT (\w+)",
>       OsLinux,
>       IgnoreMissing,
>   )
>   if err != nil {
>       return err
>   }
>   SetIntFact("cpu.vulnerability.l1tf_smt", value)
> 
> requires basically as much upfront knowledge to understand as the
> YAML version, but doesn't introduce a new DSL. And hey, it's shorter
> than either version! :)

"use functions" is another way of saying separate the declarative
data from the functional logic. This series has indeed taken this
approach and has such functions in the data processing engine.

They look more complex than what you've illustrated here, because
they cope with many more scenario. Those set of args you've shown
and just one combination of many possibilities. 

So instead of passing the arguments inline like this, you'll
want to define a struct to hold the arguments & just pass a
struct to the function. This is such a good idea this impl
already defines such structs to hold the data.

Once you've got this nice separate of data and code, loading
the data from a file is just one tiny step more to make a more
flexible tool allowing drop in extensions. 

> >  - The declarative data can be processed algorithmically. For example
> >    given the depedancies expressed between the facts, I saw that it
> >    was not neccessary to define all the facts in the same data file.
> >    It was possible split them across many files, load those files in
> >    any order, and algorithmically determine exactly which subset of
> >    checks need to be executed & in what order.
> > 
> >  - Having separated data from the code it is obviously possible to
> >    extend this without introducing any code changes. This is possible
> >    to use from outside libvirt. For example there are usage scenarios
> >    which may not be supported by certain versions of QEMU. The QEMU
> >    package can drop in some facts which will be picked up by the
> >    virt-host-validate tool. Alternatively going up the stack, an
> >    app like OpenStack which uses libvirt may wish to restrict what
> >    they use, or wish to check extra virt host features interesting
> >    to their app's usage of libvirt. Again their package can now
> >    drop in extra facts.
> 
> This last point is the only advantage I can see very clearly. I'm
> not sure it's worth introducing a specific format for, though.
> 
> >  - New features can be built ontop of the declarative data. At first
> >    I just set out to replicate the existing tool's output as is. Once
> >    that was done, it was obvious that we could trivially add a new
> >    feature allowing us to print the raw informationm about each
> >    fact that was checked, as an alternative to the human targetted
> >    message strings. IOW we got a machine readable output format
> >    essentially for free by virtue of using declarative data.
> 
> This is a benefit of storing the information as facts, which as I
> said before I think is a good idea; it doesn't mean we have to
> obtain said facts by processing YAML files.

Of course you could define everything via a set of structs in the
code, but its crazy to do that as you've now hardcoded everything
at build time, pointlessly throwing away the key extensibility
benefit of having it defined via a data file.

> >  - The implementation can be rewritten again at will. If the original
> >    C code had this split of data vs processing logic, this conversion
> >    of langauge would have been even more compelling, as none of othe
> >    declarative data would have needed changing. I'm not expecting
> >    another rewrite, but this capability was already valuable, between
> >    the v1 and v2 posting of this patch I changed from XML to YAML for
> >    the data format. This conversion was entirely automated, because
> >    it could be algorithmically processed.
> 
> If you had used code instead of a pure data format as the source for
> information, then you wouldn't have needed to perform that conversion
> in the first place O:-)



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Sep 30, 2019 at 04:30:18PM +0100, Daniel P. Berrangé wrote:
>On Mon, Sep 30, 2019 at 05:10:10PM +0200, Andrea Bolognani wrote:
>> On Mon, 2019-09-30 at 15:30 +0100, Daniel P. Berrangé wrote:
>> >  - Having separated data from the code it is obviously possible to
>> >    extend this without introducing any code changes. This is possible
>> >    to use from outside libvirt. For example there are usage scenarios
>> >    which may not be supported by certain versions of QEMU. The QEMU
>> >    package can drop in some facts which will be picked up by the
>> >    virt-host-validate tool. Alternatively going up the stack, an
>> >    app like OpenStack which uses libvirt may wish to restrict what
>> >    they use, or wish to check extra virt host features interesting
>> >    to their app's usage of libvirt. Again their package can now
>> >    drop in extra facts.
>>
>> This last point is the only advantage I can see very clearly. I'm
>> not sure it's worth introducing a specific format for, though.
>>

Yes, this one I haven't thought of and it makes sense.  It also makes way more
sense for it not to be a part of libvirt, but rather a separate project as if
the support for loading other files from elsewhere is added, there is no need
for this to be a part of libvirt any more.  Which would immediately overrule
(almost) all issues I have seen with this.

>> >  - New features can be built ontop of the declarative data. At first
>> >    I just set out to replicate the existing tool's output as is. Once
>> >    that was done, it was obvious that we could trivially add a new
>> >    feature allowing us to print the raw informationm about each
>> >    fact that was checked, as an alternative to the human targetted
>> >    message strings. IOW we got a machine readable output format
>> >    essentially for free by virtue of using declarative data.
>>
>> This is a benefit of storing the information as facts, which as I
>> said before I think is a good idea; it doesn't mean we have to
>> obtain said facts by processing YAML files.
>
>Of course you could define everything via a set of structs in the
>code, but its crazy to do that as you've now hardcoded everything
>at build time, pointlessly throwing away the key extensibility
>benefit of having it defined via a data file.
>

Except when you are using extensive conditionals or building the fact from
multiple pieces information using, for example, boolean expressions.  At that
point you are creating a programming language on top of markup language.

>> >  - The implementation can be rewritten again at will. If the original
>> >    C code had this split of data vs processing logic, this conversion
>> >    of langauge would have been even more compelling, as none of othe
>> >    declarative data would have needed changing. I'm not expecting
>> >    another rewrite, but this capability was already valuable, between
>> >    the v1 and v2 posting of this patch I changed from XML to YAML for
>> >    the data format. This conversion was entirely automated, because
>> >    it could be algorithmically processed.
>>
>> If you had used code instead of a pure data format as the source for
>> information, then you wouldn't have needed to perform that conversion
>> in the first place O:-)
>
>
>
>Regards,
>Daniel
>-- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2019-10-01 at 16:44 +0200, Martin Kletzander wrote:
> On Mon, Sep 30, 2019 at 04:30:18PM +0100, Daniel P. Berrangé wrote:
> > Of course you could define everything via a set of structs in the
> > code, but its crazy to do that as you've now hardcoded everything
> > at build time, pointlessly throwing away the key extensibility
> > benefit of having it defined via a data file.
> 
> Except when you are using extensive conditionals or building the fact from
> multiple pieces information using, for example, boolean expressions.  At that
> point you are creating a programming language on top of markup language.

I agree.

It just occurred to me that this is pretty much exactly what Ansible
does. It makes sense for them to do so, because the primary use case
is for people to define their own rules, so the people writing YAML
will far outnumber those responsible of the underlying Python engine.
I don't think this would be the case for virt-host-validate.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list