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

On 23. 3. 2020 17:10, Ján Tomko wrote:

As Dave pointed out, someone creative might name a domain to make its logfile conflict with a logfile of another domain's virtiofsd log.

Signed-off-by: Ján Tomko <jtomko@redhat.com> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> --- src/qemu/qemu_virtiofs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index aaa25bcbb8..575b47fd74 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg, { g_autofree char *name = NULL;

- name = g_strdup_printf("%s-%s", def->name, alias); + name = g_strdup_printf("%s-%s.log.fs", def->name, alias);

- return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log"); + return virFileBuildPath(cfg->logDir, name, NULL); }

Why not go with virDomainDefGetShortName() then? Using def->name to name a file is dangerous anyways. Oh, is it because we want to keep the same log name across domain cold reboots? Michal

On 23. 3. 2020 17:56, Ján Tomko wrote:

On a Monday in 2020, Michal Prívozník wrote:

...

On 23. 3. 2020 17:10, Ján Tomko wrote:

...

As Dave pointed out, someone creative might name a domain to make its logfile conflict with a logfile of another domain's virtiofsd log.

Signed-off-by: Ján Tomko <jtomko@redhat.com> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> ---  src/qemu/qemu_virtiofs.c | 4 ++--  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index aaa25bcbb8..575b47fd74 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg,  {      g_autofree char *name = NULL;

-    name = g_strdup_printf("%s-%s", def->name, alias); +    name = g_strdup_printf("%s-%s.log.fs", def->name, alias);

-    return virFileBuildPath(cfg->logDir, name, "-virtiofsd.log"); +    return virFileBuildPath(cfg->logDir, name, NULL);  }

Why not go with virDomainDefGetShortName() then? Using def->name to name a file is dangerous anyways.

Other than path name limits (which are way higher than unix socket limits), it should not be dangerous.

...

Oh, is it because we want to keep the same log name across domain cold reboots?

That too.

Okay, for whole series: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal

On a Monday in 2020, Daniel P. Berrangé wrote:

On Mon, Mar 23, 2020 at 05:10:37PM +0100, Ján Tomko wrote:

...

As Dave pointed out, someone creative might name a domain to make its logfile conflict with a logfile of another domain's virtiofsd log.

Signed-off-by: Ján Tomko <jtomko@redhat.com> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> --- src/qemu/qemu_virtiofs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c index aaa25bcbb8..575b47fd74 100644 --- a/src/qemu/qemu_virtiofs.c +++ b/src/qemu/qemu_virtiofs.c @@ -67,9 +67,9 @@ qemuVirtioFSCreateLogFilename(virQEMUDriverConfigPtr cfg, { g_autofree char *name = NULL;

- name = g_strdup_printf("%s-%s", def->name, alias); + name = g_strdup_printf("%s-%s.log.fs", def->name, alias);

Having a logfile that doesn't end in ".log" is pretty gross IMHO

Agreed.

and will mean it isn't caught by logrotate matching *.log

I did not realize that.

Can't we deal with the clash in another way that preserves .log as a suffix

Not in the same directory, but I can create a separate directory, something like: /var/log/libvirt/qemu/vhost-user/%{name}-fs-%{alias}.log /var/log/libvirt/qemu/devices/%{name}-fs-%{alias}.log /var/log/libvirt/qemu/fs/%{name}-%{alias}.log Or we can just pretend this is fine O:-) Jano

Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|