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

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).