1:16 a.m.
libvirt expects the swtpm(8) daemon to auto-terminate along with QEMU.
While that's already the case, it's currently happening for the wrong
reason: swtpm's documented way of achieving this behavior is via the
--terminate switch (which causes the daemon to shut down when the
data channel connection drops), but libvirt isn't currently using
this switch--and it should.
The reason this currently works anyway, even without the --terminate
switch, is two-fold:
(1) When QEMU terminates gracefully, it sends command CMD_SHUTDOWN to
swtpm which triggers a shutdown. Nothing wrong with this one.
(2) When QEMU dies abruptly (e.g. SIGKILL, SIGSEGV) without issuing
CMD_SHUTDOWN, swtpm should (a) shut down if the --terminate switch
was given OR (b) stay alive if --terminate wasn't given. At the
moment this isn't being respected, and swtpm unconditionally shuts
down (regardless of whether --terminate was given or not) due to a
bug in swtpm's connection handling logic [1]. libvirt currently
relies on this incorrect and undocumented upstream behavior,
trusting swtpm to shut itself down even when --terminate wasn't
given, which is wrong and bound to break.
The discussion [1] between swtpm's author and I shows that --terminate
(a) is the proper way to achieve--and guarantee--the current behavior,
(b) is innocuous to add since it won't alter existing behavior, (c)
should've been used by libvirt all along, and (d) should be enforced
by swtpm going forward.
Since libvirt presently relies on swtpm's current (incorrect) behavior
and we don't want to break libvirt, we need libvirt to start invoking
swtpm with the --terminate switch ASAP so that the upstream bug can
be fixed as soon as it's safe. Fixing the bug is the first step toward
eventually enabling non-libvirt swtpm users to optionally run swtpm as
a persistent service, allowing a VM to connect to and disconnect from
it without the daemon dying.
Proxmox VE, to which I also contribute, is already using --terminate
in its (WIP) swtpm implementation.
[1] https://github.com/stefanberger/swtpm/pull/509 -- Note that this
already-merged PR addresses only one half of the bug; the other
half (which will actually effect the change) remains on hold until
libvirt implements --terminate.
Nick Chevsky (1):
qemu_tpm: Start swtpm(8) daemon with --terminate switch
src/qemu/qemu_tpm.c | 2 ++
1 file changed, 2 insertions(+)
--
2.30.2
1:16 a.m.
New subject: [PATCH 1/1] qemu_tpm: Start swtpm(8) daemon with --terminate switch
Launch swtpm(8) with the --terminate switch, which guarantees that
the daemon will shut itself down when QEMU dies (current behavior).
We had so far been getting this "for free" (i.e. without --terminate)
due to a defect in upstream's connection handling logic [1], on which
libvirt should not rely since it will eventually be fixed. Adding
--terminate preserves and guarantees the current behavior.
[1] https://github.com/stefanberger/swtpm/pull/509
Signed-off-by: Nick Chevsky <nchevsky(a)gmail.com>
---
src/qemu/qemu_tpm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 477a26dc69..100481503c 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -576,6 +576,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
virCommandAddArg(cmd, "--log");
virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.logfile);
+ virCommandAddArg(cmd, "--terminate");
+
virCommandSetUID(cmd, swtpm_user);
virCommandSetGID(cmd, swtpm_group);
--
2.30.2
5:43 a.m.
New subject: [PATCH 1/1] qemu_tpm: Start swtpm(8) daemon with --terminate switch
On 9/13/21 8:16 AM, Nick Chevsky wrote:
Launch swtpm(8) with the --terminate switch, which guarantees that
the daemon will shut itself down when QEMU dies (current behavior).
We had so far been getting this "for free" (i.e. without --terminate)
due to a defect in upstream's connection handling logic [1], on which
libvirt should not rely since it will eventually be fixed. Adding
--terminate preserves and guarantees the current behavior.
[1] https://github.com/stefanberger/swtpm/pull/509
Signed-off-by: Nick Chevsky <nchevsky(a)gmail.com>
---
src/qemu/qemu_tpm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c
index 477a26dc69..100481503c 100644
--- a/src/qemu/qemu_tpm.c
+++ b/src/qemu/qemu_tpm.c
@@ -576,6 +576,8 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm,
virCommandAddArg(cmd, "--log");
virCommandAddArgFormat(cmd, "file=%s", tpm->data.emulator.logfile);
+ virCommandAddArg(cmd, "--terminate");
+
virCommandSetUID(cmd, swtpm_user);
virCommandSetGID(cmd, swtpm_group);
The patch is correct, but what we already have is qemuExtDevicesStop()
being called from qemuProcessStop(). The former will eventually call
qemuTPMEmulatorStop() which should kill the swtmp process, shouldn't it?
Or this patch is there to kill swtmp earlier, i.e. as soon as it sees
HUP on the socket?
Michal
12:34 p.m.
New subject: [PATCH 1/1] qemu_tpm: Start swtpm(8) daemon with --terminate switch
Hi Michal,
The patch is correct, but what we already have is
qemuExtDevicesStop()
being called from qemuProcessStop(). The former will eventually call
qemuTPMEmulatorStop() which should kill the swtmp process, shouldn't it?
I'd missed this and that's great to hear, because it means the
upstream bug (i.e. swtpm behaving as though --terminate was given even
when it wasn't) can be fixed now, without breaking libvirt.
Or this patch is there to kill swtmp earlier, i.e. as soon as it
sees
HUP on the socket?
That's the thing; this is already (incorrectly) happening right now:
swtpm is self-terminating as soon as it sees HUP on the socket, even
though --terminate wasn't given. In other words, we're currently
seeing --terminate behavior *without* --terminate. Due to this
upstream bug, I suspect swtpm is already shutting itself down by the
time libvirt executes qemuExtDevicesStop().
Once the upstream bug is fixed, swtpm will no longer self-terminate on
HUP *unless* --terminate was given. If libvirt doesn't add
--terminate, swtpm will then stay up (as it should) and be killed by
qemuExtDevicesStop() instead. Does that make sense?
While the end result will ultimately be the same, I'd still argue that
it's a good idea for libvirt to start invoking swtpm with --terminate
since (a) it's free, (b) it's more graceful, (c) it takes some
responsibility off libvirt, and (d) it adds an extra layer of
redundancy by allowing swtpm to try to shut itself down first and, if
that fails, have libvirt's qemuExtDevicesStop() kill it.
Nick
3:07 a.m.
New subject: [PATCH 1/1] qemu_tpm: Start swtpm(8) daemon with --terminate switch
On 9/14/21 7:34 PM, Nick Chevsky wrote:
Hi Michal,
> The patch is correct, but what we already have is qemuExtDevicesStop()
> being called from qemuProcessStop(). The former will eventually call
> qemuTPMEmulatorStop() which should kill the swtmp process, shouldn't it?
I'd missed this and that's great to hear, because it means the
upstream bug (i.e. swtpm behaving as though --terminate was given even
when it wasn't) can be fixed now, without breaking libvirt.
> Or this patch is there to kill swtmp earlier, i.e. as soon as it sees
> HUP on the socket?
That's the thing; this is already (incorrectly) happening right now:
swtpm is self-terminating as soon as it sees HUP on the socket, even
though --terminate wasn't given. In other words, we're currently
seeing --terminate behavior *without* --terminate. Due to this
upstream bug, I suspect swtpm is already shutting itself down by the
time libvirt executes qemuExtDevicesStop().
Once the upstream bug is fixed, swtpm will no longer self-terminate on
HUP *unless* --terminate was given. If libvirt doesn't add
--terminate, swtpm will then stay up (as it should) and be killed by
qemuExtDevicesStop() instead. Does that make sense?
While the end result will ultimately be the same, I'd still argue that
it's a good idea for libvirt to start invoking swtpm with --terminate
since (a) it's free, (b) it's more graceful, (c) it takes some
responsibility off libvirt, and (d) it adds an extra layer of
redundancy by allowing swtpm to try to shut itself down first and, if
that fails, have libvirt's qemuExtDevicesStop() kill it.
Just to be clear, I am not against this patch, in fact the opposite.
Just wanted to make clear I understand what's going on. It's good to
have redundancy.
Looking at git blame of swtpm.c I can see that --terminate was there
since the initial commit so we don't need any additional machinery to
determine whether the argument is supported or not. That's good.
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and pushed. Congratulations on your first libvirt contribution!
Michal
1164
days inactive
1166
days old
4 comments
2 participants
participants (2)
-
Michal Prívozník -
Nick Chevsky