On Fri, Jun 25, 2010 at 01:22:14PM -0400, Laine Stump wrote:

Also restore the label to its original value after qemu is finished with the file.

Prior to this patch, qemu domain restore did not function properly if selinux was set to enforce. --- src/qemu/qemu_driver.c | 6 +++++- src/security/security_selinux.c | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f4e082..9140b50 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6208,7 +6208,6 @@ error: return -1; }

-/* TODO: check seclabel restore */ static int ATTRIBUTE_NONNULL(6) qemudDomainSaveImageStartVM(virConnectPtr conn, struct qemud_driver *driver, @@ -6320,6 +6319,11 @@ qemudDomainSaveImageStartVM(virConnectPtr conn, ret = 0;

out: + if (driver->securityDriver && + driver->securityDriver->domainRestoreSavedStateLabel && + driver->securityDriver->domainRestoreSavedStateLabel(vm, path) == -1) + VIR_WARN("failed to restore save state label on %s", path); + return ret; }

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 2b43f2d..a16ede9 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -859,7 +859,7 @@ SELinuxClearSecuritySocketLabel(virSecurityDriverPtr drv, }

static int -SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_UNUSED) +SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; @@ -890,6 +890,10 @@ SELinuxSetSecurityAllLabel(virDomainObjPtr vm, const char *stdin_path ATTRIBUTE_ SELinuxSetFilecon(vm->def->os.initrd, default_content_context) < 0) return -1;

+ if (stdin_path && + SELinuxSetFilecon(stdin_path, default_content_context) < 0) + return -1; + return 0; }

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Fri, Jun 25, 2010 at 01:22:15PM -0400, Laine Stump wrote:

virStorageFileIsSharedFS would previously only work if the entire path in question was stat'able by the uid of the libvirtd process. This patch changes it to crawl backwards up the path retrying the statfs call until it gets to a partial path that *can* be stat'ed.

This is necessary to use the function to learn the fstype for files stored as a different user (and readable only by that user) on a root-squashed remote filesystem. --- src/util/storage_file.c | 37 ++++++++++++++++++++++++++++++++++++- 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/src/util/storage_file.c b/src/util/storage_file.c index 6cc8d5f..0adea40 100644 --- a/src/util/storage_file.c +++ b/src/util/storage_file.c @@ -434,9 +434,44 @@ virStorageFileGetMetadata(const char *path,

int virStorageFileIsSharedFS(const char *path) { + char *dirpath, *p; struct statfs sb; + int statfs_ret;

- if (statfs(path, &sb) < 0) { + if ((dirpath = strdup(path)) == NULL) { + virReportOOMError(); + return -1; + } + + do { + + /* Try less and less of the path until we get to a + * directory we can stat. Even if we don't have 'x' + * permission on any directory in the path on the NFS + * server (assuming it's NFS), we will be able to stat the + * mount point, and that will properly tell us if the + * fstype is NFS. + */ + + if ((p = strrchr(dirpath, '/')) == NULL) { + virReportSystemError(EINVAL, + _("Invalid relative path '%s'"), path); + VIR_FREE(dirpath); + return -1; + } + + if (p == dirpath) + *(p+1) = '\0'; + else + *p = '\0'; + + statfs_ret = statfs(dirpath, &sb); + + } while ((statfs_ret < 0) && (p != dirpath)); + + VIR_FREE(dirpath); + + if (statfs_ret < 0) { virReportSystemError(errno, _("cannot determine filesystem for '%s'"), path);

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Fri, Jun 25, 2010 at 01:22:16PM -0400, Laine Stump wrote:

Previously, this function had it's own bit of code performing the same function. Since there's now an equivalent utility function, let's use it. --- src/qemu/qemu_driver.c | 83 +++++++++++++---------------------------------- 1 files changed, 23 insertions(+), 60 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9140b50..b248fdb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -46,13 +46,6 @@ #include <sys/ioctl.h> #include <sys/un.h>

-#ifdef __linux__ -# include <sys/vfs.h> -# ifndef NFS_SUPER_MAGIC -# define NFS_SUPER_MAGIC 0x6969 -# endif /* NFS_SUPER_MAGIC */ -#endif /* __linux__ */ - #include "virterror_internal.h" #include "logging.h" #include "datatypes.h" @@ -5069,62 +5062,32 @@ static int qemudDomainSaveFlag(virDomainPtr dom, const char *path, goto endjob; }

-#ifdef __linux__ /* On Linux we can also verify the FS-type of the directory. */ - char *dirpath, *p; - struct statfs st; - int statfs_ret; - - if ((dirpath = strdup(path)) == NULL) { - virReportOOMError(); - goto endjob; - } - - do { - // Try less and less of the path until we get to a - // directory we can stat. Even if we don't have 'x' - // permission on any directory in the path on the NFS - // server (assuming it's NFS), we will be able to stat the - // mount point, and that will properly tell us if the - // fstype is NFS. - - if ((p = strrchr(dirpath, '/')) == NULL) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("Invalid relative path '%s' for domain save file"), - path); - VIR_FREE(dirpath); - goto endjob; - } - - if (p == dirpath) - *(p+1) = '\0'; - else - *p = '\0'; - - statfs_ret = statfs(dirpath, &st); - - } while ((statfs_ret == -1) && (p != dirpath)); - - if (statfs_ret == -1) { - virReportSystemError(errno, - _("Failed to create domain save file " - "'%s': statfs of all elements of path " - "failed"), - path); - VIR_FREE(dirpath); - goto endjob; - } + switch (virStorageFileIsSharedFS(path)) { + case 1: + /* it was on a network share, so we'll continue + * as outlined above + */ + break; + + case -1: + virReportSystemError(errno, + _("Failed to create domain save file " + "'%s': couldn't determine fs type"), + path); + goto endjob; + break; + + case 0: + default: + /* local file - log the error returned by virFileOperation */ + virReportSystemError(rc, + _("Failed to create domain save file '%s'"), + path); + goto endjob; + break;

- if (st.f_type != NFS_SUPER_MAGIC) { - virReportSystemError(rc, - _("Failed to create domain save file '%s'" - " (fstype of '%s' is 0x%X"), - path, dirpath, (unsigned int) st.f_type); - VIR_FREE(dirpath); - goto endjob; } - VIR_FREE(dirpath); -#endif

/* Retry creating the file as driver->user */

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Fri, Jun 25, 2010 at 01:22:17PM -0400, Laine Stump wrote:

When the saved domain image is on an NFS share, at least some part of domainSetSecurityAllLabel will fail (for example, selinux labels can't be modified). To allow domain restore to still work in this case, just ignore the errors. --- src/qemu/qemu_driver.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b248fdb..e282ecf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3368,8 +3368,10 @@ static int qemudStartVMDaemon(virConnectPtr conn, DEBUG0("Generating setting domain security labels (if required)"); if (driver->securityDriver && driver->securityDriver->domainSetSecurityAllLabel && - driver->securityDriver->domainSetSecurityAllLabel(vm, stdin_path) < 0) - goto cleanup; + driver->securityDriver->domainSetSecurityAllLabel(vm, stdin_path) < 0) { + if (virStorageFileIsSharedFS(stdin_path) != 1) + goto cleanup; + }

/* Ensure no historical cgroup for this VM is lying around bogus * settings */

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|