[libvirt] [PATCH] qemu: eliminate "Ignoring open failure" message when using root-squash NFS
This eliminates the warning message reported in: https://bugzilla.redhat.com/show_bug.cgi?id=624447 It was caused by a failure to open an image file that is not accessible by root (the uid libvirtd is running as) because it's on a root-squash NFS share, owned by a different user, with permissions of 660 (or maybe 600). The solution is to re-try the open using virFileOpenAs() when appropriate. The codepath that generates the error is during qemuSetupDiskCGroup(), but the actual open() is in a lower-level generic function called from many places (virDomainDiskDefForeachPath), so some other pieces of the code were touched just to add dummy (or possibly useful) uid and gid arguments. Eliminating this warning message has the nice side effect that the requested opearation may even succeed (which in this case isn't necessary, but shouldn't hurt anything either). --- src/conf/domain_conf.c | 13 ++++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 2 ++ src/security/security_dac.c | 1 + src/security/security_selinux.c | 7 +++++++ src/security/virt-aa-helper.c | 6 +++++- 6 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 180dd2b..2b7ec91 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13370,6 +13370,7 @@ done: int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, bool allowProbing, bool ignoreOpenFailure, + uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque) { @@ -13425,8 +13426,18 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, disk->src); goto cleanup; } + fd = open(path, O_RDONLY); + if ((fd < 0) && (errno == EACCES || errno == EPERM) && + (uid > 0 || gid > 0)) { + fd = virFileOpenAs(path, O_RDONLY, 0, uid, gid, VIR_FILE_OPEN_AS_UID); + if (fd < 0) { + /* open() and virFileOpenAs() have different semantics */ + errno = -fd; + fd = -1; + } + } - if ((fd = open(path, O_RDONLY)) < 0) { + if (fd < 0) { if (ignoreOpenFailure) { char ebuf[1024]; VIR_WARN("Ignoring open failure on %s: %s", path, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3d5d4f8..2ea7861 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1935,6 +1935,7 @@ typedef int (*virDomainDiskDefPathIterator)(virDomainDiskDefPtr disk, int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, bool allowProbing, bool ignoreOpenFailure, + uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque); diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2d970d6..a07b6cd 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -96,6 +96,7 @@ int qemuSetupDiskCgroup(struct qemud_driver *driver, return virDomainDiskDefForeachPath(disk, driver->allowDiskFormatProbing, true, + driver->user, driver->group, qemuSetupDiskPathAllow, &data); } @@ -137,6 +138,7 @@ int qemuTeardownDiskCgroup(struct qemud_driver *driver, return virDomainDiskDefForeachPath(disk, driver->allowDiskFormatProbing, true, + driver->user, driver->group, qemuTeardownDiskPathDeny, &data); } diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 2fb4a14..3e1a72f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -186,6 +186,7 @@ virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, return virDomainDiskDefForeachPath(disk, virSecurityManagerGetAllowDiskFormatProbing(mgr), false, + priv->user, priv->group, virSecurityDACSetSecurityFileLabel, mgr); } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c2dceca..8c8e4f8 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -670,9 +670,16 @@ SELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, if (disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK) return 0; + /* XXX On one hand, it would be nice to have the driver's uid:gid + * here so we could retry opens with it. On the other hand, it + * probably doesn't matter because in practice that's only useful + * for files on root-squashed NFS shares, and NFS doesn't properly + * support selinux anyway. + */ return virDomainDiskDefForeachPath(disk, allowDiskFormatProbing, true, + -1, -1, /* current process uid:gid */ SELinuxSetSecurityFileLabel, secdef); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 4561bb9..015ef2f 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -909,10 +909,14 @@ get_files(vahControl * ctl) /* XXX passing ignoreOpenFailure = true to get back to the behavior * from before using virDomainDiskDefForeachPath. actually we should * be passing ignoreOpenFailure = false and handle open errors more - * careful than just ignoring them */ + * careful than just ignoring them. + * XXX2 - if we knew the qemu user:group here we could send it in + * so that the open could be re-tried as that user:group. + */ int ret = virDomainDiskDefForeachPath(ctl->def->disks[i], ctl->allowDiskFormatProbing, true, + -1, -1 /* current uid:gid */ add_file_path, &buf); if (ret != 0) -- 1.7.7.5
[Oops. This is a prerequisite of the previous patch that I forgot to send. That patch should be 2/2 and this should be 1/2.] This just simplifies use of virFileOpenAs a bit - if you're in a place where you don't have access to a different uid|gid, just give "-1". --- src/libxl/libxl_driver.c | 4 ++-- src/storage/storage_backend.c | 8 +++----- src/util/util.c | 4 ++++ 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0500ed0..d7325c3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -216,7 +216,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from, libxlSavefileHeader hdr; char *xml = NULL; - if ((fd = virFileOpenAs(from, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) { + if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) { libxlError(VIR_ERR_OPERATION_FAILED, "%s", _("cannot read domain image")); goto error; @@ -1827,7 +1827,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, } if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR, - getuid(), getgid(), 0)) < 0) { + -1, -1, 0)) < 0) { virReportSystemError(-fd, _("Failed to create domain save file '%s'"), to); goto cleanup; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d7394e0..1bd3e6e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -380,8 +380,6 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, { int ret = -1; int fd = -1; - uid_t uid; - gid_t gid; int operation_flags; virCheckFlags(0, -1); @@ -393,15 +391,15 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid; - gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid; operation_flags = VIR_FILE_OPEN_FORCE_PERMS; if (pool->def->type == VIR_STORAGE_POOL_NETFS) operation_flags |= VIR_FILE_OPEN_AS_UID; if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode, uid, gid, + vol->target.perms.mode, + vol->target.perms.uid, + vol->target.perms.gid, operation_flags)) < 0) { virReportSystemError(-fd, _("cannot create path '%s'"), diff --git a/src/util/util.c b/src/util/util.c index 6f46d53..73003fe 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -848,6 +848,10 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, int pair[2] = { -1, -1 }; int forkRet; + /* allow using -1 to mean "current value" */ + uid = (uid == -1) ? getuid() : uid; + gid = (gid == -1) ? getgid() : gid; + if ((!(flags & VIR_FILE_OPEN_AS_UID)) || (getuid() != 0) || ((uid == 0) && (gid == 0))) { -- 1.7.7.5
On 01/12/2012 12:33 PM, Laine Stump wrote:
[Oops. This is a prerequisite of the previous patch that I forgot to send. That patch should be 2/2 and this should be 1/2.]
This just simplifies use of virFileOpenAs a bit - if you're in a place where you don't have access to a different uid|gid, just give "-1". --- src/libxl/libxl_driver.c | 4 ++-- src/storage/storage_backend.c | 8 +++----- src/util/util.c | 4 ++++ 3 files changed, 9 insertions(+), 7 deletions(-)
I like it; very much like chown()'s use of -1 to mean no change. Alas, I have a nit: POSIX permits uid_t to be an unsigned short. On implementations that make that choice, casting (uid_t)-1 to an int gives 0xffff, rather than -1, and comparison to -1 will fail. In general, it is safest if you use an explicit cast when comparing to a uid_t or gid_t value in any arithmetic operation, since the act of comparing might include an implicit conversion to int.
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 0500ed0..d7325c3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -216,7 +216,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from, libxlSavefileHeader hdr; char *xml = NULL;
- if ((fd = virFileOpenAs(from, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) { + if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0)) < 0) {
This use is fine, since virFileOpenAs is explicitly typed as taking uid_t and gid_t: -1 is always correctly promoted to uid_t or gid_t.
+++ b/src/storage/storage_backend.c @@ -380,8 +380,6 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, { int ret = -1; int fd = -1; - uid_t uid; - gid_t gid; int operation_flags;
virCheckFlags(0, -1); @@ -393,15 +391,15 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; }
- uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid; - gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
Here, perms.uid is an int (hmm, I wonder if we should fix that in virStoragePerms in conf/storage_conf.h, since that fails on platforms where uid_t is larger than an int - but that's a fix for another day).
+++ b/src/util/util.c @@ -848,6 +848,10 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, int pair[2] = { -1, -1 }; int forkRet;
+ /* allow using -1 to mean "current value" */ + uid = (uid == -1) ? getuid() : uid; + gid = (gid == -1) ? getgid() : gid;
But here, you have a bug - the use of == does type promotion to the common type no smaller than int, which means you need an explicit cast: uid = (uid == (uid_t) -1) ? getuid() : uid; And that's repetitive enough that I'd probably write it: if (uid == (uid_t) -1) uid = getuid(); Same for gid. ACK with the 2 lines in util.c fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/12/2012 12:28 PM, Laine Stump wrote:
This eliminates the warning message reported in:
https://bugzilla.redhat.com/show_bug.cgi?id=624447
It was caused by a failure to open an image file that is not accessible by root (the uid libvirtd is running as) because it's on a root-squash NFS share, owned by a different user, with permissions of 660 (or maybe 600).
The solution is to re-try the open using virFileOpenAs() when appropriate. The codepath that generates the error is during qemuSetupDiskCGroup(), but the actual open() is in a lower-level generic function called from many places (virDomainDiskDefForeachPath), so some other pieces of the code were touched just to add dummy (or possibly useful) uid and gid arguments.
Eliminating this warning message has the nice side effect that the requested opearation may even succeed (which in this case isn't
s/opearation/operation/
necessary, but shouldn't hurt anything either). --- src/conf/domain_conf.c | 13 ++++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_cgroup.c | 2 ++ src/security/security_dac.c | 1 + src/security/security_selinux.c | 7 +++++++ src/security/virt-aa-helper.c | 6 +++++- 6 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 180dd2b..2b7ec91 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -13370,6 +13370,7 @@ done: int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, bool allowProbing, bool ignoreOpenFailure, + uid_t uid, gid_t gid, virDomainDiskDefPathIterator iter, void *opaque) { @@ -13425,8 +13426,18 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, disk->src); goto cleanup; } + fd = open(path, O_RDONLY); + if ((fd < 0) && (errno == EACCES || errno == EPERM) && + (uid > 0 || gid > 0)) {
Repeating my complaints from 1/2, it is possible for uid_t to be an unsigned type (and in fact, this is the case on Linux), which means this will not filter out the case of (uid_t)-1 on all platforms. Is your goal to avoid a pointless virFileOpenAs() call? Remember, virFileOpenAs called with both uid and gid of -1 will end up opening with the current ids, but that's the same as what open() already does, so it should fail in the same manner. The only time virFileOpenAs() is useful is if there is no earlier open() attempt, or if the requested uid or gid differs from the current process ids. So I think you might be able to get away with: if (... && (uid != (uid_t)-1 || gid != (gid_t)-1)) { or just rely on a single virFileOpenAs() call without bothering with an initial open() attempt. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump