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

[Laine Stump <laine@redhat.com>]

On 2/15/23 12:04 PM, Michal Prívozník 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@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).

This last item is fixed by patches I have pending to passt itself.