Re: [Virtio-fs] [libvirt PATCHv3 10/12] qemu: add code for handling virtiofsd

* Re: [Virtio-fs] [libvirt PATCHv3 10/12] qemu: add code for handling virtiofsd
       [not found]   ` <20200203164351.GV1922177@redhat.com>
@ 2020-02-03 18:36     ` Ján Tomko
  2020-02-03 19:03       ` Daniel P. Berrangé
  2020-02-06 13:46       ` Stefan Hajnoczi
  0 siblings, 2 replies; 4+ messages in thread
From: Ján Tomko @ 2020-02-03 18:36 UTC (permalink / raw)
  To: Daniel P. Berrangé; +Cc: libvir-list, virtio-fs

[-- Attachment #1: Type: text/plain, Size: 4197 bytes --]

[adding virtiofs list]

On Mon, Feb 03, 2020 at 04:43:51PM +0000, Daniel P. Berrangé wrote:
>On Thu, Jan 30, 2020 at 06:06:26PM +0100, Ján Tomko wrote:
>> Start virtiofsd for each <filesystem> device using it.
>>
>> Pre-create the socket for communication with QEMU and pass it
>> to virtiofsd.
>>
>> Note that virtiofsd needs to run as root.
>
>So we're not able to use  virtiofsd with the session libvirtd
>which runs completely unprivileged ?
>

Not with the version of virtiofsd currently merged in the QEMU tree.

>I can understand the need to run as root if we want to support
>chown() of files, or DAC_OVERRIDE, but I'm surprised it isn't
>possible to run virtiofsd unprivileged & simply have a reduced
>featureset where it can only create files as that one user.
>

Apart from the possibly missing features (I don't know how well
virtiofsd internals are ready for those), current version of the
daemon sets up namespaces and the seccomp sandbox.

>
>> +int
>> +qemuVirtioFSStart(virLogManagerPtr logManager,
>> +                  virQEMUDriverPtr driver,
>> +                  virDomainObjPtr vm,
>> +                  virDomainFSDefPtr fs)
>> +{
>> +    g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>> +    g_autoptr(virCommand) cmd = NULL;
>> +    g_autofree char *socket_path = NULL;
>> +    g_autofree char *pidfile = NULL;
>> +    g_autofree char *logpath = NULL;
>> +    pid_t pid = (pid_t) -1;
>> +    VIR_AUTOCLOSE fd = -1;
>> +    VIR_AUTOCLOSE logfd = -1;
>> +    int ret = -1;
>> +    int rc;
>
>> +
>> +    if (!(pidfile = qemuVirtioFSCreatePidFilename(cfg, vm->def, fs->info.alias)))
>> +        goto cleanup;
>> +
>> +    if (!(socket_path = qemuVirtioFSCreateSocketFilename(vm, fs->info.alias)))
>> +        goto cleanup;
>> +
>> +    if ((fd = qemuVirtioFSOpenChardev(driver, vm, socket_path)) < 0)
>> +        goto cleanup;
>> +
>> +    logpath = qemuVirtioFSCreateLogFilename(cfg, vm->def, fs->info.alias);
>> +
>> +    if (cfg->stdioLogD) {
>> +        if ((logfd = virLogManagerDomainOpenLogFile(logManager,
>> +                                                    "qemu",
>> +                                                    vm->def->uuid,
>> +                                                    vm->def->name,
>> +                                                    logpath,
>> +                                                    0,
>> +                                                    NULL, NULL)) < 0)
>> +            goto cleanup;
>> +    } else {
>> +        if ((logfd = open(logpath, O_WRONLY | O_CREAT | O_APPEND, S_IRUSR | S_IWUSR)) < 0) {
>> +            virReportSystemError(errno, _("failed to create logfile %s"),
>> +                                 logpath);
>> +            goto cleanup;
>> +        }
>> +        if (virSetCloseExec(logfd) < 0) {
>> +            virReportSystemError(errno, _("failed to set close-on-exec flag on %s"),
>> +                                 logpath);
>> +            goto error;
>> +        }
>> +    }
>> +
>> +    if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd)))
>> +        goto cleanup;
>> +
>> +    virCommandSetPidFile(cmd, pidfile);
>> +    virCommandSetOutputFD(cmd, &logfd);
>> +    virCommandSetErrorFD(cmd, &logfd);
>> +    virCommandNonblockingFDs(cmd);
>> +    virCommandDaemonize(cmd);
>
>We're not mandating "root" here, it is just inheriting the user that
>libvirtd runs as. So IIUC ,this will run virtofsd as non-root when
>used with session libvirtd, unless there's a check somewhere else
>that prevents this scenario ?

I'll add a check.

>
>I'm also wondering about cgroups placement in this method, and
>any use of SELinux
>

Placing it into a cgroup should be easy, AFAIK it does not need to
access any devices.

As for SELinux, I don't think there's anything to be done other than
updating the selinux-policy. Recursively relabeling the whole directory
feels intrusive.

Jano

>> +
>> +    if (qemuExtDeviceLogCommand(driver, vm, cmd, "virtiofsd") < 0)
>> +        goto cleanup;
>> +
>> +    rc = virCommandRun(cmd, NULL);
>> +    logfd = -1;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread