From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id ACCD65A005E for ; Wed, 15 Feb 2023 19:32:06 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676485925; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=85xadupbBYJme5In8yJ65lfE8Ek07NTyLkLVa3ARUbQ=; b=VnqcjBUm5qMHCcD7rrCcQzBdaIwLRtAHgt+GcdzjWs5PrWhP37Ws+T5xcmyB2jluX/w42X iETCo1ajKXWZ75oz6SKaG45Ey4h8CipEDasUrqsmb1eAOT8HTNCS7zBrj76xkVrEfVnKa0 T57zV35Sm1apotOUofjraYsJ11+vsnM= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-148-ZNNdVoRZOwKG9wObGUP42g-1; Wed, 15 Feb 2023 13:30:25 -0500 X-MC-Unique: ZNNdVoRZOwKG9wObGUP42g-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9F455857D07 for ; Wed, 15 Feb 2023 18:30:25 +0000 (UTC) Received: from maya.cloud.tilaa.com (unknown [10.33.32.3]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 550602166B30; Wed, 15 Feb 2023 18:30:25 +0000 (UTC) Date: Wed, 15 Feb 2023 19:30:20 +0100 From: Stefano Brivio To: Michal =?UTF-8?B?UHLDrXZvem7DrWs=?= Subject: Re: [PATCH 4/4] qemu_passt: Don't let passt fork off Message-ID: <20230215193020.4af13f54@elisabeth> In-Reply-To: <7cbc3713-9d51-2950-2a3c-ae90928b83b6@redhat.com> References: <5abfc412e4692a38e980c8dc600e1bfbd03ddcfd.1676374699.git.mprivozn@redhat.com> <20230214140253.49bbc13a@elisabeth> <90dbb5f3-7b3f-893c-ca32-a7653eb486c6@redhat.com> <7cbc3713-9d51-2950-2a3c-ae90928b83b6@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 4N4JYVKTW3RAGOAGJQLQVRLZGZGLMM3D X-Message-ID-Hash: 4N4JYVKTW3RAGOAGJQLQVRLZGZGLMM3D X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Laine Stump , Libvirt , passt-dev@passt.top X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 15 Feb 2023 18:04:56 +0100 Michal Pr=C3=ADvozn=C3=ADk wrote: > On 2/15/23 08:50, Laine Stump wrote: > > On 2/14/23 8:02 AM, Stefano Brivio wrote: =20 > >> On Tue, 14 Feb 2023 12:51:22 +0100 > >> Michal Privoznik wrote: > >> =20 > >>> 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 > >>> =C2=A0=C2=A0=C2=A0 closed (and thus our virPidFile*() helpers see unl= ocked PID > >>> =C2=A0=C2=A0=C2=A0 file, which makes them think the process is gone),= =20 > >> > >> 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. =20 > > =20 > >> =20 > >>> 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. =20 > >=20 > > I think that (incorrect PID in the pidfile) is=C2=A0 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. > >=20 > > 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). =20 >=20 > 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, =20 if (slirp && qemuSlirpSetupCgroup(slirp, cgroup) < 0) return -1; - - if (net->type =3D=3D VIR_DOMAIN_NET_TYPE_USER && - net->backend.type =3D=3D VIR_DOMAIN_NET_BACKEND_PASST && - qemuPasstSetupCgroup(vm, net, cgroup) < 0) { - return -1; - } } =20 for (i =3D 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" =20 #define VIR_FROM_THIS VIR_FROM_NONE =20 VIR_LOG_INIT("qemu.passt"); =20 =20 -static char * -qemuPasstCreatePidFilename(virDomainObj *vm, - virDomainNetDef *net) -{ - qemuDomainObjPrivate *priv =3D vm->privateData; - virQEMUDriver *driver =3D priv->driver; - g_autofree char *shortName =3D virDomainDefGetShortName(vm->def); - g_autoptr(virQEMUDriverConfig) cfg =3D virQEMUDriverGetConfig(driver); - g_autofree char *name =3D NULL; - - name =3D 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, } =20 =20 -static int -qemuPasstGetPid(virDomainObj *vm, - virDomainNetDef *net, - pid_t *pid) -{ - g_autofree char *pidfile =3D 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 =3D 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 =3D qemuPasstCreateSocketPath(vm, net= ); + struct sockaddr_un addr =3D { .sun_family =3D AF_UNIX }; + int fd; + + fd =3D socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) { + virReportError(errno, + "%s", _("Unable to open socket to connect to passt"= )); + return; + } =20 -int -qemuPasstSetupCgroup(virDomainObj *vm, - virDomainNetDef *net, - virCgroup *cgroup) -{ - pid_t pid =3D (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; + } =20 - if (qemuPasstGetPid(vm, net, &pid) < 0 || pid <=3D 0) - return -1; + if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { + if (errno !=3D ECONNREFUSED && errno !=3D ENOENT) + virReportError(errno, + "%s", _("Unable to connect to passt to terminate it= ")); + } =20 - return virCgroupAddProcess(cgroup, pid); + out: + VIR_FORCE_CLOSE(fd); } =20 =20 @@ -140,13 +115,9 @@ qemuPasstStart(virDomainObj *vm, virQEMUDriver *driver =3D priv->driver; g_autofree char *passtSocketName =3D qemuPasstCreateSocketPath(vm, net= ); g_autoptr(virCommand) cmd =3D NULL; - g_autofree char *pidfile =3D qemuPasstCreatePidFilename(vm, net); g_autofree char *errbuf =3D NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; - pid_t pid =3D (pid_t) -1; - int exitstatus =3D 0; - int cmdret =3D 0; =20 cmd =3D virCommandNew(PASST); =20 @@ -157,7 +128,6 @@ qemuPasstStart(virDomainObj *vm, "--one-off", "--socket", passtSocketName, "--mac-addr", virMacAddrFormat(&net->mac, macaddr= ), - "--pid", pidfile, NULL); =20 if (net->mtu) { @@ -254,26 +224,15 @@ qemuPasstStart(virDomainObj *vm, virCommandAddArg(cmd, virBufferCurrentContent(&buf)); } =20 - if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0) return -1; =20 - if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdr= et) < 0) - goto error; - - if (cmdret < 0 || exitstatus !=3D 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; } =20 return 0; - - error: - ignore_value(virPidFileReadPathIfLocked(pidfile, &pid)); - if (pid !=3D -1) - virProcessKillPainfully(pid, true); - unlink(pidfile); - - return -1; } --=20 Stefano