12:30 p.m.
On Wed, 15 Feb 2023 18:04:56 +0100
Michal Prívozník <mprivozn(a)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(a)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...
*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