[PATCH 0/1] qemu_tpm: Start swtpm(8) daemon with --terminate switch
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
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@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
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@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
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
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@redhat.com> and pushed. Congratulations on your first libvirt contribution! Michal
participants (2)
-
Michal Prívozník -
Nick Chevsky