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

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