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

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(a)linux.ibm.com&gt; wrote: > > > > > > > > > > > > > > > > > > > > On 10/4/24 9:32 AM, marcandre.lureau(a)redhat.com wrote: > > > > > > From: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > > > > > > > > > > > > Learn to parse a file path for the TPM state. > > > > > > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; > > > > > > --- > > > > > > 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.