On Tue, Jan 25, 2011 at 03:54:12PM -0500, Laine Stump wrote: > On 01/25/2011 12:49 PM, Daniel P. Berrange wrote: > >On Tue, Jan 25, 2011 at 04:24:19AM -0500, Laine Stump wrote: > >>This patch is a partial resolution to the following bug: > >> > >> https://bugzilla.redhat.com/show_bug.cgi?id=667756 > >> > >>(to complete the fix, an updated selinux-policy package is required, > >>to add the policy that allows libvirt to set the context of a fifo, > >>which was previously not allowed). > >> > >>Explanation : When an incoming migration is over a pipe (for example, > >>if the image was compressed and is being fed through gzip, or was on a > >>root-squash nfs server, so needed to be opened by a child process > >>running as a different uid), qemu cannot read it unless the selinux > >>context label for the pipe has been set properly. > >> > >>The solution is to check the fd used as the source of the migration > >>just before passing it to qemu; if it's a fifo (implying that it's a > >>pipe), we call the newly added virSecurityManagerSetFDLabel() function > >>to set the context properly. > >>--- > >> src/qemu/qemu_driver.c | 18 ++++++++++++++++++ > >> 1 files changed, 18 insertions(+), 0 deletions(-) > >> > >>diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > >>index 34cc29f..985b062 100644 > >>--- a/src/qemu/qemu_driver.c > >>+++ b/src/qemu/qemu_driver.c > >>@@ -2667,6 +2667,24 @@ static int qemudStartVMDaemon(virConnectPtr conn, > >> vm, stdin_path)< 0) > >> goto cleanup; > >> > >>+ if (stdin_fd != -1) { > >>+ /* if there's an fd to migrate from, and it's a pipe, put the > >>+ * proper security label on it > >>+ */ > >>+ struct stat stdin_sb; > >>+ > >>+ DEBUG0("setting security label on pipe used for migration"); > >>+ > >>+ if (fstat(stdin_fd,&stdin_sb)< 0) { > >>+ virReportSystemError(errno, > >>+ _("cannot stat fd %d"), stdin_fd); > >>+ goto cleanup; > >>+ } > >>+ if (S_ISFIFO(stdin_sb.st_mode)&& > >>+ virSecurityManagerSetFDLabel(driver->securityManager, vm, stdin_fd)< 0) > >>+ goto cleanup; > >>+ } > >This feels like the wrong place to put this call. The callers > >of qemudStartVMDaemon() which opened 'stdin_fd' in the first > >place will already know if it is a pipe or not. If we put > >the virSecurityManagerSetFDLabel call in the appropriate > >callers, then the fstat() complexity is avoided. > > That was my first intent too. However, in the case of an image on > root-squashed NFS, the knowledge about whether to directly open, or > open via a pipe to a child process, is made in qemudOpenAsUID(), > which doesn't have access to the domainObj, so cannot call the > security driver. > > In a broader view, qemudOpenAsUID() is really a potentially general > purpose function that could be used outside of this context some > day, but gumming it up with application-specific things like a > domainObj would lock it into being specific to domain-related > functions. > > More specifically (and importantly), the domainObj hasn't even been > constructed until after qemudOpenAsUID() is finished and returned, > since it's going to be created by the caller from the header of the > file that qemudOpenAsUID() has just opened. So by the time we have > the domainObj, we no longer have the knowledge that the fd is > actually the read side of a pipe - we would still have to call fstat > (or clutter up the calling sequence to pass back an "is_fifo" bool > or something, which sounds even less right). > > Compared to that, putting the call to SetFDLabel() in a single > place, qualified by fstat() to see if the fd was a fifo, seemed like > a much less intrusive change to the code. (The other instance of a > pipe being created (for compression) is less problematic, as > everything we need is already there. However, since we're already > doing the fstat() for the root-squash case, and since doing two > FDSetLabels() would be superfluous (in the case of a compressed > image stored on a root-squashed share), I figured we might as well > have a single call in a common place (which, by the way, is > strategically located as late as possible, so that any future > additions of pipes will automatically be caught and accounted for).) > > However, If anyone has a suggestion for dealing with the chicken-egg > problem of domainObj vs fifo that is less ugly, please speak up, and > I'll be happy to implement it! :-) Ok, I see what you mean here. ACK to the original patch Daniel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list