9:50 a.m.
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
9:50 a.m.
New subject: [libvirt] [PATCH 1/2] storage: Really fix setting mode for backend exec in NFS root-squash env
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(a)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
9:50 a.m.
New subject: [libvirt] [PATCH 2/2] storage: Change virStorageBackendVolOpen to use virFileOpenAs
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(a)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
10:15 a.m.
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
3320
days inactive
3323
days old
3 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik