After startup, virtiofds forks itself to drop its privileges.
Kill the whole process group instead of just the parent.

https://bugzilla.redhat.com/show_bug.cgi?id=1808697

Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Andrew Jones <drjones@redhat.com>
---
 src/qemu/qemu_virtiofs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index d579ce1d33..d6159206eb 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -285,7 +285,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
 
     rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
     if (rc >= 0 && pid != (pid_t) -1)
-        virProcessKillPainfully(pid, true);
+        virProcessKillPainfully(-pid, true);
 
     if (unlink(pidfile) < 0 &&
         errno != ENOENT) {
-- 
2.25.1

On 23. 3. 2020 17:10, Ján Tomko wrote:
> After startup, virtiofds forks itself to drop its privileges.
> Kill the whole process group instead of just the parent.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1808697
> 
> Signed-off-by: Ján Tomko <jtomko@redhat.com>
> Reported-by: Andrew Jones <drjones@redhat.com>
> ---
>  src/qemu/qemu_virtiofs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> index d579ce1d33..d6159206eb 100644
> --- a/src/qemu/qemu_virtiofs.c
> +++ b/src/qemu/qemu_virtiofs.c
> @@ -285,7 +285,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
>  
>      rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
>      if (rc >= 0 && pid != (pid_t) -1)
> -        virProcessKillPainfully(pid, true);
> +        virProcessKillPainfully(-pid, true);
>  
>      if (unlink(pidfile) < 0 &&
>          errno != ENOENT) {
> 

Ah, this means that other places are affected too (e.g. qemu-pr-helper,
which shouldn't fork(), but libvirt can just use kill(-pid) to be sure).

But since I've posted a patch that will remove these lines and replace
them with a simple virPidFileForceCleanupPath(), we can do that in one
place for the benefit of others.

Michal

On a Monday in 2020, Michal Prívozník wrote:
>On 23. 3. 2020 17:10, Ján Tomko wrote:
>> After startup, virtiofds forks itself to drop its privileges.
>> Kill the whole process group instead of just the parent.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1808697
>>
>> Signed-off-by: Ján Tomko <jtomko@redhat.com>
>> Reported-by: Andrew Jones <drjones@redhat.com>
>> ---
>>  src/qemu/qemu_virtiofs.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
>> index d579ce1d33..d6159206eb 100644
>> --- a/src/qemu/qemu_virtiofs.c
>> +++ b/src/qemu/qemu_virtiofs.c
>> @@ -285,7 +285,7 @@ qemuVirtioFSStop(virQEMUDriverPtr driver G_GNUC_UNUSED,
>>
>>      rc = virPidFileReadPathIfAlive(pidfile, &pid, NULL);
>>      if (rc >= 0 && pid != (pid_t) -1)
>> -        virProcessKillPainfully(pid, true);
>> +        virProcessKillPainfully(-pid, true);
>>
>>      if (unlink(pidfile) < 0 &&
>>          errno != ENOENT) {
>>
>
>Ah, this means that other places are affected too (e.g. qemu-pr-helper,
>which shouldn't fork(), but libvirt can just use kill(-pid) to be sure).
>

I just realized that virProcessKillPainfully goes down all the way to
virProcessKill which ignores pids <= 1, so this patch is not doing what
it should.

Jano

>But since I've posted a patch that will remove these lines and replace
>them with a simple virPidFileForceCleanupPath(), we can do that in one
>place for the benefit of others.
>
>Michal
>