[PATCH] qemu_tpm: Do async IO when starting swtpm emulator
When vTPM is secured via virSecret libvirt passes the secret value via an FD when swtpm is started (arguments --key and --migration-key). The writing of the secret into the FDs is handled via virCommand, specifically qemu_tpm calls virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a thread to handle writing into the FD via virCommandDoAsyncIOHelper. But the thread is not created unless VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix it, virCommandDoAsyncIO() must be called. The credit goes to Marc-André Lureau <marcandre.lureau@redhat.com> who has done all the debugging and proposed fix in the bugzilla. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2064115 Fixes: a9c500d2b50c5c041a1bb6ae9724402cf1cec8fe Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 50f9caabf3..56bccee128 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -899,6 +899,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, if (!(pidfile = qemuTPMEmulatorPidFileBuildPath(cfg->swtpmStateDir, shortName))) return -1; + virCommandDoAsyncIO(cmd); virCommandDaemonize(cmd); virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); -- 2.34.1
Hi On Mon, Mar 21, 2022 at 6:59 PM Michal Privoznik <mprivozn@redhat.com> wrote:
When vTPM is secured via virSecret libvirt passes the secret value via an FD when swtpm is started (arguments --key and --migration-key). The writing of the secret into the FDs is handled via virCommand, specifically qemu_tpm calls virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a thread to handle writing into the FD via virCommandDoAsyncIOHelper. But the thread is not created unless VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix it, virCommandDoAsyncIO() must be called.
The credit goes to Marc-André Lureau <marcandre.lureau@redhat.com> who has done all the debugging and proposed fix in the bugzilla.
(thanks for the credit :) Wouldn't it make sense to return an error if SendBuffers is used without AsyncIO then? Or just enable AsyncIO as necessary? (beware, I am not very familiar with virCommand API. I don't know what this would imply) Also it would be nice to cover that "behaviour" in a test (even better if we could cover the swtpm setup & start handling too, although I realize this is more work!)
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2064115 Fixes: a9c500d2b50c5c041a1bb6ae9724402cf1cec8fe Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_tpm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 50f9caabf3..56bccee128 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -899,6 +899,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, if (!(pidfile = qemuTPMEmulatorPidFileBuildPath(cfg->swtpmStateDir, shortName))) return -1;
+ virCommandDoAsyncIO(cmd); virCommandDaemonize(cmd); virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); -- 2.34.1
-- Marc-André Lureau
On 3/21/22 20:56, Marc-André Lureau wrote:
Hi
On Mon, Mar 21, 2022 at 6:59 PM Michal Privoznik <mprivozn@redhat.com <mailto:mprivozn@redhat.com>> wrote:
When vTPM is secured via virSecret libvirt passes the secret value via an FD when swtpm is started (arguments --key and --migration-key). The writing of the secret into the FDs is handled via virCommand, specifically qemu_tpm calls virCommandSetSendBuffer()) and then virCommandRunAsync() spawns a thread to handle writing into the FD via virCommandDoAsyncIOHelper. But the thread is not created unless VIR_EXEC_ASYNC_IO flag is set, which it isn't. In order to fix it, virCommandDoAsyncIO() must be called.
The credit goes to Marc-André Lureau <marcandre.lureau@redhat.com <mailto:marcandre.lureau@redhat.com>> who has done all the debugging and proposed fix in the bugzilla.
(thanks for the credit :)
Wouldn't it make sense to return an error if SendBuffers is used without AsyncIO then? Or just enable AsyncIO as necessary? (beware, I am not very familiar with virCommand API. I don't know what this would imply)
Yeah, I could try to make virCommand error out, but at any of those functions. Thing is, it's actually vitCommandDaemonize() that's troublemaker here. For SendBuffers to work, virCommandProcessIO() has to run and there are two places where it's called: 1) for non-daemonized processes it's called directly from virCommandRun(), 2) for daemonized processes it's called from the intermediary child. Therefore, SendBuffers() can work even without AsyncIO(). Anyway, the idea behind virCommand is that caller doesn't have to check for retval of individual virCommand* calls as they do that itself as the very first thing - virCommandHasError() - and turn themselves into NOP if there was an error earlier. This allows us to write shorter functions, e.g.: cmd = virComandNew("/usr/bin/binary"); virCommandSomething(cmd); virCommandSomethingElse(cmd); virCommandSomethingThatFails(cmd); /* no error reported here */ rc = virCommandRun(cmd, NULL); /* it's only here that the error from earlier is propagated and rc is set to -1 */ In general it's okay to swap virCommand calls, unless appending arguments (which would then be swapped too).
Also it would be nice to cover that "behaviour" in a test (even better if we could cover the swtpm setup & start handling too, although I realize this is more work!)
I'm not exactly sure what to test here. We don't have a test suite for this area of the code. I mean, we do have a test that covers virCommand and I can definitely add a test case there, but it runs only a small & dumb binary of ours. But at least we would have proper example of use somewhere. Let me post that in v2. Thanks! Michal
participants (3)
-
Marc-André Lureau -
Michal Privoznik -
Michal Prívozník