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

Hi On Mon, Oct 14, 2024 at 5:41 PM Daniel P. Berrangé <berrange(a)redhat.com&gt; 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(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. > Now that tpmstate-opt-lock is provided by swtpm (https://github.com/stefanberger/swtpm/commit/aa483aeb6df87ed56ccf3d5778d6...), should we make the file backend feature depend on it? Or should libvirt just warn if locking isn't available?