On 2/14/23 6:51 AM, Michal Privoznik wrote:
...
  This reverts commit
0c4e716835eaf2a575bd063fde074c0fc7c4e4d4.
 
 This patch was pushed by my mistake. Even though it got ACKed on
 the list, I've raised couple of issues with it. They will be
 fixed in next commits. 
I admire your optimism :-)

...
  
 Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com> 
Reviewed-by: Laine Stump <laine(a)redhat.com>

...
  ---
   src/qemu/qemu_passt.c | 9 +++++----
   1 file changed, 5 insertions(+), 4 deletions(-)
 
 diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c
 index f640a69c00..0f09bf3db8 100644
 --- a/src/qemu/qemu_passt.c
 +++ b/src/qemu/qemu_passt.c
 @@ -141,23 +141,24 @@ qemuPasstStart(virDomainObj *vm,
       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;
 +    VIR_AUTOCLOSE errfd = -1;
   
       cmd = virCommandNew(PASST);
   
       virCommandClearCaps(cmd);
 -    virCommandSetErrorBuffer(cmd, &errbuf);
 +    virCommandSetPidFile(cmd, pidfile);
 +    virCommandSetErrorFD(cmd, &errfd);
 +    virCommandDaemonize(cmd);
   
       virCommandAddArgList(cmd,
                            "--one-off",
                            "--socket", passtSocketName,
                            "--mac-addr", virMacAddrFormat(&net->mac,
macaddr),
 -                         "--pid", pidfile,
                            NULL);
   
       if (net->mtu) {
 @@ -263,7 +264,7 @@ qemuPasstStart(virDomainObj *vm,
   
       if (cmdret < 0 || exitstatus != 0) {
           virReportError(VIR_ERR_INTERNAL_ERROR,
 -                       _("Could not start 'passt': %s"), errbuf);
 +                       _("Could not start 'passt'. exitstatus: %d"),
exitstatus);
           goto error;
       }
    

On 2/14/23 6:51 AM, Michal Privoznik wrote:
...
  We can have external helper processes running for
domain
 <interface/> too (e.g. slirp or passt). But this is not reflected
 in qemuExtDevicesHasDevice() which simply ignores these. 
The slirp-helper patches missed adding the check in this (oddly-named) 
function (even while adding in the correct hunk to 
qemuExtDevicesSetupCroup()) probably because it wasn't really obvious 
without reading/interpreting/understanding all the code in two separate 
files that it was needed; my passt patches missed adding the check in 
this function because I was following the pattern of what was done for 
slirp, and slirp hadn't touched this function (nor had it touched the 
function that calls both of these functions, 
qemuSetupCgroupForExtDevices(), which is in another file). It's 
reasonable to think that some future person may also not notice 
qemuExtDevicesHasDevice(), and believe that they only need to modify 
qemuExtDevicesSetupCgroup().

Anyway, my point is that I think this could be avoided by adding a 
comment in qemuExtDevicesSetupCgroup() that points out it is only called 
if qemuExtDevicesHasDevice() returns true, and so any addition to 
qemuExtDevicesSetupCgroup() should have a corresponding addition to 
qemuExtDevicesHasDevice(). It's too late at night / early in the morning 
for my brain to compose a concise sentence to this effect, but it would 
make me happy if you added one before pushing.

...
  
 Signed-off-by: Michal Privoznik &lt;mprivozn(a)redhat.com&gt; 
Reviewed-by: Laine Stump &lt;laine(a)redhat.com&gt;

...
  ---
   src/qemu/qemu_extdevice.c | 11 +++++++++++
   1 file changed, 11 insertions(+)
 
 diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
 index fdefe59215..47e97f3565 100644
 --- a/src/qemu/qemu_extdevice.c
 +++ b/src/qemu/qemu_extdevice.c
 @@ -296,6 +296,17 @@ qemuExtDevicesHasDevice(virDomainDef *def)
               return true;
       }
   
 +    for (i = 0; i < def->nnets; i++) {
 +        virDomainNetDef *net = def->nets[i];
 +
 +        if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp)
 +            return true;
 +
 +        if (net->type == VIR_DOMAIN_NET_TYPE_USER &&
 +            net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST)
 +            return true;
 +    }
 +
       for (i = 0; i < def->ntpms; i++) {
           if (def->tpms[i]->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)
               return true; 

On 2/14/23 14:02, 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. 
That's not what's happening here. In fact, the opposite. We let libvirt
create the file, lock and write it. Then the file's FD is leaked into
passt process. This way, we can learn whether the PID file is still
valid: if we just try to lock it and:

a) fail, then the file is still locked, i.e. passt is still running,
b) succeed, then the file is no longer locket by passt, i.e. it's no
longer running.

Thing is, passt doesn't even know about the FD (unless it starts closing
FDs "randomly", e.g. via closefrom(3) or an alternative). This is the
reason we can't use passt's --pid, because it writes the PID file
without any locking dance. Therefore, libvirt can't know whether the PID
file is still valid. If it did just read the file and killed PID from
there it might kill a random process that was unfortunate to be assigned
the same PID.

BTW, there are two other issues with PID files in passt:
1) no locking means, that the file is very easily overwritten:

 $ passt --pid /tmp/passt.pid 2>/dev/null & \
   passt --pid /tmp/passt.pid 2>/dev/null; \
   pgrep passt ; cat /tmp/passt.pid
[1] 21029
[1]+  Done                    passt --pid /tmp/passt.pid 2> /dev/null
21034
21035
21035

2) with --daemonize, only the PID of the parent process is written which
is pretty much useless, as the parent quits shortly after clone().

Now, libvirt's virPidFile* machinery (src/util/virpidfile.c) ensures
that case 1) won't happen (yes, we have internal APIs that read pid
files unlocked, but those are not used from this code we're talking about).

And this patch tries to fix the case 2) by instructing passt to not
clone(). If it means that it can't use PID namespace, then so be it.
It's better to not let processes behind.

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

 Fortunately, passt has '--foreground' argument, which causes it
 to undergo the same security measures but without forking off the
 child. 
 
 They're not the same -- unfortunately they can't be, because, on Linux,
 you can't change the PID of an existing process, so there's no way to
 enter a new PID namespace without clone().
 
 If passt remains in the same PID namespace, it's still able to see PIDs
 of other processes, which is not desirable from a security perspective. 
I understand that, but it's already confined in plenty other ways. It's
way worse to leave a process running than being able to see other PIDs.

...
  
 Again from a security perspective, this is probably a small impact, so
 I guess it's fine if there's no other way around it. But I see a lot of
 ways around it...
  
Is there? If we have a helper process that fork()-s then the only way we
can 'track' its PIDs (and kill them) is by placing them into a CGroup.
Until we do that we have to trust helper processes to behave properly.
But that's something I'd like to avoid.

Michal

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

...
 >
> Fortunately, passt has '--foreground' argument, which causes it
> to undergo the same security measures but without forking off the
> child. 
But if we do --foreground in combination with calling 
virCommandDaemonize(), then won't we still have the problem that libvirt 
won't know whether or not passt has failed to start (not unless we want 
to put in a bunch of gorp to continue grabbing stderr while watching to 
see if the passt process has exited, etc. It's going to take some 
convincing for me to think we should run passt with --foreground rather 
than letting it daemonize itself.

...
  
 They're not the same -- unfortunately they can't be, because, on Linux,
 you can't change the PID of an existing process, so there's no way to
 enter a new PID namespace without clone().
 
 If passt remains in the same PID namespace, it's still able to see PIDs
 of other processes, which is not desirable from a security perspective.
 
 Again from a security perspective, this is probably a small impact, so
 I guess it's fine if there's no other way around it. But I see a lot of
 ways around it...
  

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(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).
 
This last item is fixed by patches I have pending to passt itself.

On 2/15/23 19:30, Stefano Brivio wrote:
...
  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.c…
 
Thanks, this looks exactly like what we need. So for now I can just pass
--stderr if there's no --log-file, to deal with those "releases" that
don't have those patches merged yet.

...
  
 *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.
 
 
This effectively disables placing passt into the CGroup set up for
emulator thread. And I don't think we want that. Firstly, it makes
statistics gathering report incorrect values. Secondly, these helper
processes are "implementation detail" - I mean, users don't really care
(from accounting POV) whether a task runs in emulator thread inside of
QEMU or in a separate process. It's still an emulation and as such
should be accounted for. And also, on NUMA machines we definitely want
to place passt as close to the emulator as possible (i.e. if emulator
thread is pinned than helper processes should be pinned too).

Furthermore, it enhances security, because libvirt sets up devices
controller in such way, that only devices from domain XML are allowed
and everything else is forbidden.

I could go on with other controllers but I believe you get the picture.
Now true, for qemu:///session we don't set any CGroups as we lack the
permissions to do so [1], and this is probably the target audience for
this feature anyway, but for qemu:///system (when running
libvirtd/virtqemud as root) we do set up CGroups and MUST place helper
processes into them. I mean, if we are concerned about security (just
look at the discussion about --foreground), then CGroups are definitely
step in the right direction.

1: and even this might change in the future as there are some plans to
let a privileged component create the CGroup for us (e.g. systemd).

Michal

On Thu, 16 Feb 2023 09:52:27 +0100
Michal Prívozník <mprivozn(a)redhat.com> wrote:

...
  On 2/15/23 19:30, Stefano Brivio wrote:
  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.c…   

 Thanks, this looks exactly like what we need. So for now I can just pass
 --stderr if there's no --log-file, to deal with those "releases" that
 don't have those patches merged yet. 
I wouldn't even bother with that, the user base (especially with
libvirt) is small enough that we can be quite confident that 100% of the
users will upgrade as soon as a new release (most likely coming this
week) includes them. I'd suggest to keep it simple, because you really
can.

Those are actual releases. Their naming just doesn't follow "semantic
versioning".

...
 
  *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. 
 
 This effectively disables placing passt into the CGroup set up for
 emulator thread. And I don't think we want that. Firstly, it makes
 statistics gathering report incorrect values. Secondly, these helper
 processes are "implementation detail" - I mean, users don't really care
 (from accounting POV) whether a task runs in emulator thread inside of
 QEMU or in a separate process. It's still an emulation and as such
 should be accounted for. And also, on NUMA machines we definitely want
 to place passt as close to the emulator as possible (i.e. if emulator
 thread is pinned than helper processes should be pinned too). 
Yes, definitely, I see now -- I thought, earlier, that cgroups were just
used to handle lifecycles at the moment.

...
  [...] 
-- 
Stefano