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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 15 Feb 2023 18:04:56 +0100
Michal Prívozník <mprivozn@redhat.com> 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 <mprivozn@redhat.com> 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.com/

*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;
 }

-- 
@@ -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