[libvirt] [PATCH 0/2] Address more nfs root-squash issues
Fix a couple of more issues with an NFS root-squash environment. The details are in the commit messages and bugzilla comments. NB: While using virFileOpenAs in the virStorageBackendVolOpen path and checking for EACESS/EPERM afterwards may seem a bit counter intuitive, but those will be returned if the virFileOpenAs code doesn't attempt to open in the forked environment. John Ferlan (2): storage: Really fix setting mode for backend exec in NFS root-squash env storage: Change virStorageBackendVolOpen to use virFileOpenAs src/storage/storage_backend.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) -- 2.5.0
https://bugzilla.redhat.com/show_bug.cgi?id=1282288 Although commit id '77346f27' resolves part of the problem regarding creating a qemu-img image in an NFS root-squash environment, it really didn't fix the entire problem. Unfortunately it only masked the problem. It seems qemu-img must open/create the image using 0644, which if used by target.perms would result in the chmod not being called since the mode desired and set match. Although qemu-img could conceivably ignore the mode when creating, libvirt has more knowledge of the environment and can make the adjustment to the mode far more easily by using virFileOpenAs with VIR_FILE_OPEN_FORCE_MODE. If that's successful, then we know on return the file will have the right owner and mode, so we can declare success Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 77e87b3..3f36aa3 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -702,8 +702,28 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, check if the file was created */ - if (stat(vol->target.path, &st) >= 0) + if (stat(vol->target.path, &st) >= 0) { filecreated = true; + + /* seems qemu-img disregards umask and open/creates using 0644. + * If that doesn't match what we expect, then let's try to + * re-open the file and attempt to force the mode change. + */ + if (mode != (st.st_mode & S_IRWXUGO)) { + int fd = -1; + int flags = VIR_FILE_OPEN_FORK | VIR_FILE_OPEN_FORCE_MODE; + + if ((fd = virFileOpenAs(vol->target.path, O_RDWR, mode, + vol->target.perms->uid, + vol->target.perms->gid, + flags)) >= 0) { + /* Success - means we're good */ + VIR_FORCE_CLOSE(fd); + ret = 0; + goto cleanup; + } + } + } } } -- 2.5.0
https://bugzilla.redhat.com/show_bug.cgi?id=1282288 Rather than using just open on the path, allow for the possibility that the path to be opened resides on an NFS root-squash target and was created under a different uid/gid. Without using virFileOpenAs an attempt to get the volume size data may fail if the current user doesn't have permissions to read the volume, such as would be the case if mode wasn't supplied in the volume XML and the default VIR_STORAGE_DEFAULT_VOL_PERM_MODE (e.g. 0600) was used. Under this scenario the owner/group is not root:root, thus this path run under root would fail to open/read the volume. NB: The virFileOpenAs code using OPEN_FORK will only work when the failure is not EACESS/EPERM and the path resolves to a shared file system. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 3f36aa3..194736b 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1484,9 +1484,18 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, * sockets, which we already filtered; but using it prevents a * TOCTTOU race. However, later on we will want to read() the * header from this fd, and virFileRead* routines require a - * blocking fd, so fix it up after verifying we avoided a - * race. */ - if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) { + * blocking fd, so fix it up after verifying we avoided a race. + * + * Use of virFileOpenAs allows this path to open a file using + * the uid and gid as it was created in order to open. Since this + * path is not using O_CREAT or O_TMPFILE, mode is meaningless. + * Opening under user/group is especially important in an NFS + * root-squash environment. If the target path isn't on shared + * file system, the open will fail in the OPEN_FORK path. + */ + if ((fd = virFileOpenAs(path, O_RDONLY|O_NONBLOCK|O_NOCTTY, + 0, sb->st_uid, sb->st_gid, + VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK)) < 0) { if ((errno == ENOENT || errno == ELOOP) && S_ISLNK(sb->st_mode) && noerror) { VIR_WARN("ignoring dangling symlink '%s'", path); -- 2.5.0
On 17.11.2015 16:50, John Ferlan wrote:
Fix a couple of more issues with an NFS root-squash environment. The details are in the commit messages and bugzilla comments.
NB: While using virFileOpenAs in the virStorageBackendVolOpen path and checking for EACESS/EPERM afterwards may seem a bit counter intuitive, but those will be returned if the virFileOpenAs code doesn't attempt to open in the forked environment.
John Ferlan (2): storage: Really fix setting mode for backend exec in NFS root-squash env storage: Change virStorageBackendVolOpen to use virFileOpenAs
src/storage/storage_backend.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-)
ACK to both Michal
participants (2)
-
John Ferlan -
Michal Privoznik