On Mon, Oct 01, 2018 at 01:14:34PM +0200, Ján Tomko wrote: > On Mon, Oct 01, 2018 at 10:22:59AM +0200, Erik Skultety wrote: > > On Thu, Sep 27, 2018 at 05:02:13PM +0200, Ján Tomko wrote: > > > We switched to opening mode='bind' sockets ourselves: > > > commit 30fb2276d88b275dc2aad6ddd28c100d944b59a5 > > > qemu: support passing pre-opened UNIX socket listen FD > > > in v4.5.0-rc1~251 > > > > > > Then fixed qemuBuildChrChardevStr to change libvirtd's label > > > while creating the socket: > > > commit b0c6300fc42bbc3e5eb0b236392f7344581c5810 > > > qemu: ensure FDs passed to QEMU for chardevs have correct SELinux labels > > > v4.5.0-rc1~52 > > > > > > Also add labeling of these sockets to the DAC driver. > > > Instead of trying to figure out which one was created by libvirt, > > > label it if it exists. > > > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1633389 > > > > > > Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> > > > --- > > > src/security/security_dac.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > > > index 62442745dd..da4a6c72fe 100644 > > > --- a/src/security/security_dac.c > > > +++ b/src/security/security_dac.c > > > @@ -1308,7 +1308,12 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, > > > break; > > > > > > case VIR_DOMAIN_CHR_TYPE_UNIX: > > > - if (!dev_source->data.nix.listen) { > > > + if (!dev_source->data.nix.listen || > > > + (dev_source->data.nix.path && > > > + virFileExists(dev_source->data.nix.path))) { > > > + /* Also label mode='bind' sockets if they exist, > > > + * e.g. because they were created by libvirt > > > + * and passed via FD */ > > > if (virSecurityDACSetOwnership(mgr, NULL, > > > dev_source->data.nix.path, > > > user, group) < 0) > > > > So we're labeling path even if nix.listen == false, shouldn't we check for the > > file's existence too? Or have we already done it and I missed this fact? > > For mode='connect', the path not existing is fatal, so we can safely > call virSecurityDACSetOwnership and let it fail. > > For mode='bind', the path not existing is valid for QEMUs which do not > support FD passing for chardevs. If QEMU supports FD passing, the > path should exist because libvirt created it earlier. I could not think > of a nicer idea on how to pass down the logic that decides whether > libvirt pre-creates it in qemuBuildChrChardevStr (which, of course, is > an odd place to be creating sockets), so I opted to use the file's > existence as a witness of libvirt pre-creating it. Which is what I tried > to say by this commit message snippet: > > > Instead of trying to figure out which one was created by libvirt, > > > label it if it exists. > > Shall I reword the message? E.g.: > Instead of duplicating the logic which decides whether libvirt should > pre-create the socket, assume an existing path means it was created by > libvirt. s/means/meaning that ...otherwise sounds meaningful... > > > > Basically what I'm aiming at is that nix.path will always be set at this point, > > Yes, the dev_source->data.nix.path condition might be superfluous. I was > being overly cautious. > > As evidenced by: > commit 614193fac67445a7e92bf620ffef726ed1bd6f07 > conf: Fix check for chardev source path > An empty path should be caught by our validation. > > > either explicitly by the user (most cases) or it would have been generated by > > us if the target type was either VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN or > > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. > > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN | > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN is still > VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_XEN. ;) > > > I'm just simply wondering whether the > > condition cannot be further simplified to: > > > > if (virFileExists(foo) && virSecurityDACSetOwnership(...) < 0) > > This introduces a change in behavior for mode='connect' - the DAC driver > will no longer report an error for a non-existent path. > I have not checked whether it's checked anywhere else (possibly the > SELinux driver), but it is a legitimate error and I don't see the need > to skip it. > > Moreover, this hides the reason for using virFileExists in the first > place - it's only needed for type='listen' sockets. Fair enough, let's got with your proposed change then (wait for after the release though): Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx> > > Hope that answers your questions. > Looking forward to hearing from you. > > Jano > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
