[PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>

marcandre.lureau@redhat.com posted 5 patches 1 year, 4 months ago
There is a newer version of this series
[PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by marcandre.lureau@redhat.com 1 year, 4 months ago
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Learn to parse a file path for the TPM state.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 docs/formatdomain.rst                       | 19 ++++++++++++++
 src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
 src/conf/domain_conf.h                      |  9 +++++++
 src/conf/schemas/domaincommon.rng           | 14 +++++++++++
 tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
 5 files changed, 71 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 4336cff3ac..992bb98730 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
    The default version used depends on the combination of hypervisor, guest
    architecture, TPM model and backend.
 
+``source``
+   The ``source`` element specifies the location of the TPM state storage . This
+   element only works with the ``emulator`` backend.
+
+   If not specified, the storage configuration is left to libvirt discretion.
+
+   This element requires that swtpm v0.7 or later is installed.
+
+   The following attributes are supported:
+
+   ``type``
+      The type of storage. It's possible to provide "file" to utilize a single
+      file or block device where the TPM state will be stored.
+
+   ``path``
+      The path to the TPM state storage.
+
+   :since:`Since v10.8.0`
+
 ``persistent_state``
    The ``persistent_state`` attribute indicates whether 'swtpm' TPM state is
    kept or not when a transient domain is powered off or undefined. This
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 284a3815b3..9dd8b6b55d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1322,6 +1322,12 @@ VIR_ENUM_IMPL(virDomainTPMVersion,
               "2.0",
 );
 
+VIR_ENUM_IMPL(virDomainTPMSourceType,
+              VIR_DOMAIN_TPM_SOURCE_TYPE_LAST,
+              "default",
+              "file",
+);
+
 VIR_ENUM_IMPL(virDomainTPMPcrBank,
               VIR_DOMAIN_TPM_PCR_BANK_LAST,
               "sha1",
@@ -10784,6 +10790,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
     int nbackends;
     int nnodes;
     size_t i;
+    xmlNodePtr source_node = NULL;
     g_autofree char *path = NULL;
     g_autofree char *secretuuid = NULL;
     g_autofree char *persistent_state = NULL;
@@ -10857,6 +10864,22 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
             def->data.emulator.hassecretuuid = true;
         }
 
+        source_node = virXPathNode("./backend/source", ctxt);
+        if (source_node) {
+            if (virXMLPropEnum(source_node, "type",
+                               virDomainTPMSourceTypeTypeFromString,
+                               VIR_XML_PROP_NONZERO,
+                               &def->data.emulator.source_type) < 0)
+                goto error;
+            path = virXMLPropString(source_node, "path");
+            if (!path) {
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("missing TPM source path"));
+                goto error;
+            }
+            def->data.emulator.source_path = g_steal_pointer(&path);
+        }
+
         persistent_state = virXMLPropString(backends[0], "persistent_state");
         if (persistent_state) {
             if (virStringParseYesNo(persistent_state,
@@ -25070,6 +25093,11 @@ virDomainTPMDefFormat(virBuffer *buf,
 
             virXMLFormatElement(&backendChildBuf, "active_pcr_banks", NULL, &activePcrBanksBuf);
         }
+        if (def->data.emulator.source_type != VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT) {
+            virBufferAsprintf(&backendChildBuf, "<source type='%s'",
+                              virDomainTPMSourceTypeTypeToString(def->data.emulator.source_type));
+            virBufferEscapeString(&backendChildBuf, " path='%s'/>\n", def->data.emulator.source_path);
+        }
         break;
     case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
         if (def->data.external.source->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6b27322e3e..7a70f68177 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1463,6 +1463,13 @@ typedef enum {
     VIR_DOMAIN_TPM_PCR_BANK_LAST
 } virDomainPcrBank;
 
+typedef enum {
+    VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT = 0,
+    VIR_DOMAIN_TPM_SOURCE_TYPE_FILE,
+
+    VIR_DOMAIN_TPM_SOURCE_TYPE_LAST
+} virDomainTPMSourceType;
+
 #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"
 
 struct _virDomainTPMDef {
@@ -1478,6 +1485,7 @@ struct _virDomainTPMDef {
         struct {
             virDomainTPMVersion version;
             virDomainChrSourceDef *source;
+            virDomainTPMSourceType source_type;
             char *source_path;
             char *logfile;
             unsigned int debug;
@@ -4277,6 +4285,7 @@ VIR_ENUM_DECL(virDomainRNGBackend);
 VIR_ENUM_DECL(virDomainTPMModel);
 VIR_ENUM_DECL(virDomainTPMBackend);
 VIR_ENUM_DECL(virDomainTPMVersion);
+VIR_ENUM_DECL(virDomainTPMSourceType);
 VIR_ENUM_DECL(virDomainTPMPcrBank);
 VIR_ENUM_DECL(virDomainMemoryModel);
 VIR_ENUM_DECL(virDomainMemoryBackingModel);
diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
index efb5f00d77..72c8b6c694 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -5923,6 +5923,7 @@
           <interleave>
             <ref name="tpm-backend-emulator-encryption"/>
             <ref name="tpm-backend-emulator-active-pcr-banks"/>
+            <ref name="tpm-backend-emulator-source"/>
           </interleave>
           <optional>
             <attribute name="persistent_state">
@@ -5981,6 +5982,19 @@
     </optional>
   </define>
 
+  <define name="tpm-backend-emulator-source">
+    <optional>
+      <element name="source">
+        <attribute name="type">
+          <value>file</value>
+        </attribute>
+        <attribute name="path">
+          <ref name="filePath"/>
+        </attribute>
+      </element>
+    </optional>
+  </define>
+
   <define name="tpm-backend-emulator-encryption">
     <optional>
       <element name="encryption">
diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
index 8a613db456..3d6300f544 100644
--- a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
+++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
@@ -34,6 +34,7 @@
           <sha256/>
           <sha512/>
         </active_pcr_banks>
+        <source type='file' path='/path/to/state'/>
       </backend>
     </tpm>
     <audio id='1' type='none'/>
-- 
2.45.2.827.g557ae147e6
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Stefan Berger 1 year, 4 months ago

On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Learn to parse a file path for the TPM state.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   docs/formatdomain.rst                       | 19 ++++++++++++++
>   src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
>   src/conf/domain_conf.h                      |  9 +++++++
>   src/conf/schemas/domaincommon.rng           | 14 +++++++++++
>   tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
>   5 files changed, 71 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 4336cff3ac..992bb98730 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
>      The default version used depends on the combination of hypervisor, guest
>      architecture, TPM model and backend.
>   
> +``source``
> +   The ``source`` element specifies the location of the TPM state storage . This
> +   element only works with the ``emulator`` backend.
> +
> +   If not specified, the storage configuration is left to libvirt discretion.
> +
> +   This element requires that swtpm v0.7 or later is installed.
> +
> +   The following attributes are supported:
> +
> +   ``type``
> +      The type of storage. It's possible to provide "file" to utilize a single
> +      file or block device where the TPM state will be stored.
> +
> +   ``path``
> +      The path to the TPM state storage.

The file backend of swtpm does not do the locking similar to what the 
dir backend does because those who added the file backend didn't 
need/want it. If we now give full control to the path of the TPM state 
file to the user via the domain XML then whose fault is it if two VMs 
use the same path to a file backend and stomp on the TPM state file? Is 
it the fault of the user because of how he defined the path in the XMLs?


> +
> +   :since:`Since v10.8.0`
> +

>   ``persistent_state``
>      The ``persistent_state`` attribute indicates whether 'swtpm' TPM state is
>      kept or not when a transient domain is powered off or undefined. This
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 284a3815b3..9dd8b6b55d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1322,6 +1322,12 @@ VIR_ENUM_IMPL(virDomainTPMVersion,
>                 "2.0",
>   );
>   
> +VIR_ENUM_IMPL(virDomainTPMSourceType,
> +              VIR_DOMAIN_TPM_SOURCE_TYPE_LAST,
> +              "default",
> +              "file",
> +);
> +
>   VIR_ENUM_IMPL(virDomainTPMPcrBank,
>                 VIR_DOMAIN_TPM_PCR_BANK_LAST,
>                 "sha1",
> @@ -10784,6 +10790,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
>       int nbackends;
>       int nnodes;
>       size_t i;
> +    xmlNodePtr source_node = NULL;
>       g_autofree char *path = NULL;
>       g_autofree char *secretuuid = NULL;
>       g_autofree char *persistent_state = NULL;
> @@ -10857,6 +10864,22 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
>               def->data.emulator.hassecretuuid = true;
>           }
>   
> +        source_node = virXPathNode("./backend/source", ctxt);
> +        if (source_node) {
> +            if (virXMLPropEnum(source_node, "type",
> +                               virDomainTPMSourceTypeTypeFromString,
> +                               VIR_XML_PROP_NONZERO,
> +                               &def->data.emulator.source_type) < 0)
> +                goto error;
> +            path = virXMLPropString(source_node, "path");
> +            if (!path) {
> +                virReportError(VIR_ERR_XML_ERROR, "%s",
> +                               _("missing TPM source path"));
> +                goto error;
> +            }
> +            def->data.emulator.source_path = g_steal_pointer(&path);
> +        }
> +
>           persistent_state = virXMLPropString(backends[0], "persistent_state");
>           if (persistent_state) {
>               if (virStringParseYesNo(persistent_state,
> @@ -25070,6 +25093,11 @@ virDomainTPMDefFormat(virBuffer *buf,
>   
>               virXMLFormatElement(&backendChildBuf, "active_pcr_banks", NULL, &activePcrBanksBuf);
>           }
> +        if (def->data.emulator.source_type != VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT) {
> +            virBufferAsprintf(&backendChildBuf, "<source type='%s'",
> +                              virDomainTPMSourceTypeTypeToString(def->data.emulator.source_type));
> +            virBufferEscapeString(&backendChildBuf, " path='%s'/>\n", def->data.emulator.source_path);
> +        }
>           break;
>       case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
>           if (def->data.external.source->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 6b27322e3e..7a70f68177 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1463,6 +1463,13 @@ typedef enum {
>       VIR_DOMAIN_TPM_PCR_BANK_LAST
>   } virDomainPcrBank;
>   
> +typedef enum {
> +    VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT = 0,
> +    VIR_DOMAIN_TPM_SOURCE_TYPE_FILE,
> +
> +    VIR_DOMAIN_TPM_SOURCE_TYPE_LAST
> +} virDomainTPMSourceType;
> +
>   #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"
>   
>   struct _virDomainTPMDef {
> @@ -1478,6 +1485,7 @@ struct _virDomainTPMDef {
>           struct {
>               virDomainTPMVersion version;
>               virDomainChrSourceDef *source;
> +            virDomainTPMSourceType source_type;
>               char *source_path;
>               char *logfile;
>               unsigned int debug;
> @@ -4277,6 +4285,7 @@ VIR_ENUM_DECL(virDomainRNGBackend);
>   VIR_ENUM_DECL(virDomainTPMModel);
>   VIR_ENUM_DECL(virDomainTPMBackend);
>   VIR_ENUM_DECL(virDomainTPMVersion);
> +VIR_ENUM_DECL(virDomainTPMSourceType);
>   VIR_ENUM_DECL(virDomainTPMPcrBank);
>   VIR_ENUM_DECL(virDomainMemoryModel);
>   VIR_ENUM_DECL(virDomainMemoryBackingModel);
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index efb5f00d77..72c8b6c694 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -5923,6 +5923,7 @@
>             <interleave>
>               <ref name="tpm-backend-emulator-encryption"/>
>               <ref name="tpm-backend-emulator-active-pcr-banks"/>
> +            <ref name="tpm-backend-emulator-source"/>
>             </interleave>
>             <optional>
>               <attribute name="persistent_state">
> @@ -5981,6 +5982,19 @@
>       </optional>
>     </define>
>   
> +  <define name="tpm-backend-emulator-source">
> +    <optional>
> +      <element name="source">
> +        <attribute name="type">
> +          <value>file</value>
> +        </attribute>
> +        <attribute name="path">
> +          <ref name="filePath"/>
> +        </attribute>
> +      </element>
> +    </optional>
> +  </define>
> +
>     <define name="tpm-backend-emulator-encryption">
>       <optional>
>         <element name="encryption">
> diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> index 8a613db456..3d6300f544 100644
> --- a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> +++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> @@ -34,6 +34,7 @@
>             <sha256/>
>             <sha512/>
>           </active_pcr_banks>
> +        <source type='file' path='/path/to/state'/>
>         </backend>
>       </tpm>
>       <audio id='1' type='none'/>
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Marc-André Lureau 1 year, 4 months ago
Hi

On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Learn to parse a file path for the TPM state.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   docs/formatdomain.rst                       | 19 ++++++++++++++
> >   src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> >   src/conf/domain_conf.h                      |  9 +++++++
> >   src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> >   tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> >   5 files changed, 71 insertions(+)
> >
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 4336cff3ac..992bb98730 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> >      The default version used depends on the combination of hypervisor, guest
> >      architecture, TPM model and backend.
> >
> > +``source``
> > +   The ``source`` element specifies the location of the TPM state storage . This
> > +   element only works with the ``emulator`` backend.
> > +
> > +   If not specified, the storage configuration is left to libvirt discretion.
> > +
> > +   This element requires that swtpm v0.7 or later is installed.
> > +
> > +   The following attributes are supported:
> > +
> > +   ``type``
> > +      The type of storage. It's possible to provide "file" to utilize a single
> > +      file or block device where the TPM state will be stored.
> > +
> > +   ``path``
> > +      The path to the TPM state storage.
>
> The file backend of swtpm does not do the locking similar to what the
> dir backend does because those who added the file backend didn't
> need/want it. If we now give full control to the path of the TPM state
> file to the user via the domain XML then whose fault is it if two VMs
> use the same path to a file backend and stomp on the TPM state file? Is
> it the fault of the user because of how he defined the path in the XMLs?

Imho, it's desirable to have a similar locking behaviour regardless of
the backend and prevent users for mistakenly using the same file.

>
>
> > +
> > +   :since:`Since v10.8.0`
> > +
>
> >   ``persistent_state``
> >      The ``persistent_state`` attribute indicates whether 'swtpm' TPM state is
> >      kept or not when a transient domain is powered off or undefined. This
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 284a3815b3..9dd8b6b55d 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -1322,6 +1322,12 @@ VIR_ENUM_IMPL(virDomainTPMVersion,
> >                 "2.0",
> >   );
> >
> > +VIR_ENUM_IMPL(virDomainTPMSourceType,
> > +              VIR_DOMAIN_TPM_SOURCE_TYPE_LAST,
> > +              "default",
> > +              "file",
> > +);
> > +
> >   VIR_ENUM_IMPL(virDomainTPMPcrBank,
> >                 VIR_DOMAIN_TPM_PCR_BANK_LAST,
> >                 "sha1",
> > @@ -10784,6 +10790,7 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
> >       int nbackends;
> >       int nnodes;
> >       size_t i;
> > +    xmlNodePtr source_node = NULL;
> >       g_autofree char *path = NULL;
> >       g_autofree char *secretuuid = NULL;
> >       g_autofree char *persistent_state = NULL;
> > @@ -10857,6 +10864,22 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
> >               def->data.emulator.hassecretuuid = true;
> >           }
> >
> > +        source_node = virXPathNode("./backend/source", ctxt);
> > +        if (source_node) {
> > +            if (virXMLPropEnum(source_node, "type",
> > +                               virDomainTPMSourceTypeTypeFromString,
> > +                               VIR_XML_PROP_NONZERO,
> > +                               &def->data.emulator.source_type) < 0)
> > +                goto error;
> > +            path = virXMLPropString(source_node, "path");
> > +            if (!path) {
> > +                virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                               _("missing TPM source path"));
> > +                goto error;
> > +            }
> > +            def->data.emulator.source_path = g_steal_pointer(&path);
> > +        }
> > +
> >           persistent_state = virXMLPropString(backends[0], "persistent_state");
> >           if (persistent_state) {
> >               if (virStringParseYesNo(persistent_state,
> > @@ -25070,6 +25093,11 @@ virDomainTPMDefFormat(virBuffer *buf,
> >
> >               virXMLFormatElement(&backendChildBuf, "active_pcr_banks", NULL, &activePcrBanksBuf);
> >           }
> > +        if (def->data.emulator.source_type != VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT) {
> > +            virBufferAsprintf(&backendChildBuf, "<source type='%s'",
> > +                              virDomainTPMSourceTypeTypeToString(def->data.emulator.source_type));
> > +            virBufferEscapeString(&backendChildBuf, " path='%s'/>\n", def->data.emulator.source_path);
> > +        }
> >           break;
> >       case VIR_DOMAIN_TPM_TYPE_EXTERNAL:
> >           if (def->data.external.source->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 6b27322e3e..7a70f68177 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1463,6 +1463,13 @@ typedef enum {
> >       VIR_DOMAIN_TPM_PCR_BANK_LAST
> >   } virDomainPcrBank;
> >
> > +typedef enum {
> > +    VIR_DOMAIN_TPM_SOURCE_TYPE_DEFAULT = 0,
> > +    VIR_DOMAIN_TPM_SOURCE_TYPE_FILE,
> > +
> > +    VIR_DOMAIN_TPM_SOURCE_TYPE_LAST
> > +} virDomainTPMSourceType;
> > +
> >   #define VIR_DOMAIN_TPM_DEFAULT_DEVICE "/dev/tpm0"
> >
> >   struct _virDomainTPMDef {
> > @@ -1478,6 +1485,7 @@ struct _virDomainTPMDef {
> >           struct {
> >               virDomainTPMVersion version;
> >               virDomainChrSourceDef *source;
> > +            virDomainTPMSourceType source_type;
> >               char *source_path;
> >               char *logfile;
> >               unsigned int debug;
> > @@ -4277,6 +4285,7 @@ VIR_ENUM_DECL(virDomainRNGBackend);
> >   VIR_ENUM_DECL(virDomainTPMModel);
> >   VIR_ENUM_DECL(virDomainTPMBackend);
> >   VIR_ENUM_DECL(virDomainTPMVersion);
> > +VIR_ENUM_DECL(virDomainTPMSourceType);
> >   VIR_ENUM_DECL(virDomainTPMPcrBank);
> >   VIR_ENUM_DECL(virDomainMemoryModel);
> >   VIR_ENUM_DECL(virDomainMemoryBackingModel);
> > diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> > index efb5f00d77..72c8b6c694 100644
> > --- a/src/conf/schemas/domaincommon.rng
> > +++ b/src/conf/schemas/domaincommon.rng
> > @@ -5923,6 +5923,7 @@
> >             <interleave>
> >               <ref name="tpm-backend-emulator-encryption"/>
> >               <ref name="tpm-backend-emulator-active-pcr-banks"/>
> > +            <ref name="tpm-backend-emulator-source"/>
> >             </interleave>
> >             <optional>
> >               <attribute name="persistent_state">
> > @@ -5981,6 +5982,19 @@
> >       </optional>
> >     </define>
> >
> > +  <define name="tpm-backend-emulator-source">
> > +    <optional>
> > +      <element name="source">
> > +        <attribute name="type">
> > +          <value>file</value>
> > +        </attribute>
> > +        <attribute name="path">
> > +          <ref name="filePath"/>
> > +        </attribute>
> > +      </element>
> > +    </optional>
> > +  </define>
> > +
> >     <define name="tpm-backend-emulator-encryption">
> >       <optional>
> >         <element name="encryption">
> > diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> > index 8a613db456..3d6300f544 100644
> > --- a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> > +++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
> > @@ -34,6 +34,7 @@
> >             <sha256/>
> >             <sha512/>
> >           </active_pcr_banks>
> > +        <source type='file' path='/path/to/state'/>
> >         </backend>
> >       </tpm>
> >       <audio id='1' type='none'/>
>
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Stefan Berger 1 year, 4 months ago

On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> Learn to parse a file path for the TPM state.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    docs/formatdomain.rst                       | 19 ++++++++++++++
>>>    src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
>>>    src/conf/domain_conf.h                      |  9 +++++++
>>>    src/conf/schemas/domaincommon.rng           | 14 +++++++++++
>>>    tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
>>>    5 files changed, 71 insertions(+)
>>>
>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>> index 4336cff3ac..992bb98730 100644
>>> --- a/docs/formatdomain.rst
>>> +++ b/docs/formatdomain.rst
>>> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
>>>       The default version used depends on the combination of hypervisor, guest
>>>       architecture, TPM model and backend.
>>>
>>> +``source``
>>> +   The ``source`` element specifies the location of the TPM state storage . This
>>> +   element only works with the ``emulator`` backend.
>>> +
>>> +   If not specified, the storage configuration is left to libvirt discretion.
>>> +
>>> +   This element requires that swtpm v0.7 or later is installed.
>>> +
>>> +   The following attributes are supported:
>>> +
>>> +   ``type``
>>> +      The type of storage. It's possible to provide "file" to utilize a single
>>> +      file or block device where the TPM state will be stored.
>>> +
>>> +   ``path``
>>> +      The path to the TPM state storage.
>>
>> The file backend of swtpm does not do the locking similar to what the
>> dir backend does because those who added the file backend didn't
>> need/want it. If we now give full control to the path of the TPM state
>> file to the user via the domain XML then whose fault is it if two VMs
>> use the same path to a file backend and stomp on the TPM state file? Is
>> it the fault of the user because of how he defined the path in the XMLs?
> 
> Imho, it's desirable to have a similar locking behaviour regardless of
> the backend and prevent users for mistakenly using the same file.

We will only be able to support the locking with an option on the 
command line for swtpm (refelected by a new capability verb) and support 
this series here once that has become available with a new version of 
swtpm. Otherwise I would avoid giving full control to the path to the 
users but let libvirt choose a per-VM unique name for the state file.
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
> 
> 
> On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > Hi
> > 
> > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > 
> > > 
> > > 
> > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > 
> > > > Learn to parse a file path for the TPM state.
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >    docs/formatdomain.rst                       | 19 ++++++++++++++
> > > >    src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > >    src/conf/domain_conf.h                      |  9 +++++++
> > > >    src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > >    tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > >    5 files changed, 71 insertions(+)
> > > > 
> > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > index 4336cff3ac..992bb98730 100644
> > > > --- a/docs/formatdomain.rst
> > > > +++ b/docs/formatdomain.rst
> > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > >       The default version used depends on the combination of hypervisor, guest
> > > >       architecture, TPM model and backend.
> > > > 
> > > > +``source``
> > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > +   element only works with the ``emulator`` backend.
> > > > +
> > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > +
> > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > +
> > > > +   The following attributes are supported:
> > > > +
> > > > +   ``type``
> > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > +      file or block device where the TPM state will be stored.
> > > > +
> > > > +   ``path``
> > > > +      The path to the TPM state storage.
> > > 
> > > The file backend of swtpm does not do the locking similar to what the
> > > dir backend does because those who added the file backend didn't
> > > need/want it. If we now give full control to the path of the TPM state
> > > file to the user via the domain XML then whose fault is it if two VMs
> > > use the same path to a file backend and stomp on the TPM state file? Is
> > > it the fault of the user because of how he defined the path in the XMLs?
> > 
> > Imho, it's desirable to have a similar locking behaviour regardless of
> > the backend and prevent users for mistakenly using the same file.
> 
> We will only be able to support the locking with an option on the command
> line for swtpm (refelected by a new capability verb) and support this series
> here once that has become available with a new version of swtpm. Otherwise I
> would avoid giving full control to the path to the users but let libvirt
> choose a per-VM unique name for the state file.

Relying on libvirt to give a unique path does not avoid the need for
locking, because IME users are liable to do unexpected things like
putting a shared filesystem underneath, and libvirt won't guarantee
any uniqueness across hosts - locking is required for that.

With 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 :|
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Stefan Berger 1 year, 3 months ago

On 10/14/24 5:17 AM, Daniel P. Berrangé wrote:
> On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
>>
>>
>> On 10/11/24 10:10 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> Learn to parse a file path for the TPM state.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>     docs/formatdomain.rst                       | 19 ++++++++++++++
>>>>>     src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
>>>>>     src/conf/domain_conf.h                      |  9 +++++++
>>>>>     src/conf/schemas/domaincommon.rng           | 14 +++++++++++
>>>>>     tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
>>>>>     5 files changed, 71 insertions(+)
>>>>>
>>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>>> index 4336cff3ac..992bb98730 100644
>>>>> --- a/docs/formatdomain.rst
>>>>> +++ b/docs/formatdomain.rst
>>>>> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
>>>>>        The default version used depends on the combination of hypervisor, guest
>>>>>        architecture, TPM model and backend.
>>>>>
>>>>> +``source``
>>>>> +   The ``source`` element specifies the location of the TPM state storage . This
>>>>> +   element only works with the ``emulator`` backend.
>>>>> +
>>>>> +   If not specified, the storage configuration is left to libvirt discretion.
>>>>> +
>>>>> +   This element requires that swtpm v0.7 or later is installed.
>>>>> +
>>>>> +   The following attributes are supported:
>>>>> +
>>>>> +   ``type``
>>>>> +      The type of storage. It's possible to provide "file" to utilize a single
>>>>> +      file or block device where the TPM state will be stored.
>>>>> +
>>>>> +   ``path``
>>>>> +      The path to the TPM state storage.
>>>>
>>>> The file backend of swtpm does not do the locking similar to what the
>>>> dir backend does because those who added the file backend didn't
>>>> need/want it. If we now give full control to the path of the TPM state
>>>> file to the user via the domain XML then whose fault is it if two VMs
>>>> use the same path to a file backend and stomp on the TPM state file? Is
>>>> it the fault of the user because of how he defined the path in the XMLs?
>>>
>>> Imho, it's desirable to have a similar locking behaviour regardless of
>>> the backend and prevent users for mistakenly using the same file.
>>
>> We will only be able to support the locking with an option on the command
>> line for swtpm (refelected by a new capability verb) and support this series
>> here once that has become available with a new version of swtpm. Otherwise I
>> would avoid giving full control to the path to the users but let libvirt
>> choose a per-VM unique name for the state file.
> 
> Relying on libvirt to give a unique path does not avoid the need for
> locking, because IME users are liable to do unexpected things like
> putting a shared filesystem underneath, and libvirt won't guarantee
> any uniqueness across hosts - locking is required for that.

Can we just lock shared block devices without a shared filesystem 
somehow supporting the distributed locking? So far swtpm has been using 
fcntl(lock_fd, F_SETLK, ...) on a .lock file.

> 
> With regards,
> Daniel
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Mon, Oct 14, 2024 at 09:35:14AM -0400, Stefan Berger wrote:
> 
> 
> On 10/14/24 5:17 AM, Daniel P. Berrangé wrote:
> > On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
> > > 
> > > 
> > > On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > 
> > > > > > Learn to parse a file path for the TPM state.
> > > > > > 
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > >     docs/formatdomain.rst                       | 19 ++++++++++++++
> > > > > >     src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > > > >     src/conf/domain_conf.h                      |  9 +++++++
> > > > > >     src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > > > >     tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > > > >     5 files changed, 71 insertions(+)
> > > > > > 
> > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > > > index 4336cff3ac..992bb98730 100644
> > > > > > --- a/docs/formatdomain.rst
> > > > > > +++ b/docs/formatdomain.rst
> > > > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > > > >        The default version used depends on the combination of hypervisor, guest
> > > > > >        architecture, TPM model and backend.
> > > > > > 
> > > > > > +``source``
> > > > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > > > +   element only works with the ``emulator`` backend.
> > > > > > +
> > > > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > > > +
> > > > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > > > +
> > > > > > +   The following attributes are supported:
> > > > > > +
> > > > > > +   ``type``
> > > > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > > > +      file or block device where the TPM state will be stored.
> > > > > > +
> > > > > > +   ``path``
> > > > > > +      The path to the TPM state storage.
> > > > > 
> > > > > The file backend of swtpm does not do the locking similar to what the
> > > > > dir backend does because those who added the file backend didn't
> > > > > need/want it. If we now give full control to the path of the TPM state
> > > > > file to the user via the domain XML then whose fault is it if two VMs
> > > > > use the same path to a file backend and stomp on the TPM state file? Is
> > > > > it the fault of the user because of how he defined the path in the XMLs?
> > > > 
> > > > Imho, it's desirable to have a similar locking behaviour regardless of
> > > > the backend and prevent users for mistakenly using the same file.
> > > 
> > > We will only be able to support the locking with an option on the command
> > > line for swtpm (refelected by a new capability verb) and support this series
> > > here once that has become available with a new version of swtpm. Otherwise I
> > > would avoid giving full control to the path to the users but let libvirt
> > > choose a per-VM unique name for the state file.
> > 
> > Relying on libvirt to give a unique path does not avoid the need for
> > locking, because IME users are liable to do unexpected things like
> > putting a shared filesystem underneath, and libvirt won't guarantee
> > any uniqueness across hosts - locking is required for that.
> 
> Can we just lock shared block devices without a shared filesystem somehow
> supporting the distributed locking? So far swtpm has been using
> fcntl(lock_fd, F_SETLK, ...) on a .lock file.

fcntl(lock_fd, F_SETLK...) works fine when done on block device FDs.
The scope of any such locks is local to the OS though, it won't lock
across hosts, if the same blockdev is exposed to many hosts, so mgmt
apps still need to be careful not todo stupid things.

With 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 :|
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Marc-André Lureau 1 year, 3 months ago
Hi

On Mon, Oct 14, 2024 at 5:41 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Mon, Oct 14, 2024 at 09:35:14AM -0400, Stefan Berger wrote:
> >
> >
> > On 10/14/24 5:17 AM, Daniel P. Berrangé wrote:
> > > On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
> > > >
> > > >
> > > > On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > > > > Hi
> > > > >
> > > > > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > >
> > > > > > > Learn to parse a file path for the TPM state.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > ---
> > > > > > >     docs/formatdomain.rst                       | 19 ++++++++++++++
> > > > > > >     src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > > > > >     src/conf/domain_conf.h                      |  9 +++++++
> > > > > > >     src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > > > > >     tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > > > > >     5 files changed, 71 insertions(+)
> > > > > > >
> > > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > > > > index 4336cff3ac..992bb98730 100644
> > > > > > > --- a/docs/formatdomain.rst
> > > > > > > +++ b/docs/formatdomain.rst
> > > > > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > > > > >        The default version used depends on the combination of hypervisor, guest
> > > > > > >        architecture, TPM model and backend.
> > > > > > >
> > > > > > > +``source``
> > > > > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > > > > +   element only works with the ``emulator`` backend.
> > > > > > > +
> > > > > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > > > > +
> > > > > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > > > > +
> > > > > > > +   The following attributes are supported:
> > > > > > > +
> > > > > > > +   ``type``
> > > > > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > > > > +      file or block device where the TPM state will be stored.
> > > > > > > +
> > > > > > > +   ``path``
> > > > > > > +      The path to the TPM state storage.
> > > > > >
> > > > > > The file backend of swtpm does not do the locking similar to what the
> > > > > > dir backend does because those who added the file backend didn't
> > > > > > need/want it. If we now give full control to the path of the TPM state
> > > > > > file to the user via the domain XML then whose fault is it if two VMs
> > > > > > use the same path to a file backend and stomp on the TPM state file? Is
> > > > > > it the fault of the user because of how he defined the path in the XMLs?
> > > > >
> > > > > Imho, it's desirable to have a similar locking behaviour regardless of
> > > > > the backend and prevent users for mistakenly using the same file.
> > > >
> > > > We will only be able to support the locking with an option on the command
> > > > line for swtpm (refelected by a new capability verb) and support this series
> > > > here once that has become available with a new version of swtpm. Otherwise I
> > > > would avoid giving full control to the path to the users but let libvirt
> > > > choose a per-VM unique name for the state file.
> > >
> > > Relying on libvirt to give a unique path does not avoid the need for
> > > locking, because IME users are liable to do unexpected things like
> > > putting a shared filesystem underneath, and libvirt won't guarantee
> > > any uniqueness across hosts - locking is required for that.
> >
> > Can we just lock shared block devices without a shared filesystem somehow
> > supporting the distributed locking? So far swtpm has been using
> > fcntl(lock_fd, F_SETLK, ...) on a .lock file.
>
> fcntl(lock_fd, F_SETLK...) works fine when done on block device FDs.
> The scope of any such locks is local to the OS though, it won't lock
> across hosts, if the same blockdev is exposed to many hosts, so mgmt
> apps still need to be careful not todo stupid things.
>

Now that tpmstate-opt-lock is provided by swtpm
(https://github.com/stefanberger/swtpm/commit/aa483aeb6df87ed56ccf3d5778d6fd8019089bda),
should we make the file backend feature depend on it? Or should
libvirt just warn if locking isn't available?

Looking for advice on what to do to get this series merged.

thanks
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Mon, Oct 21, 2024 at 03:06:13PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Oct 14, 2024 at 5:41 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Mon, Oct 14, 2024 at 09:35:14AM -0400, Stefan Berger wrote:
> > >
> > >
> > > On 10/14/24 5:17 AM, Daniel P. Berrangé wrote:
> > > > On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
> > > > >
> > > > >
> > > > > On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > >
> > > > > > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > >
> > > > > > > > Learn to parse a file path for the TPM state.
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > ---
> > > > > > > >     docs/formatdomain.rst                       | 19 ++++++++++++++
> > > > > > > >     src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > > > > > >     src/conf/domain_conf.h                      |  9 +++++++
> > > > > > > >     src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > > > > > >     tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > > > > > >     5 files changed, 71 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > > > > > index 4336cff3ac..992bb98730 100644
> > > > > > > > --- a/docs/formatdomain.rst
> > > > > > > > +++ b/docs/formatdomain.rst
> > > > > > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > > > > > >        The default version used depends on the combination of hypervisor, guest
> > > > > > > >        architecture, TPM model and backend.
> > > > > > > >
> > > > > > > > +``source``
> > > > > > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > > > > > +   element only works with the ``emulator`` backend.
> > > > > > > > +
> > > > > > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > > > > > +
> > > > > > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > > > > > +
> > > > > > > > +   The following attributes are supported:
> > > > > > > > +
> > > > > > > > +   ``type``
> > > > > > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > > > > > +      file or block device where the TPM state will be stored.
> > > > > > > > +
> > > > > > > > +   ``path``
> > > > > > > > +      The path to the TPM state storage.
> > > > > > >
> > > > > > > The file backend of swtpm does not do the locking similar to what the
> > > > > > > dir backend does because those who added the file backend didn't
> > > > > > > need/want it. If we now give full control to the path of the TPM state
> > > > > > > file to the user via the domain XML then whose fault is it if two VMs
> > > > > > > use the same path to a file backend and stomp on the TPM state file? Is
> > > > > > > it the fault of the user because of how he defined the path in the XMLs?
> > > > > >
> > > > > > Imho, it's desirable to have a similar locking behaviour regardless of
> > > > > > the backend and prevent users for mistakenly using the same file.
> > > > >
> > > > > We will only be able to support the locking with an option on the command
> > > > > line for swtpm (refelected by a new capability verb) and support this series
> > > > > here once that has become available with a new version of swtpm. Otherwise I
> > > > > would avoid giving full control to the path to the users but let libvirt
> > > > > choose a per-VM unique name for the state file.
> > > >
> > > > Relying on libvirt to give a unique path does not avoid the need for
> > > > locking, because IME users are liable to do unexpected things like
> > > > putting a shared filesystem underneath, and libvirt won't guarantee
> > > > any uniqueness across hosts - locking is required for that.
> > >
> > > Can we just lock shared block devices without a shared filesystem somehow
> > > supporting the distributed locking? So far swtpm has been using
> > > fcntl(lock_fd, F_SETLK, ...) on a .lock file.
> >
> > fcntl(lock_fd, F_SETLK...) works fine when done on block device FDs.
> > The scope of any such locks is local to the OS though, it won't lock
> > across hosts, if the same blockdev is exposed to many hosts, so mgmt
> > apps still need to be careful not todo stupid things.
> >
> 
> Now that tpmstate-opt-lock is provided by swtpm
> (https://github.com/stefanberger/swtpm/commit/aa483aeb6df87ed56ccf3d5778d6fd8019089bda),
> should we make the file backend feature depend on it? Or should
> libvirt just warn if locking isn't available?

I don't mind either way, as both options work.

With 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 :|
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Stefan Berger 1 year, 3 months ago

On 10/14/24 9:41 AM, Daniel P. Berrangé wrote:
> On Mon, Oct 14, 2024 at 09:35:14AM -0400, Stefan Berger wrote:
>>
>>
>> On 10/14/24 5:17 AM, Daniel P. Berrangé wrote:
>>> On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
>>>>
>>>>
>>>> On 10/11/24 10:10 AM, Marc-André Lureau wrote:
>>>>> Hi
>>>>>
>>>>> On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> Learn to parse a file path for the TPM state.
>>>>>>>
>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>> ---
>>>>>>>      docs/formatdomain.rst                       | 19 ++++++++++++++
>>>>>>>      src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
>>>>>>>      src/conf/domain_conf.h                      |  9 +++++++
>>>>>>>      src/conf/schemas/domaincommon.rng           | 14 +++++++++++
>>>>>>>      tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
>>>>>>>      5 files changed, 71 insertions(+)
>>>>>>>
>>>>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>>>>> index 4336cff3ac..992bb98730 100644
>>>>>>> --- a/docs/formatdomain.rst
>>>>>>> +++ b/docs/formatdomain.rst
>>>>>>> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
>>>>>>>         The default version used depends on the combination of hypervisor, guest
>>>>>>>         architecture, TPM model and backend.
>>>>>>>
>>>>>>> +``source``
>>>>>>> +   The ``source`` element specifies the location of the TPM state storage . This
>>>>>>> +   element only works with the ``emulator`` backend.
>>>>>>> +
>>>>>>> +   If not specified, the storage configuration is left to libvirt discretion.
>>>>>>> +
>>>>>>> +   This element requires that swtpm v0.7 or later is installed.
>>>>>>> +
>>>>>>> +   The following attributes are supported:
>>>>>>> +
>>>>>>> +   ``type``
>>>>>>> +      The type of storage. It's possible to provide "file" to utilize a single
>>>>>>> +      file or block device where the TPM state will be stored.
>>>>>>> +
>>>>>>> +   ``path``
>>>>>>> +      The path to the TPM state storage.
>>>>>>
>>>>>> The file backend of swtpm does not do the locking similar to what the
>>>>>> dir backend does because those who added the file backend didn't
>>>>>> need/want it. If we now give full control to the path of the TPM state
>>>>>> file to the user via the domain XML then whose fault is it if two VMs
>>>>>> use the same path to a file backend and stomp on the TPM state file? Is
>>>>>> it the fault of the user because of how he defined the path in the XMLs?
>>>>>
>>>>> Imho, it's desirable to have a similar locking behaviour regardless of
>>>>> the backend and prevent users for mistakenly using the same file.
>>>>
>>>> We will only be able to support the locking with an option on the command
>>>> line for swtpm (refelected by a new capability verb) and support this series
>>>> here once that has become available with a new version of swtpm. Otherwise I
>>>> would avoid giving full control to the path to the users but let libvirt
>>>> choose a per-VM unique name for the state file.
>>>
>>> Relying on libvirt to give a unique path does not avoid the need for
>>> locking, because IME users are liable to do unexpected things like
>>> putting a shared filesystem underneath, and libvirt won't guarantee
>>> any uniqueness across hosts - locking is required for that.
>>
>> Can we just lock shared block devices without a shared filesystem somehow
>> supporting the distributed locking? So far swtpm has been using
>> fcntl(lock_fd, F_SETLK, ...) on a .lock file.
> 
> fcntl(lock_fd, F_SETLK...) works fine when done on block device FDs.
> The scope of any such locks is local to the OS though, it won't lock
> across hosts, if the same blockdev is exposed to many hosts, so mgmt
> apps still need to be careful not todo stupid things.

Can non-careful users destroy any partition now when specifying it for 
TPM state storage?

> 
> With regards,
> Daniel
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Mon, Oct 14, 2024 at 10:41:56AM -0400, Stefan Berger wrote:
> 
> 
> On 10/14/24 9:41 AM, Daniel P. Berrangé wrote:
> > On Mon, Oct 14, 2024 at 09:35:14AM -0400, Stefan Berger wrote:
> > > 
> > > 
> > > On 10/14/24 5:17 AM, Daniel P. Berrangé wrote:
> > > > On Fri, Oct 11, 2024 at 10:16:51AM -0400, Stefan Berger wrote:
> > > > > 
> > > > > 
> > > > > On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > > 
> > > > > > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > 
> > > > > > > > Learn to parse a file path for the TPM state.
> > > > > > > > 
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > ---
> > > > > > > >      docs/formatdomain.rst                       | 19 ++++++++++++++
> > > > > > > >      src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > > > > > >      src/conf/domain_conf.h                      |  9 +++++++
> > > > > > > >      src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > > > > > >      tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > > > > > >      5 files changed, 71 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > > > > > index 4336cff3ac..992bb98730 100644
> > > > > > > > --- a/docs/formatdomain.rst
> > > > > > > > +++ b/docs/formatdomain.rst
> > > > > > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > > > > > >         The default version used depends on the combination of hypervisor, guest
> > > > > > > >         architecture, TPM model and backend.
> > > > > > > > 
> > > > > > > > +``source``
> > > > > > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > > > > > +   element only works with the ``emulator`` backend.
> > > > > > > > +
> > > > > > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > > > > > +
> > > > > > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > > > > > +
> > > > > > > > +   The following attributes are supported:
> > > > > > > > +
> > > > > > > > +   ``type``
> > > > > > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > > > > > +      file or block device where the TPM state will be stored.
> > > > > > > > +
> > > > > > > > +   ``path``
> > > > > > > > +      The path to the TPM state storage.
> > > > > > > 
> > > > > > > The file backend of swtpm does not do the locking similar to what the
> > > > > > > dir backend does because those who added the file backend didn't
> > > > > > > need/want it. If we now give full control to the path of the TPM state
> > > > > > > file to the user via the domain XML then whose fault is it if two VMs
> > > > > > > use the same path to a file backend and stomp on the TPM state file? Is
> > > > > > > it the fault of the user because of how he defined the path in the XMLs?
> > > > > > 
> > > > > > Imho, it's desirable to have a similar locking behaviour regardless of
> > > > > > the backend and prevent users for mistakenly using the same file.
> > > > > 
> > > > > We will only be able to support the locking with an option on the command
> > > > > line for swtpm (refelected by a new capability verb) and support this series
> > > > > here once that has become available with a new version of swtpm. Otherwise I
> > > > > would avoid giving full control to the path to the users but let libvirt
> > > > > choose a per-VM unique name for the state file.
> > > > 
> > > > Relying on libvirt to give a unique path does not avoid the need for
> > > > locking, because IME users are liable to do unexpected things like
> > > > putting a shared filesystem underneath, and libvirt won't guarantee
> > > > any uniqueness across hosts - locking is required for that.
> > > 
> > > Can we just lock shared block devices without a shared filesystem somehow
> > > supporting the distributed locking? So far swtpm has been using
> > > fcntl(lock_fd, F_SETLK, ...) on a .lock file.
> > 
> > fcntl(lock_fd, F_SETLK...) works fine when done on block device FDs.
> > The scope of any such locks is local to the OS though, it won't lock
> > across hosts, if the same blockdev is exposed to many hosts, so mgmt
> > apps still need to be careful not todo stupid things.
> 
> Can non-careful users destroy any partition now when specifying it for TPM
> state storage?

Anything is possible, but that's not our problem. Libvirt provides the
mechanism to do things, but aims to stay out of "usage policy" decisions
as much as practical.

With 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 :|
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Marc-André Lureau 1 year, 4 months ago
Hi

On Fri, Oct 11, 2024 at 6:17 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> >>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>
> >>> Learn to parse a file path for the TPM state.
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>> ---
> >>>    docs/formatdomain.rst                       | 19 ++++++++++++++
> >>>    src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> >>>    src/conf/domain_conf.h                      |  9 +++++++
> >>>    src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> >>>    tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> >>>    5 files changed, 71 insertions(+)
> >>>
> >>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> >>> index 4336cff3ac..992bb98730 100644
> >>> --- a/docs/formatdomain.rst
> >>> +++ b/docs/formatdomain.rst
> >>> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> >>>       The default version used depends on the combination of hypervisor, guest
> >>>       architecture, TPM model and backend.
> >>>
> >>> +``source``
> >>> +   The ``source`` element specifies the location of the TPM state storage . This
> >>> +   element only works with the ``emulator`` backend.
> >>> +
> >>> +   If not specified, the storage configuration is left to libvirt discretion.
> >>> +
> >>> +   This element requires that swtpm v0.7 or later is installed.
> >>> +
> >>> +   The following attributes are supported:
> >>> +
> >>> +   ``type``
> >>> +      The type of storage. It's possible to provide "file" to utilize a single
> >>> +      file or block device where the TPM state will be stored.
> >>> +
> >>> +   ``path``
> >>> +      The path to the TPM state storage.
> >>
> >> The file backend of swtpm does not do the locking similar to what the
> >> dir backend does because those who added the file backend didn't
> >> need/want it. If we now give full control to the path of the TPM state
> >> file to the user via the domain XML then whose fault is it if two VMs
> >> use the same path to a file backend and stomp on the TPM state file? Is
> >> it the fault of the user because of how he defined the path in the XMLs?
> >
> > Imho, it's desirable to have a similar locking behaviour regardless of
> > the backend and prevent users for mistakenly using the same file.
>
> We will only be able to support the locking with an option on the
> command line for swtpm (refelected by a new capability verb) and support
> this series here once that has become available with a new version of
> swtpm. Otherwise I would avoid giving full control to the path to the
> users but let libvirt choose a per-VM unique name for the state file.

The use-case is to let the user define a specific block device path.
There isn't much interest in merely switching from directory to file
backend otherwise, afaik.

I think we could also address the missing locking in libvirt, since
all the resources managed by libvirt should be under its control.
Extra locking can be added by swtpm later.
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Stefan Berger 1 year, 4 months ago

On 10/11/24 10:32 AM, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Oct 11, 2024 at 6:17 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 10/11/24 10:10 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> Learn to parse a file path for the TPM state.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>     docs/formatdomain.rst                       | 19 ++++++++++++++
>>>>>     src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
>>>>>     src/conf/domain_conf.h                      |  9 +++++++
>>>>>     src/conf/schemas/domaincommon.rng           | 14 +++++++++++
>>>>>     tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
>>>>>     5 files changed, 71 insertions(+)
>>>>>
>>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>>> index 4336cff3ac..992bb98730 100644
>>>>> --- a/docs/formatdomain.rst
>>>>> +++ b/docs/formatdomain.rst
>>>>> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
>>>>>        The default version used depends on the combination of hypervisor, guest
>>>>>        architecture, TPM model and backend.
>>>>>
>>>>> +``source``
>>>>> +   The ``source`` element specifies the location of the TPM state storage . This
>>>>> +   element only works with the ``emulator`` backend.
>>>>> +
>>>>> +   If not specified, the storage configuration is left to libvirt discretion.
>>>>> +
>>>>> +   This element requires that swtpm v0.7 or later is installed.
>>>>> +
>>>>> +   The following attributes are supported:
>>>>> +
>>>>> +   ``type``
>>>>> +      The type of storage. It's possible to provide "file" to utilize a single
>>>>> +      file or block device where the TPM state will be stored.
>>>>> +
>>>>> +   ``path``
>>>>> +      The path to the TPM state storage.
>>>>
>>>> The file backend of swtpm does not do the locking similar to what the
>>>> dir backend does because those who added the file backend didn't
>>>> need/want it. If we now give full control to the path of the TPM state
>>>> file to the user via the domain XML then whose fault is it if two VMs
>>>> use the same path to a file backend and stomp on the TPM state file? Is
>>>> it the fault of the user because of how he defined the path in the XMLs?
>>>
>>> Imho, it's desirable to have a similar locking behaviour regardless of
>>> the backend and prevent users for mistakenly using the same file.
>>
>> We will only be able to support the locking with an option on the
>> command line for swtpm (refelected by a new capability verb) and support
>> this series here once that has become available with a new version of
>> swtpm. Otherwise I would avoid giving full control to the path to the
>> users but let libvirt choose a per-VM unique name for the state file.
> 
> The use-case is to let the user define a specific block device path.

Why would they store it on a block device rather than a file system?
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Fri, Oct 11, 2024 at 10:46:58AM -0400, Stefan Berger wrote:
> 
> 
> On 10/11/24 10:32 AM, Marc-André Lureau wrote:
> > Hi
> > 
> > On Fri, Oct 11, 2024 at 6:17 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > 
> > > 
> > > 
> > > On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > 
> > > > > > Learn to parse a file path for the TPM state.
> > > > > > 
> > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > ---
> > > > > >     docs/formatdomain.rst                       | 19 ++++++++++++++
> > > > > >     src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > > > >     src/conf/domain_conf.h                      |  9 +++++++
> > > > > >     src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > > > >     tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > > > >     5 files changed, 71 insertions(+)
> > > > > > 
> > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > > > index 4336cff3ac..992bb98730 100644
> > > > > > --- a/docs/formatdomain.rst
> > > > > > +++ b/docs/formatdomain.rst
> > > > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > > > >        The default version used depends on the combination of hypervisor, guest
> > > > > >        architecture, TPM model and backend.
> > > > > > 
> > > > > > +``source``
> > > > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > > > +   element only works with the ``emulator`` backend.
> > > > > > +
> > > > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > > > +
> > > > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > > > +
> > > > > > +   The following attributes are supported:
> > > > > > +
> > > > > > +   ``type``
> > > > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > > > +      file or block device where the TPM state will be stored.
> > > > > > +
> > > > > > +   ``path``
> > > > > > +      The path to the TPM state storage.
> > > > > 
> > > > > The file backend of swtpm does not do the locking similar to what the
> > > > > dir backend does because those who added the file backend didn't
> > > > > need/want it. If we now give full control to the path of the TPM state
> > > > > file to the user via the domain XML then whose fault is it if two VMs
> > > > > use the same path to a file backend and stomp on the TPM state file? Is
> > > > > it the fault of the user because of how he defined the path in the XMLs?
> > > > 
> > > > Imho, it's desirable to have a similar locking behaviour regardless of
> > > > the backend and prevent users for mistakenly using the same file.
> > > 
> > > We will only be able to support the locking with an option on the
> > > command line for swtpm (refelected by a new capability verb) and support
> > > this series here once that has become available with a new version of
> > > swtpm. Otherwise I would avoid giving full control to the path to the
> > > users but let libvirt choose a per-VM unique name for the state file.
> > 
> > The use-case is to let the user define a specific block device path.
> 
> Why would they store it on a block device rather than a file system?

If they want to make the storage available to multiple hosts, then using
a block device is simpler, as most filesystems are unsafe to concurrently
expose in multiple hosts.

With 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 :|
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Stefan Berger 1 year, 3 months ago

On 10/14/24 5:19 AM, Daniel P. Berrangé wrote:
> On Fri, Oct 11, 2024 at 10:46:58AM -0400, Stefan Berger wrote:
>>
>>
>> On 10/11/24 10:32 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Fri, Oct 11, 2024 at 6:17 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/11/24 10:10 AM, Marc-André Lureau wrote:
>>>>> Hi
>>>>>
>>>>> On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
>>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>>
>>>>>>> Learn to parse a file path for the TPM state.
>>>>>>>
>>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>> ---
>>>>>>>      docs/formatdomain.rst                       | 19 ++++++++++++++
>>>>>>>      src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
>>>>>>>      src/conf/domain_conf.h                      |  9 +++++++
>>>>>>>      src/conf/schemas/domaincommon.rng           | 14 +++++++++++
>>>>>>>      tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
>>>>>>>      5 files changed, 71 insertions(+)
>>>>>>>
>>>>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>>>>> index 4336cff3ac..992bb98730 100644
>>>>>>> --- a/docs/formatdomain.rst
>>>>>>> +++ b/docs/formatdomain.rst
>>>>>>> @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
>>>>>>>         The default version used depends on the combination of hypervisor, guest
>>>>>>>         architecture, TPM model and backend.
>>>>>>>
>>>>>>> +``source``
>>>>>>> +   The ``source`` element specifies the location of the TPM state storage . This
>>>>>>> +   element only works with the ``emulator`` backend.
>>>>>>> +
>>>>>>> +   If not specified, the storage configuration is left to libvirt discretion.
>>>>>>> +
>>>>>>> +   This element requires that swtpm v0.7 or later is installed.
>>>>>>> +
>>>>>>> +   The following attributes are supported:
>>>>>>> +
>>>>>>> +   ``type``
>>>>>>> +      The type of storage. It's possible to provide "file" to utilize a single
>>>>>>> +      file or block device where the TPM state will be stored.
>>>>>>> +
>>>>>>> +   ``path``
>>>>>>> +      The path to the TPM state storage.
>>>>>>
>>>>>> The file backend of swtpm does not do the locking similar to what the
>>>>>> dir backend does because those who added the file backend didn't
>>>>>> need/want it. If we now give full control to the path of the TPM state
>>>>>> file to the user via the domain XML then whose fault is it if two VMs
>>>>>> use the same path to a file backend and stomp on the TPM state file? Is
>>>>>> it the fault of the user because of how he defined the path in the XMLs?
>>>>>
>>>>> Imho, it's desirable to have a similar locking behaviour regardless of
>>>>> the backend and prevent users for mistakenly using the same file.
>>>>
>>>> We will only be able to support the locking with an option on the
>>>> command line for swtpm (refelected by a new capability verb) and support
>>>> this series here once that has become available with a new version of
>>>> swtpm. Otherwise I would avoid giving full control to the path to the
>>>> users but let libvirt choose a per-VM unique name for the state file.
>>>
>>> The use-case is to let the user define a specific block device path.
>>
>> Why would they store it on a block device rather than a file system?
> 
> If they want to make the storage available to multiple hosts, then using
> a block device is simpler, as most filesystems are unsafe to concurrently
> expose in multiple hosts.

You then need n block devices for n swtpm instances?

> 
> With regards,
> Daniel
Re: [PATCH v3 3/5] schema: add TPM emulator <source type='file' path='..'>
Posted by Daniel P. Berrangé 1 year, 3 months ago
On Mon, Oct 14, 2024 at 09:15:16AM -0400, Stefan Berger wrote:
> 
> 
> On 10/14/24 5:19 AM, Daniel P. Berrangé wrote:
> > On Fri, Oct 11, 2024 at 10:46:58AM -0400, Stefan Berger wrote:
> > > 
> > > 
> > > On 10/11/24 10:32 AM, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Fri, Oct 11, 2024 at 6:17 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On 10/11/24 10:10 AM, Marc-André Lureau wrote:
> > > > > > Hi
> > > > > > 
> > > > > > On Fri, Oct 11, 2024 at 5:49 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/4/24 9:32 AM, marcandre.lureau@redhat.com wrote:
> > > > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > 
> > > > > > > > Learn to parse a file path for the TPM state.
> > > > > > > > 
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > > > > > ---
> > > > > > > >      docs/formatdomain.rst                       | 19 ++++++++++++++
> > > > > > > >      src/conf/domain_conf.c                      | 28 +++++++++++++++++++++
> > > > > > > >      src/conf/domain_conf.h                      |  9 +++++++
> > > > > > > >      src/conf/schemas/domaincommon.rng           | 14 +++++++++++
> > > > > > > >      tests/qemuxmlconfdata/tpm-emulator-tpm2.xml |  1 +
> > > > > > > >      5 files changed, 71 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > > > > > > index 4336cff3ac..992bb98730 100644
> > > > > > > > --- a/docs/formatdomain.rst
> > > > > > > > +++ b/docs/formatdomain.rst
> > > > > > > > @@ -8173,6 +8173,25 @@ Example: usage of the TPM Emulator
> > > > > > > >         The default version used depends on the combination of hypervisor, guest
> > > > > > > >         architecture, TPM model and backend.
> > > > > > > > 
> > > > > > > > +``source``
> > > > > > > > +   The ``source`` element specifies the location of the TPM state storage . This
> > > > > > > > +   element only works with the ``emulator`` backend.
> > > > > > > > +
> > > > > > > > +   If not specified, the storage configuration is left to libvirt discretion.
> > > > > > > > +
> > > > > > > > +   This element requires that swtpm v0.7 or later is installed.
> > > > > > > > +
> > > > > > > > +   The following attributes are supported:
> > > > > > > > +
> > > > > > > > +   ``type``
> > > > > > > > +      The type of storage. It's possible to provide "file" to utilize a single
> > > > > > > > +      file or block device where the TPM state will be stored.
> > > > > > > > +
> > > > > > > > +   ``path``
> > > > > > > > +      The path to the TPM state storage.
> > > > > > > 
> > > > > > > The file backend of swtpm does not do the locking similar to what the
> > > > > > > dir backend does because those who added the file backend didn't
> > > > > > > need/want it. If we now give full control to the path of the TPM state
> > > > > > > file to the user via the domain XML then whose fault is it if two VMs
> > > > > > > use the same path to a file backend and stomp on the TPM state file? Is
> > > > > > > it the fault of the user because of how he defined the path in the XMLs?
> > > > > > 
> > > > > > Imho, it's desirable to have a similar locking behaviour regardless of
> > > > > > the backend and prevent users for mistakenly using the same file.
> > > > > 
> > > > > We will only be able to support the locking with an option on the
> > > > > command line for swtpm (refelected by a new capability verb) and support
> > > > > this series here once that has become available with a new version of
> > > > > swtpm. Otherwise I would avoid giving full control to the path to the
> > > > > users but let libvirt choose a per-VM unique name for the state file.
> > > > 
> > > > The use-case is to let the user define a specific block device path.
> > > 
> > > Why would they store it on a block device rather than a file system?
> > 
> > If they want to make the storage available to multiple hosts, then using
> > a block device is simpler, as most filesystems are unsafe to concurrently
> > expose in multiple hosts.
> 
> You then need n block devices for n swtpm instances?

Yes.

In the context of something like KubeVirt this is fine, each VM runs inside
a POD, and each POD can be allocated a block device by Kubernetes.

With 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 :|