Re: [PATCH 4/4] qemu_passt: Don't let passt fork off

15 Feb 2023

On Wed, 15 Feb 2023 18:04:56 +0100
Michal Prívozník  wrote:
...
On 2/15/23 08:50, Laine Stump wrote:
...
On 2/14/23 8:02 AM, Stefano Brivio wrote:
...
On Tue, 14 Feb 2023 12:51:22 +0100
Michal Privoznik  wrote:
...
When passt starts it tries to do some security measures to
restrict itself. For instance, it creates its own namespaces,
umounts basically everything, drops capabilities, forks off to
further restrict itself (the child is where all interesting work
takes place now). This is sound, except it's causing two
problems:
1) the PID file FD, which we leak into the passt process, gets
    closed (and thus our virPidFile*() helpers see unlocked PID
    file, which makes them think the process is gone),
I didn't realise this was the case, but giving passt write (unless I'm
missing something) access to a file created by libvirtd doesn't look
desirable to me.
...
...
2) the PID file no longer reflects true PID of the process.
Worse, the child calls setsid() so we can't even kill the whole
process group. I mean, we can but it won't be any good.
I think that (incorrect PID in the pidfile) is  happening because Michal
is using the original version of my patches that were pushed - I had
mimicked the behavior of slirp, where libvirt deamonizes the new
process. If that process then daemonizes itself, we have some sort of
"double daemon"; libvirt has saved off the pid of what it thinks is
going to be the final process, but then that process further forks and
exits from the process whose pid libvirt saved. But because passt was
cleaning up after itself I hadn't noticed the discrepancy in pids when
testing.
Without going into all the details of the pidfile and locking and etc, I
just want to say that if we can fork/exec dnsmasq and let it daemonize
itself and create its own pidfile, then certainly we can do the same
thing for passt. (and if there's a fundamental problem, then it's a
fundamental problem for dnsmasq as well).
Alright. I think I have a solution that would please everybody involved.
I'll post it tomorrow though. I need to test it thoroughly. We would be
able to get passt's PID (which is needed not only for killing it, but
also for CGroup placement), NOT use --foreground and still pass errors
from it to users (that is unless logfile was specified, because
unfortunately, --log-file and --stderr are mutually exclusive).
That doesn't need to be the case (--log-file and --stderr being
mutually exclusive)... if you have a use case for it, let's change that
in passt. I just wanted to keep it simple for users ("give a log file,
and be sure it won't spam").

Also mind that Laine's series:
  https://archives.passt.top/passt-dev/20230215082437.110151-1-laine@redhat.co...

*should* already cover all the cases where libvirt is interested in
relaying "early" errors back to the user.

By the way, the one below is pretty much the patch I would have proposed
for libvirt. I prepared it earlier today and didn't have a chance to
test it yet, it's compile-tested only, and doesn't take cgroups into
account (which, it seems, is needed no matter the lifecycle).

So I'm sharing it here as reference (that's how simple I wanted it to
be -- minus cgroups), or if it's convenient for you to copy and paste
something.

---

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index fdefe59215..23d25c134a 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -337,12 +337,6 @@ qemuExtDevicesSetupCgroup(virQEMUDriver *driver,
 
         if (slirp && qemuSlirpSetupCgroup(slirp, cgroup) < 0)
             return -1;
-
-        if (net->type == VIR_DOMAIN_NET_TYPE_USER &&
-            net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST &&
-            qemuPasstSetupCgroup(vm, net, cgroup) < 0) {
-            return -1;
-        }
     }
 
     for (i = 0; i < def->ntpms; i++) {
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
index f640a69c00..2327b3e25e 100644
--- a/src/qemu/qemu_passt.c
+++ b/src/qemu/qemu_passt.c
@@ -28,29 +28,12 @@
 #include "virerror.h"
 #include "virjson.h"
 #include "virlog.h"
-#include "virpidfile.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 VIR_LOG_INIT("qemu.passt");
 
 
-static char *
-qemuPasstCreatePidFilename(virDomainObj *vm,
-                           virDomainNetDef *net)
-{
-    qemuDomainObjPrivate *priv = vm->privateData;
-    virQEMUDriver *driver = priv->driver;
-    g_autofree char *shortName = virDomainDefGetShortName(vm->def);
-    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
-    g_autofree char *name = NULL;
-
-    name = g_strdup_printf("%s-%s-passt", shortName, net->info.alias);
-
-    return virPidFileBuildPath(cfg->passtStateDir, name);
-}
-
-
 static char *
 qemuPasstCreateSocketPath(virDomainObj *vm,
                           virDomainNetDef *net)
@@ -65,17 +48,6 @@ qemuPasstCreateSocketPath(virDomainObj *vm,
 }
 
 
-static int
-qemuPasstGetPid(virDomainObj *vm,
-                virDomainNetDef *net,
-                pid_t *pid)
-{
-    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
-
-    return virPidFileReadPathIfLocked(pidfile, pid);
-}
-
-
 int
 qemuPasstAddNetProps(virDomainObj *vm,
                      virDomainNetDef *net,
@@ -106,29 +78,32 @@ void
 qemuPasstStop(virDomainObj *vm,
               virDomainNetDef *net)
 {
-    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
-    virErrorPtr orig_err;
-
-    virErrorPreserveLast(&orig_err);
-
-    if (virPidFileForceCleanupPath(pidfile) < 0)
-        VIR_WARN("Unable to kill passt process");
-
-    virErrorRestore(&orig_err);
-}
-
+    g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
+    struct sockaddr_un addr = { .sun_family = AF_UNIX };
+    int fd;
+
+    fd = socket(AF_UNIX, SOCK_STREAM, 0);
+    if (fd < 0) {
+        virReportError(errno,
+                       "%s", _("Unable to open socket to connect to passt"));
+        return;
+    }
 
-int
-qemuPasstSetupCgroup(virDomainObj *vm,
-                     virDomainNetDef *net,
-                     virCgroup *cgroup)
-{
-    pid_t pid = (pid_t) -1;
+    if (virStrcpyStatic(addr.sun_path, passtSocketName) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Socket path %s too big for destination"),
+                       passtSocketName);
+        goto out;
+    }
 
-    if (qemuPasstGetPid(vm, net, &pid) < 0 || pid <= 0)
-        return -1;
+    if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+        if (errno != ECONNREFUSED && errno != ENOENT)
+            virReportError(errno,
+                       "%s", _("Unable to connect to passt to terminate it"));
+    }
 
-    return virCgroupAddProcess(cgroup, pid);
+ out:
+    VIR_FORCE_CLOSE(fd);
 }
 
 
@@ -140,13 +115,9 @@ qemuPasstStart(virDomainObj *vm,
     virQEMUDriver *driver = priv->driver;
     g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net);
     g_autoptr(virCommand) cmd = NULL;
-    g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net);
     g_autofree char *errbuf = NULL;
     char macaddr[VIR_MAC_STRING_BUFLEN];
     size_t i;
-    pid_t pid = (pid_t) -1;
-    int exitstatus = 0;
-    int cmdret = 0;
 
     cmd = virCommandNew(PASST);
 
@@ -157,7 +128,6 @@ qemuPasstStart(virDomainObj *vm,
                          "--one-off",
                          "--socket", passtSocketName,
                          "--mac-addr", virMacAddrFormat(&net->mac, macaddr),
-                         "--pid", pidfile,
                          NULL);
 
     if (net->mtu) {
@@ -254,26 +224,15 @@ qemuPasstStart(virDomainObj *vm,
         virCommandAddArg(cmd, virBufferCurrentContent(&buf));
     }
 
-
     if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0)
         return -1;
 
-    if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0)
-        goto error;
-
-    if (cmdret < 0 || exitstatus != 0) {
+    /* passt forks once it's ready, terminates on connection closure */
+    if (virCommandRun(cmd, NULL) < 0) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
                        _("Could not start 'passt': %s"), errbuf);
-        goto error;
+        return -1;
     }
 
     return 0;
-
- error:
-    ignore_value(virPidFileReadPathIfLocked(pidfile, &pid));
-    if (pid != -1)
-        virProcessKillPainfully(pid, true);
-    unlink(pidfile);
-
-    return -1;
 }

-- 
Stefano

    