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