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

On Wed, 15 Feb 2023 18:04:56 +0100 Michal Prívozník <mprivozn(a)redhat.com&gt; 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&gt; 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.