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