[libvirt] [PATCH 0/2] Some new utility functions
I'm planning to use these to fix the problems with creating storage volumes on root-squashing NFS servers, but since they may be of general utility I thought I'd throw them out for comments beforehand - it something different would make them more generally useful, better to change it now than after I've already finished code to use them. My plan is to use virRunWithHook() to call setuid (or possibly some sort of capng calisthenics) prior to execing qemu-img and qcow-create. virFileCreate and virDirCreate will be used when creating raw volumes. I have written test code to verify that virFileCreate works in several different scenarios. Haven't done so with virRun yet, but it's a fairly simple change, just exposing __virExec arguments that were already there. (I'd previously written test code (yes, it worked ;-)) that used a virRun with uid/gid args added on, but have decided against that approach, since this seems more flexible)
Similar to virExecWithHook, but waits for child to exit. Useful for doing things like setuid after the fork but before the exec. --- src/util/util.c | 25 ++++++++++++++++++------- src/util/util.h | 3 +++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 44a4b2f..1d493de 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -792,9 +792,11 @@ error: * only if the command could not be run. */ int -virRun(virConnectPtr conn, - const char *const*argv, - int *status) { +virRunWithHook(virConnectPtr conn, + const char *const*argv, + virExecHook hook, + void *data, + int *status) { pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -811,7 +813,7 @@ virRun(virConnectPtr conn, if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { + VIR_EXEC_NONE, hook, data, NULL)) < 0) { ret = execret; goto error; } @@ -867,9 +869,11 @@ virRun(virConnectPtr conn, #else /* __MINGW32__ */ int -virRun(virConnectPtr conn, - const char *const *argv ATTRIBUTE_UNUSED, - int *status) +virRunWithHook(virConnectPtr conn, + const char *const *argv ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; @@ -895,6 +899,13 @@ virExec(virConnectPtr conn, #endif /* __MINGW32__ */ +int +virRun(virConnectPtr conn, + const char *const*argv, + int *status) { + return virRunWithHook(conn, argv, NULL, NULL, status); +} + /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index d556daa..5e70038 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,9 @@ int virExec(virConnectPtr conn, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; +int virRunWithHook(virConnectPtr conn, const char *const*argv, + virExecHook hook, void *data, + int *status) ATTRIBUTE_RETURN_CHECK; int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.6.6
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown. If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.) Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 1d493de..d9a8e6e 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); } + +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file %s"), + path); + goto error; + } + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) { + if (fchown(fd, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to (%u, %u)"), + path, uid, gid); + close(fd); + goto error; + } + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to close new file %s"), + path); + goto error; + } + fd = -1; +error: + return ret; +} + +static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int ret = 0; + + if (mkdir(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create directory %s"), + path); + goto error; + } + + if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) { + if (chown(path, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to (%u, %u)"), + path, uid, gid); + goto error; + } + } +error: + return ret; +} + +#ifndef WIN32 +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret, status, ret = 0; + int fd = -1; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virFileCreateSimple(path, mode, uid, gid); + } + + /* parent is running as root, but caller requested that the + * file be created as some other user and/or group). The + * following dance avoids problems caused by root-squashing + * NFS servers. */ + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork o create file '%s'"), path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) + && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in + * some cases */ + /* Should we log a warning here? */ + return virFileCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to group %u"), + path, gid); + } + } + return ret; + } + + /* child - set desired uid/gid, then attempt to create the file */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating %s"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating %s"), + uid, path); + goto childerror; + } + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create file %s"), + path); + } + goto childerror; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("child failed to close new file %s"), + path); + goto childerror; + } +childerror: + _exit(ret); + +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret; + int status, ret = 0; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virDirCreateSimple(path, mode, uid, gid); + } + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork to create directory '%s'"), + path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &ret, 0) == -1) && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in some cases */ + /* Should we log a warning here? */ + return virDirCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to group %u"), + path, gid); + } + } + + return ret; + } + + /* child - set desired uid/gid, then attempt to create the directory */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating %s"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating %s"), + uid, path); + goto childerror; + } + if (mkdir(path, mode) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create directory %s"), + path); + } + goto childerror; + } +childerror: + _exit(ret); +} + +#else /* WIN32 */ + +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virFileCreateSimple(path, mode, uid, gid); +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virDirCreateSimple(path, mode, uid, gid); +} +#endif + int virFileMakePath(const char *path) { struct stat st; diff --git a/src/util/util.h b/src/util/util.h index 5e70038..2762862 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); +enum { + VIR_FILE_CREATE_NONE = 0, + VIR_FILE_CREATE_AS_UID = (1 << 0), +}; + +int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; +int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; int virFileBuildPath(const char *dir, -- 1.6.6
The first two patches add some utility functions, obsoleting the earlier two patches I sent to the list on Jan 8: https://www.redhat.com/archives/libvir-list/2010-January/msg00114.html The third patch uses these new utilities to rework the storage volume build functions to eliminate problems with pools located on root-squashing NFS servers. Note that the method of setting uid and gid of the created volumes remains the same in all cases where the pool containing the volume isn't type netfs - the volume will be created, then chowned. Only in the case that the volume will be in a pool of type netfs do we use a new method - first fork() and setuid/setgid in the child process before creating the volume. After returning from the child process, a couple extra steps are taken - 1) check if the child failed to create the new volume, and if so, try again as root, and 2) check if the volume really was created with the desired uid/gid, and if not, try setting it with chown. (This final step solves the problem of a parent directory that has the setgid bit on - simple calling setgid on the process that creates the file will then have no effect on the gid of the new file - it must be set explicitly after creation). The one problem I have with this final patch is the way that I determine if the pool containing the volume is of type netfs. The virStorageVolDef itself has no information about the pool; that information is only known a few layers up the call chain when someone is getting the buildVol or .buildVolFrom memor of a virStorageBackend struct. The way that I found to maintain the information of pool type (or more precisely, whether this is a netfs pool or something else) was to make buildVol and buildVolFrom point to different functions in the case of virStorageBackenNetFilesystem (it previously pointed to the same function as for type dir and fs). Since that first level function is just a stub that calls _virStorageBackendFileSystemVolBuild with a flag that is always 0, and since _virStorageBackendFileSystemVolBuild then calls the actual "create_func" with that same flag, AND since flag is currently unused by any of these functions, I just have the toplevel stub function set the bit "VIR_STORAGE_BUILD_NETFS" in the flag, and use that information when the time comes in the individual create_funcs. I don't like this because as far as I know, the flags are really there to allow for future expansion in the public API, and I've now subverted that usage 2/3 of the way down the call chain. So, I would appreciate either some help in deciding the best way for lower level functions (like the functions pointed to by create_func) to learn the type of the pool that will contain the volume pointed to by the virStorageVolDefPtr, or alternately affirmation that my subversion of the existing flags is acceptable. P.S. I have tested creation of raw and qcow volumes in several differnet scenarios (eg, parent directory isn't owned by the desired uid, creating a need to fall back to original method, parent directory has setgid bit on, etc.) with liberal DEBUGs throughout the code and verified that pretty much every path works.
The first patch is unchanged. The 2nd and 3rd patches reflact Dan's suggestions. (wrt Cole's concern about the pool object being unlocked during the buildVol call - Dan pointed out that this is read-only usage of the object, and it can't be deleted while in use, so it's safe to pass it in here.) Note that I've tested this as much as I can with my own rig plus some DEBUG statements and small code changes (in particular, forcing creation of volumes on local filesystems to use the code intended for netfs), but testing on real NFS (as well as Samba to see if it solves that problem too) would be very much appreciated.
Similar to virExecWithHook, but waits for child to exit. Useful for doing things like setuid after the fork but before the exec. --- src/util/util.c | 25 ++++++++++++++++++------- src/util/util.h | 3 +++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 44a4b2f..1d493de 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -792,9 +792,11 @@ error: * only if the command could not be run. */ int -virRun(virConnectPtr conn, - const char *const*argv, - int *status) { +virRunWithHook(virConnectPtr conn, + const char *const*argv, + virExecHook hook, + void *data, + int *status) { pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -811,7 +813,7 @@ virRun(virConnectPtr conn, if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { + VIR_EXEC_NONE, hook, data, NULL)) < 0) { ret = execret; goto error; } @@ -867,9 +869,11 @@ virRun(virConnectPtr conn, #else /* __MINGW32__ */ int -virRun(virConnectPtr conn, - const char *const *argv ATTRIBUTE_UNUSED, - int *status) +virRunWithHook(virConnectPtr conn, + const char *const *argv ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; @@ -895,6 +899,13 @@ virExec(virConnectPtr conn, #endif /* __MINGW32__ */ +int +virRun(virConnectPtr conn, + const char *const*argv, + int *status) { + return virRunWithHook(conn, argv, NULL, NULL, status); +} + /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index d556daa..5e70038 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,9 @@ int virExec(virConnectPtr conn, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; +int virRunWithHook(virConnectPtr conn, const char *const*argv, + virExecHook hook, void *data, + int *status) ATTRIBUTE_RETURN_CHECK; int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.6.6
On Wed, Jan 13, 2010 at 01:25:05AM -0500, Laine Stump wrote:
Similar to virExecWithHook, but waits for child to exit. Useful for doing things like setuid after the fork but before the exec. --- src/util/util.c | 25 ++++++++++++++++++------- src/util/util.h | 3 +++ 2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 44a4b2f..1d493de 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -792,9 +792,11 @@ error: * only if the command could not be run. */ int -virRun(virConnectPtr conn, - const char *const*argv, - int *status) { +virRunWithHook(virConnectPtr conn, + const char *const*argv, + virExecHook hook, + void *data, + int *status) { pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -811,7 +813,7 @@ virRun(virConnectPtr conn,
if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { + VIR_EXEC_NONE, hook, data, NULL)) < 0) { ret = execret; goto error; } @@ -867,9 +869,11 @@ virRun(virConnectPtr conn, #else /* __MINGW32__ */
int -virRun(virConnectPtr conn, - const char *const *argv ATTRIBUTE_UNUSED, - int *status) +virRunWithHook(virConnectPtr conn, + const char *const *argv ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; @@ -895,6 +899,13 @@ virExec(virConnectPtr conn,
#endif /* __MINGW32__ */
+int +virRun(virConnectPtr conn, + const char *const*argv, + int *status) { + return virRunWithHook(conn, argv, NULL, NULL, status); +} + /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index d556daa..5e70038 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,9 @@ int virExec(virConnectPtr conn, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; +int virRunWithHook(virConnectPtr conn, const char *const*argv, + virExecHook hook, void *data, + int *status) ATTRIBUTE_RETURN_CHECK;
int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
--
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown. If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.) Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 1d493de..1cb29f4 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); } + +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) { + if (fchown(fd, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + close(fd); + goto error; + } + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to close new file '%s'"), + path); + goto error; + } + fd = -1; +error: + return ret; +} + +static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int ret = 0; + + if (mkdir(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create directory '%s'"), + path); + goto error; + } + + if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) { + if (chown(path, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + goto error; + } + } +error: + return ret; +} + +#ifndef WIN32 +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret, status, ret = 0; + int fd = -1; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virFileCreateSimple(path, mode, uid, gid); + } + + /* parent is running as root, but caller requested that the + * file be created as some other user and/or group). The + * following dance avoids problems caused by root-squashing + * NFS servers. */ + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork o create file '%s'"), path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) + && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in + * some cases */ + /* Should we log a warning here? */ + return virFileCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to group %u"), + path, gid); + } + } + return ret; + } + + /* child - set desired uid/gid, then attempt to create the file */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create file '%s'"), + path); + } + goto childerror; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("child failed to close new file '%s'"), + path); + goto childerror; + } +childerror: + _exit(ret); + +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret; + int status, ret = 0; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virDirCreateSimple(path, mode, uid, gid); + } + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork to create directory '%s'"), + path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in some cases */ + /* Should we log a warning here? */ + return virDirCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to group %u"), + path, gid); + } + } + + return ret; + } + + /* child - set desired uid/gid, then attempt to create the directory */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if (mkdir(path, mode) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create directory '%s'"), + path); + } + goto childerror; + } +childerror: + _exit(ret); +} + +#else /* WIN32 */ + +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virFileCreateSimple(path, mode, uid, gid); +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virDirCreateSimple(path, mode, uid, gid); +} +#endif + int virFileMakePath(const char *path) { struct stat st; diff --git a/src/util/util.h b/src/util/util.h index 5e70038..2762862 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); +enum { + VIR_FILE_CREATE_NONE = 0, + VIR_FILE_CREATE_AS_UID = (1 << 0), +}; + +int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; +int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; int virFileBuildPath(const char *dir, -- 1.6.6
On Wed, Jan 13, 2010 at 01:25:06AM -0500, Laine Stump wrote:
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown.
If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.)
Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 1d493de..1cb29f4 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); }
+ +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) { + if (fchown(fd, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + close(fd); + goto error; + } + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to close new file '%s'"), + path); + goto error; + } + fd = -1; +error: + return ret; +} + +static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int ret = 0; + + if (mkdir(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create directory '%s'"), + path); + goto error; + } + + if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) { + if (chown(path, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + goto error; + } + } +error: + return ret; +} + +#ifndef WIN32 +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret, status, ret = 0; + int fd = -1; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virFileCreateSimple(path, mode, uid, gid); + } + + /* parent is running as root, but caller requested that the + * file be created as some other user and/or group). The + * following dance avoids problems caused by root-squashing + * NFS servers. */ + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork o create file '%s'"), path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) + && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in + * some cases */ + /* Should we log a warning here? */ + return virFileCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to group %u"), + path, gid); + } + } + return ret; + } + + /* child - set desired uid/gid, then attempt to create the file */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create file '%s'"), + path); + } + goto childerror; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("child failed to close new file '%s'"), + path); + goto childerror; + } +childerror: + _exit(ret); + +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret; + int status, ret = 0; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virDirCreateSimple(path, mode, uid, gid); + } + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork to create directory '%s'"), + path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in some cases */ + /* Should we log a warning here? */ + return virDirCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to group %u"), + path, gid); + } + } + + return ret; + } + + /* child - set desired uid/gid, then attempt to create the directory */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if (mkdir(path, mode) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create directory '%s'"), + path); + } + goto childerror; + } +childerror: + _exit(ret); +} + +#else /* WIN32 */ + +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virFileCreateSimple(path, mode, uid, gid); +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virDirCreateSimple(path, mode, uid, gid); +} +#endif + int virFileMakePath(const char *path) { struct stat st; diff --git a/src/util/util.h b/src/util/util.h index 5e70038..2762862 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+enum { + VIR_FILE_CREATE_NONE = 0, + VIR_FILE_CREATE_AS_UID = (1 << 0), +}; + +int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; +int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK;
int virFileBuildPath(const char *dir,
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Quoting Laine Stump (laine@laine.org):
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown.
If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.)
Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 1d493de..1cb29f4 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); }
+ +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
How about checking for CAP_CHOWN instead of getuid()==0? Otherwise if I try installing this certain ways, virFileCreateSimple() will refuse to try the chown even though it could succeed. -serge
Quoting Serge E. Hallyn (serue@us.ibm.com):
Quoting Laine Stump (laine@laine.org):
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown.
If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.)
Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 1d493de..1cb29f4 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); }
+ +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
How about checking for CAP_CHOWN instead of getuid()==0? Otherwise if I try installing this certain ways, virFileCreateSimple() will refuse to try the chown even though it could succeed.
I guess for certain netfs's the uid might matter more, so checking for either condition - or in fact just trying the fchown, then doing a stat, then making sure st.st_{ug}id are correct after the fact - seems useful. -serge
On 01/13/2010 09:32 AM, Serge E. Hallyn wrote:
Quoting Serge E. Hallyn (serue@us.ibm.com):
Quoting Laine Stump (laine@laine.org):
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown.
If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.)
Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 1d493de..1cb29f4 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); }
+ +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode))< 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if ((getuid() == 0)&& ((uid != 0) || (gid != 0))) {
How about checking for CAP_CHOWN instead of getuid()==0? Otherwise if I try installing this certain ways, virFileCreateSimple() will refuse to try the chown even though it could succeed.
I guess for certain netfs's the uid might matter more, so checking for either condition - or in fact just trying the fchown, then doing a stat, then making sure st.st_{ug}id are correct after the fact - seems useful.
That was actually just a duplication of existing functionality from the code that will now call virFileCreate() (right down to the lack of any error reporting if you're not running as root and the caller requests the file to be created with a different uid). The same check/chown is done in a couple other places in this patch series, so your comment applies to those as well. If I were to remove the check of current uid, should a failure of chown then be reported as an error, or ignored (making it closer to current behavior for non-root users)? And does this need to be added to the current patch, or can it be considered a follow-on? (I guess the question behind that is do people commonly use libvirt in a manner that would be affected by this, or is this theorizing about something someone *might* do?)
In order to avoid problems trying to chown files that were created by root on a root-squashing nfs server, fork a new process that setuid's to the desired uid before creating the file. (It's only done this way if the pool containing the new volume is of type 'netfs', otherwise the old method of creating the file followed by chown() is used.) This changes the semantics of the "create_func" slightly - previously it was assumed that this function just created the file, then the caller would chown it to the desired uid. Now, create_func does both operations. There are multiple functions that can take on the role of create_func: createFileDir - previously called mkdir(), now calls virDirCreate(). virStorageBackendCreateRaw - previously called open(), now calls virFileCreate(). virStorageBackendCreateQemuImg - use virRunWithHook() to setuid/gid. virStorageBackendCreateQcowCreate - same. virStorageBackendCreateBlockFrom - preserve old behavior. --- src/storage/storage_backend.c | 115 ++++++++++++++++++++++++++++---- src/storage/storage_backend.h | 8 ++- src/storage/storage_backend_disk.c | 3 +- src/storage/storage_backend_fs.c | 25 +++---- src/storage/storage_backend_logical.c | 3 +- src/storage/storage_driver.c | 4 +- 6 files changed, 123 insertions(+), 35 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9dc801c..1a39a55 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -205,6 +205,7 @@ cleanup: static int virStorageBackendCreateBlockFrom(virConnectPtr conn, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) @@ -229,6 +230,15 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn, goto cleanup; } + if (getuid() == 0) { + if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { + virReportSystemError(conn, errno, + _("cannot chown %s to (%u, %u)"), + vol->target.path, vol->target.perms.uid, + vol->target.perms.gid); + goto cleanup; + } + } if (close(fd) < 0) { virReportSystemError(conn, errno, _("cannot close file '%s'"), @@ -247,12 +257,14 @@ cleanup: int virStorageBackendCreateRaw(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { int fd = -1; int ret = -1; + int createstat; unsigned long long remain; char *buf = NULL; @@ -263,14 +275,23 @@ virStorageBackendCreateRaw(virConnectPtr conn, return -1; } - if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode)) < 0) { - virReportSystemError(conn, errno, + if ((createstat = virFileCreate(vol->target.path, vol->target.perms.mode, + vol->target.perms.uid, vol->target.perms.gid, + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_FILE_CREATE_AS_UID : 0))) < 0) { + virReportSystemError(conn, createstat, _("cannot create path '%s'"), vol->target.path); goto cleanup; } + if ((fd = open(vol->target.path, O_RDWR | O_EXCL)) < 0) { + virReportSystemError(conn, errno, + _("cannot open new path '%s'"), + vol->target.path); + goto cleanup; + } + /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, vol->capacity) < 0) { @@ -453,12 +474,82 @@ cleanup: return ret; } +static int virStorageBuildSetUIDHook(void *data) { + virStorageVolDefPtr vol = data; + + if ((vol->target.perms.gid != 0) + && (setgid(vol->target.perms.gid) != 0)) { + virReportSystemError(NULL, errno, + _("Cannot set gid to %u before creating %s"), + vol->target.perms.gid, vol->target.path); + return -1; + } + if ((vol->target.perms.uid != 0) + && (setuid(vol->target.perms.uid) != 0)) { + virReportSystemError(NULL, errno, + _("Cannot set uid to %u before creating %s"), + vol->target.perms.uid, vol->target.path); + return -1; + } + return 0; +} + +static int virStorageBackendCreateExecCommand(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + const char **cmdargv) { + struct stat st; + gid_t gid; + uid_t uid; + int filecreated = 0; + + if ((pool->def->type == VIR_STORAGE_POOL_NETFS) + && (getuid() == 0) + && ((vol->target.perms.uid != 0) || (vol->target.perms.gid != 0))) { + if (virRunWithHook(conn, cmdargv, + virStorageBuildSetUIDHook, vol, NULL) == 0) { + /* command was successfully run, check if the file was created */ + if (stat(vol->target.path, &st) >=0) + filecreated = 1; + } + } + if (!filecreated) { + if (virRun(conn, cmdargv, NULL) < 0) { + virReportSystemError(NULL, errno, + _("Cannot run %s to create %s"), + cmdargv[0], vol->target.path); + return -1; + } + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(NULL, errno, + _("%s failed to create %s"), + cmdargv[0], vol->target.path); + return -1; + } + } + + gid = (vol->target.perms.gid != 0) ? vol->target.perms.gid : -1; + uid = (vol->target.perms.uid != 0) ? vol->target.perms.uid : -1; + if (((gid != -1) && (gid != st.st_gid)) + || ((uid != -1) && (uid != st.st_uid))) { + /* either uid or gid wasn't properly set during creation */ + if (chown(vol->target.path, uid, gid) < 0) { + virReportSystemError(NULL, errno, _("cannot chown %s to (%u, %u)"), + vol->target.path, uid, gid); + return -1; + } + } + return 0; +} + static int virStorageBackendCreateQemuImg(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { + int ret; char size[100]; char *create_tool; short use_kvmimg; @@ -616,14 +707,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, /* Size in KB */ snprintf(size, sizeof(size), "%lluK", vol->capacity/1024); - if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - + ret = virStorageBackendCreateExecCommand(conn, pool, vol, imgargv); VIR_FREE(imgargv[0]); - return 0; + return ret; } /* @@ -632,10 +719,12 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, */ static int virStorageBackendCreateQcowCreate(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { + int ret; char size[100]; const char *imgargv[4]; @@ -672,14 +761,10 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, imgargv[2] = vol->target.path; imgargv[3] = NULL; - if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - + ret = virStorageBackendCreateExecCommand(conn, pool, vol, imgargv); VIR_FREE(imgargv[0]); - return 0; + return ret; } virStorageBackendBuildVolFrom diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 88c6161..fc5d8fb 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -34,14 +34,18 @@ typedef int (*virStorageBackendRefreshPool)(virConnectPtr conn, virStoragePoolOb typedef int (*virStorageBackendStopPool)(virConnectPtr conn, virStoragePoolObjPtr pool); typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags); -typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol); +typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); -typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); +typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStoragePoolObjPtr pool, + virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, + unsigned int flags); /* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags); diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 72ccd81..5ecf210 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -599,6 +599,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn, static int virStorageBackendDiskBuildVolFrom(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags) @@ -609,7 +610,7 @@ virStorageBackendDiskBuildVolFrom(virConnectPtr conn, if (!build_func) return -1; - return build_func(conn, vol, inputvol, flags); + return build_func(conn, pool, vol, inputvol, flags); } static int diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4fe40b3..1f98627 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -737,6 +737,7 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, } static int createFileDir(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { @@ -747,7 +748,10 @@ static int createFileDir(virConnectPtr conn, return -1; } - if (mkdir(vol->target.path, vol->target.perms.mode) < 0) { + if (virDirCreate(vol->target.path, vol->target.perms.mode, + vol->target.perms.uid, vol->target.perms.gid, + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_FILE_CREATE_AS_UID : 0)) != 0) { virReportSystemError(conn, errno, _("cannot create path '%s'"), vol->target.path); @@ -759,6 +763,7 @@ static int createFileDir(virConnectPtr conn, static int _virStorageBackendFileSystemVolBuild(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol) { @@ -794,7 +799,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, return -1; } - if (create_func(conn, vol, inputvol, 0) < 0) + if (create_func(conn, pool, vol, inputvol, 0) < 0) return -1; if ((fd = open(vol->target.path, O_RDONLY)) < 0) { @@ -804,16 +809,6 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, return -1; } - /* We can only chown/grp if root */ - if (getuid() == 0) { - if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { - virReportSystemError(conn, errno, - _("cannot set file owner '%s'"), - vol->target.path); - close(fd); - return -1; - } - } if (fchmod(fd, vol->target.perms.mode) < 0) { virReportSystemError(conn, errno, _("cannot set file mode '%s'"), @@ -839,8 +834,9 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, */ static int virStorageBackendFileSystemVolBuild(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { - return _virStorageBackendFileSystemVolBuild(conn, vol, NULL); + return _virStorageBackendFileSystemVolBuild(conn, pool, vol, NULL); } /* @@ -848,10 +844,11 @@ virStorageBackendFileSystemVolBuild(virConnectPtr conn, */ static int virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { - return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol); + return _virStorageBackendFileSystemVolBuild(conn, pool, vol, inputvol); } /** diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 5eebfac..c2e74a5 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -660,6 +660,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, static int virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags) @@ -670,7 +671,7 @@ virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, if (!build_func) return -1; - return build_func(conn, vol, inputvol, flags); + return build_func(conn, pool, vol, inputvol, flags); } static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c2f7850..50fcbe2 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1331,7 +1331,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, voldef->building = 1; virStoragePoolObjUnlock(pool); - buildret = backend->buildVol(obj->conn, buildvoldef); + buildret = backend->buildVol(obj->conn, pool, buildvoldef); storageDriverLock(driver); virStoragePoolObjLock(pool); @@ -1484,7 +1484,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjUnlock(origpool); } - buildret = backend->buildVolFrom(obj->conn, newvol, origvol, flags); + buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags); storageDriverLock(driver); virStoragePoolObjLock(pool); -- 1.6.6
Similar to virExecWithHook, but waits for child to exit. Useful for doing things like setuid after the fork but before the exec. --- src/util/util.c | 25 ++++++++++++++++++------- src/util/util.h | 3 +++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 44a4b2f..1d493de 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -792,9 +792,11 @@ error: * only if the command could not be run. */ int -virRun(virConnectPtr conn, - const char *const*argv, - int *status) { +virRunWithHook(virConnectPtr conn, + const char *const*argv, + virExecHook hook, + void *data, + int *status) { pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -811,7 +813,7 @@ virRun(virConnectPtr conn, if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { + VIR_EXEC_NONE, hook, data, NULL)) < 0) { ret = execret; goto error; } @@ -867,9 +869,11 @@ virRun(virConnectPtr conn, #else /* __MINGW32__ */ int -virRun(virConnectPtr conn, - const char *const *argv ATTRIBUTE_UNUSED, - int *status) +virRunWithHook(virConnectPtr conn, + const char *const *argv ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; @@ -895,6 +899,13 @@ virExec(virConnectPtr conn, #endif /* __MINGW32__ */ +int +virRun(virConnectPtr conn, + const char *const*argv, + int *status) { + return virRunWithHook(conn, argv, NULL, NULL, status); +} + /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index d556daa..5e70038 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,9 @@ int virExec(virConnectPtr conn, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; +int virRunWithHook(virConnectPtr conn, const char *const*argv, + virExecHook hook, void *data, + int *status) ATTRIBUTE_RETURN_CHECK; int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.6.6
On Mon, Jan 11, 2010 at 12:46:10AM -0500, Laine Stump wrote:
Similar to virExecWithHook, but waits for child to exit. Useful for doing things like setuid after the fork but before the exec. --- src/util/util.c | 25 ++++++++++++++++++------- src/util/util.h | 3 +++ 2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 44a4b2f..1d493de 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -792,9 +792,11 @@ error: * only if the command could not be run. */ int -virRun(virConnectPtr conn, - const char *const*argv, - int *status) { +virRunWithHook(virConnectPtr conn, + const char *const*argv, + virExecHook hook, + void *data, + int *status) { pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -811,7 +813,7 @@ virRun(virConnectPtr conn,
if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { + VIR_EXEC_NONE, hook, data, NULL)) < 0) { ret = execret; goto error; } @@ -867,9 +869,11 @@ virRun(virConnectPtr conn, #else /* __MINGW32__ */
int -virRun(virConnectPtr conn, - const char *const *argv ATTRIBUTE_UNUSED, - int *status) +virRunWithHook(virConnectPtr conn, + const char *const *argv ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; @@ -895,6 +899,13 @@ virExec(virConnectPtr conn,
#endif /* __MINGW32__ */
+int +virRun(virConnectPtr conn, + const char *const*argv, + int *status) { + return virRunWithHook(conn, argv, NULL, NULL, status); +} + /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index d556daa..5e70038 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,9 @@ int virExec(virConnectPtr conn, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; +int virRunWithHook(virConnectPtr conn, const char *const*argv, + virExecHook hook, void *data, + int *status) ATTRIBUTE_RETURN_CHECK;
int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown. If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.) Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 1d493de..6d31f47 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); } + +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file %s"), + path); + goto error; + } + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) { + if (fchown(fd, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to (%u, %u)"), + path, uid, gid); + close(fd); + goto error; + } + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to close new file %s"), + path); + goto error; + } + fd = -1; +error: + return ret; +} + +static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int ret = 0; + + if (mkdir(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create directory %s"), + path); + goto error; + } + + if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) { + if (chown(path, uid, gid) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to (%u, %u)"), + path, uid, gid); + goto error; + } + } +error: + return ret; +} + +#ifndef WIN32 +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret, status, ret = 0; + int fd = -1; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virFileCreateSimple(path, mode, uid, gid); + } + + /* parent is running as root, but caller requested that the + * file be created as some other user and/or group). The + * following dance avoids problems caused by root-squashing + * NFS servers. */ + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork o create file '%s'"), path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) + && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in + * some cases */ + /* Should we log a warning here? */ + return virFileCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to group %u"), + path, gid); + } + } + return ret; + } + + /* child - set desired uid/gid, then attempt to create the file */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating %s"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating %s"), + uid, path); + goto childerror; + } + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create file %s"), + path); + } + goto childerror; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("child failed to close new file %s"), + path); + goto childerror; + } +childerror: + _exit(ret); + +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + pid_t pid; + int waitret; + int status, ret = 0; + + if ((!(flags & VIR_FILE_CREATE_AS_UID)) + || (getuid() != 0) + || ((uid == 0) && (gid == 0))) { + return virDirCreateSimple(path, mode, uid, gid); + } + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork to create directory '%s'"), + path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + return ret; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in some cases */ + /* Should we log a warning here? */ + return virDirCreateSimple(path, mode, uid, gid); + } + if ((ret == 0) && (gid != 0)) { + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + struct stat st; + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("stat of '%s' failed"), + path); + return ret; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown %s to group %u"), + path, gid); + } + } + + return ret; + } + + /* child - set desired uid/gid, then attempt to create the directory */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating %s"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating %s"), + uid, path); + goto childerror; + } + if (mkdir(path, mode) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create directory %s"), + path); + } + goto childerror; + } +childerror: + _exit(ret); +} + +#else /* WIN32 */ + +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virFileCreateSimple(path, mode, uid, gid); +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virDirCreateSimple(path, mode, uid, gid); +} +#endif + int virFileMakePath(const char *path) { struct stat st; diff --git a/src/util/util.h b/src/util/util.h index 5e70038..2762862 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); +enum { + VIR_FILE_CREATE_NONE = 0, + VIR_FILE_CREATE_AS_UID = (1 << 0), +}; + +int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; +int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; int virFileBuildPath(const char *dir, -- 1.6.6
On Mon, Jan 11, 2010 at 12:46:11AM -0500, Laine Stump wrote:
These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown.
If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.)
Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 9 ++ 2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 1d493de..6d31f47 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1126,6 +1126,253 @@ int virFileExists(const char *path) return(0); }
+ +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) { + int fd = -1; + int ret = 0; + + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file %s"), + path);
Can you just put single quotes around the '%s' placeholder for the path. Every now & then someone uses a crazy path with whitespace and quotes help identify that kind of issue. Likewise for the rest of the calls. Functionally the patch looks good to me. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
In order to avoid problems trying to chown files that were created by root on a root-squashing nfs server, fork a new process that setuid's to the desired uid before creating the file. (It's only done this way if the pool containing the new volume is of type 'netfs', otherwise the old method of creating the file followed by chown() is used.) This changes the semantics of the "create_func" slightly - previously it was assumed that this function just created the file, then the caller would chown it to the desired uid. Now, create_func does both operations. There are multiple functions that can take on the role of create_func: createFileDir - previously called mkdir(), now calls virDirCreate(). virStorageBackendCreateRaw - previously called open(), now calls virFileCreate(). virStorageBackendCreateQemuImg - use virRunWithHook() to setuid/gid. virStorageBackendCreateQcowCreate - same. virStorageBackendCreateBlockFrom - preserve old behavior. --- src/storage/storage_backend.c | 115 ++++++++++++++++++++++++++++++++------ src/storage/storage_backend.h | 4 + src/storage/storage_backend_fs.c | 48 ++++++++++------ 3 files changed, 132 insertions(+), 35 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9dc801c..8a312ca 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -229,6 +229,15 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn, goto cleanup; } + if (getuid() == 0) { + if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { + virReportSystemError(conn, errno, + _("cannot chown %s to (%u, %u)"), + vol->target.path, vol->target.perms.uid, + vol->target.perms.gid); + goto cleanup; + } + } if (close(fd) < 0) { virReportSystemError(conn, errno, _("cannot close file '%s'"), @@ -249,10 +258,11 @@ int virStorageBackendCreateRaw(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { int fd = -1; int ret = -1; + int createstat; unsigned long long remain; char *buf = NULL; @@ -263,14 +273,23 @@ virStorageBackendCreateRaw(virConnectPtr conn, return -1; } - if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode)) < 0) { - virReportSystemError(conn, errno, + if ((createstat = virFileCreate(vol->target.path, vol->target.perms.mode, + vol->target.perms.uid, vol->target.perms.gid, + (flags & VIR_STORAGE_BUILD_NETFS + ? VIR_FILE_CREATE_AS_UID : 0))) < 0) { + virReportSystemError(conn, createstat, _("cannot create path '%s'"), vol->target.path); goto cleanup; } + if ((fd = open(vol->target.path, O_RDWR | O_EXCL)) < 0) { + virReportSystemError(conn, errno, + _("cannot open new path '%s'"), + vol->target.path); + goto cleanup; + } + /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, vol->capacity) < 0) { @@ -453,12 +472,81 @@ cleanup: return ret; } +static int virStorageBuildSetUIDHook(void *data) { + virStorageVolDefPtr vol = data; + + if ((vol->target.perms.gid != 0) + && (setgid(vol->target.perms.gid) != 0)) { + virReportSystemError(NULL, errno, + _("Cannot set gid to %u before creating %s"), + vol->target.perms.gid, vol->target.path); + return -1; + } + if ((vol->target.perms.uid != 0) + && (setuid(vol->target.perms.uid) != 0)) { + virReportSystemError(NULL, errno, + _("Cannot set uid to %u before creating %s"), + vol->target.perms.uid, vol->target.path); + return -1; + } + return 0; +} + +static int virStorageBackendCreateExecCommand(virConnectPtr conn, + virStorageVolDefPtr vol, + const char **cmdargv, + unsigned int flags) { + struct stat st; + gid_t gid; + uid_t uid; + int filecreated = 0; + + if ((flags & VIR_STORAGE_BUILD_NETFS) + && (getuid() == 0) + && ((vol->target.perms.uid != 0) || (vol->target.perms.gid != 0))) { + if (virRunWithHook(conn, cmdargv, + virStorageBuildSetUIDHook, vol, NULL) == 0) { + /* command was successfully run, check if the file was created */ + if (stat(vol->target.path, &st) >=0) + filecreated = 1; + } + } + if (!filecreated) { + if (virRun(conn, cmdargv, NULL) < 0) { + virReportSystemError(NULL, errno, + _("Cannot run %s to create %s"), + cmdargv[0], vol->target.path); + return -1; + } + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(NULL, errno, + _("%s failed to create %s"), + cmdargv[0], vol->target.path); + return -1; + } + } + + gid = (vol->target.perms.gid != 0) ? vol->target.perms.gid : -1; + uid = (vol->target.perms.uid != 0) ? vol->target.perms.uid : -1; + if (((gid != -1) && (gid != st.st_gid)) + || ((uid != -1) && (uid != st.st_uid))) { + /* either uid or gid wasn't properly set during creation */ + if (chown(vol->target.path, uid, gid) < 0) { + virReportSystemError(NULL, errno, _("cannot chown %s to (%u, %u)"), + vol->target.path, uid, gid); + return -1; + } + } + return 0; +} + static int virStorageBackendCreateQemuImg(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { + int ret; char size[100]; char *create_tool; short use_kvmimg; @@ -616,14 +704,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, /* Size in KB */ snprintf(size, sizeof(size), "%lluK", vol->capacity/1024); - if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - + ret = virStorageBackendCreateExecCommand(conn, vol, imgargv, flags); VIR_FREE(imgargv[0]); - return 0; + return ret; } /* @@ -636,6 +720,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { + int ret; char size[100]; const char *imgargv[4]; @@ -672,14 +757,10 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, imgargv[2] = vol->target.path; imgargv[3] = NULL; - if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - + ret = virStorageBackendCreateExecCommand(conn, vol, imgargv, flags); VIR_FREE(imgargv[0]); - return 0; + return ret; } virStorageBackendBuildVolFrom diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 88c6161..1e33308 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -40,6 +40,10 @@ typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObj typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); +enum { + VIR_STORAGE_BUILD_NETFS = (1 << 0), +}; + /* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, virStorageVolDefPtr vol, diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 4fe40b3..8fdf8b7 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -739,7 +739,7 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, static int createFileDir(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, - unsigned int flags ATTRIBUTE_UNUSED) { + unsigned int flags) { if (inputvol) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", @@ -747,7 +747,9 @@ static int createFileDir(virConnectPtr conn, return -1; } - if (mkdir(vol->target.path, vol->target.perms.mode) < 0) { + if (virDirCreate(vol->target.path, vol->target.perms.mode, + vol->target.perms.uid, vol->target.perms.gid, + flags & VIR_STORAGE_BUILD_NETFS ? VIR_FILE_CREATE_AS_UID : 0) != 0) { virReportSystemError(conn, errno, _("cannot create path '%s'"), vol->target.path); @@ -760,7 +762,8 @@ static int createFileDir(virConnectPtr conn, static int _virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol) + virStorageVolDefPtr inputvol, + unsigned int flags) { int fd; virStorageBackendBuildVolFrom create_func; @@ -794,7 +797,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, return -1; } - if (create_func(conn, vol, inputvol, 0) < 0) + if (create_func(conn, vol, inputvol, flags) < 0) return -1; if ((fd = open(vol->target.path, O_RDONLY)) < 0) { @@ -804,16 +807,6 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, return -1; } - /* We can only chown/grp if root */ - if (getuid() == 0) { - if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { - virReportSystemError(conn, errno, - _("cannot set file owner '%s'"), - vol->target.path); - close(fd); - return -1; - } - } if (fchmod(fd, vol->target.perms.mode) < 0) { virReportSystemError(conn, errno, _("cannot set file mode '%s'"), @@ -840,7 +833,15 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, static int virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr vol) { - return _virStorageBackendFileSystemVolBuild(conn, vol, NULL); + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, 0); +} + +/* version for vols created in netfs-based pools */ +static int +virStorageBackendFileSystemNetVolBuild(virConnectPtr conn, + virStorageVolDefPtr vol) { + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, + VIR_STORAGE_BUILD_NETFS); } /* @@ -851,9 +852,20 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { - return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol); + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, 0); +} + +/* version for vols created in netfs-based pools */ +static int +virStorageBackendFileSystemNetVolBuildFrom(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, + VIR_STORAGE_BUILD_NETFS); } + /** * Remove a volume - just unlinks for now */ @@ -959,8 +971,8 @@ virStorageBackend virStorageBackendNetFileSystem = { .refreshPool = virStorageBackendFileSystemRefresh, .stopPool = virStorageBackendFileSystemStop, .deletePool = virStorageBackendFileSystemDelete, - .buildVol = virStorageBackendFileSystemVolBuild, - .buildVolFrom = virStorageBackendFileSystemVolBuildFrom, + .buildVol = virStorageBackendFileSystemNetVolBuild, + .buildVolFrom = virStorageBackendFileSystemNetVolBuildFrom, .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, -- 1.6.6
On Mon, Jan 11, 2010 at 12:46:12AM -0500, Laine Stump wrote:
In order to avoid problems trying to chown files that were created by root on a root-squashing nfs server, fork a new process that setuid's to the desired uid before creating the file. (It's only done this way if the pool containing the new volume is of type 'netfs', otherwise the old method of creating the file followed by chown() is used.)
This changes the semantics of the "create_func" slightly - previously it was assumed that this function just created the file, then the caller would chown it to the desired uid. Now, create_func does both operations.
There are multiple functions that can take on the role of create_func:
createFileDir - previously called mkdir(), now calls virDirCreate(). virStorageBackendCreateRaw - previously called open(), now calls virFileCreate(). virStorageBackendCreateQemuImg - use virRunWithHook() to setuid/gid. virStorageBackendCreateQcowCreate - same. virStorageBackendCreateBlockFrom - preserve old behavior. --- src/storage/storage_backend.c | 115 ++++++++++++++++++++++++++++++++------ src/storage/storage_backend.h | 4 + src/storage/storage_backend_fs.c | 48 ++++++++++------ 3 files changed, 132 insertions(+), 35 deletions(-)
[snip]
@@ -840,7 +833,15 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, static int virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr vol) { - return _virStorageBackendFileSystemVolBuild(conn, vol, NULL); + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, 0); +} + +/* version for vols created in netfs-based pools */ +static int +virStorageBackendFileSystemNetVolBuild(virConnectPtr conn, + virStorageVolDefPtr vol) { + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, + VIR_STORAGE_BUILD_NETFS); }
/* @@ -851,9 +852,20 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { - return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol); + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, 0); +} + +/* version for vols created in netfs-based pools */ +static int +virStorageBackendFileSystemNetVolBuildFrom(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, + VIR_STORAGE_BUILD_NETFS); }
I think I'd be inclined to edit storage_backend.h and change the typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol); to typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); so that the build method has direct access to the pool type. Likewise for the BuildFrom method prototype. Once that change ripples through all the code, the place where you need to distinguish local vs net filesytem will have access to the pool object it wants Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 01/12/2010 07:36 AM, Daniel P. Berrange wrote:
On Mon, Jan 11, 2010 at 12:46:12AM -0500, Laine Stump wrote:
[snip]
@@ -840,7 +833,15 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, static int virStorageBackendFileSystemVolBuild(virConnectPtr conn, virStorageVolDefPtr vol) { - return _virStorageBackendFileSystemVolBuild(conn, vol, NULL); + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, 0); +} + +/* version for vols created in netfs-based pools */ +static int +virStorageBackendFileSystemNetVolBuild(virConnectPtr conn, + virStorageVolDefPtr vol) { + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, + VIR_STORAGE_BUILD_NETFS); }
/* @@ -851,9 +852,20 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { - return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol); + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, 0); +} + +/* version for vols created in netfs-based pools */ +static int +virStorageBackendFileSystemNetVolBuildFrom(virConnectPtr conn, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) { + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, + VIR_STORAGE_BUILD_NETFS); }
I think I'd be inclined to edit storage_backend.h and change the
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol);
to
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol);
so that the build method has direct access to the pool type. Likewise for the BuildFrom method prototype. Once that change ripples through all the code, the place where you need to distinguish local vs net filesytem will have access to the pool object it wants
This is a possibly problematic, since BuildVol is intended to be called with the pool lock dropped. So maybe BuildVol should have an extra 'int pool_type' argument rather than the whole pool object. - Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Laine Stump -
Serge E. Hallyn