Re: [libvirt] [PATCHv3 1/2] util: refactor virFileOpenAs

On 01/27/2012 10:43 AM, Laine Stump wrote: > virFileOpenAs previously would only try opening a file as the current > user, or as a different user, but wouldn't try both methods in a > single call. This made it cumbersome to use as a replacement for > open(2). Additionally, it had a lot of historical baggage that led to > it being difficult to understand. > > This patch refactors virFileOpenAs in the following ways: > > V3 differences - Eric Blake wondered if fchmod() issued by uid 0 of a > file opened as a different user on a root-squashed NFS would actually > work. It turns out it won't, so I put fchmod back into the child > process. And since there are already so many other pieces of code > related with this that would try to log a message on failure > (e.g. virSetUIDGID()), I backed off on that as well, and have put the > log messages in child process code back in - that's a bigger problem > for another day. Sometimes it seems like those big nasty problems are there until someone actually hits them in practice. Oh well. Even if we don't make it 100% perfect, I still think that your rewrite makes it easier to understand, and therefore easier to maintain, should we come across the day where we need to fix the nasty problems. And for anyone in the future re-reading this thread while trying to fix the problems (hello there!), here's an idea - virSetUIDGID should be broken into two functions, virSetUIDGIDRaw, which only uses async-safe functions and returns -errno on failure, and virSetUIDGID that formats errors into log message for normal users. > } else { > - if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, > - uid, gid, 0))< 0) { > + if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid, > + vfoflags | VIR_FILE_OPEN_NOFORK))< 0) { > /* If we failed as root, and the error was permission-denied > (EACCES or EPERM), assume it's on a network-connected share > where root access is restricted (eg, root-squashed NFS). If the > @@ -2385,7 +2387,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags, > if ((fd = virFileOpenAs(path, oflags, > S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, > driver->user, driver->group, > - VIR_FILE_OPEN_AS_UID))< 0) { > + vfoflags | VIR_FILE_OPEN_FORK))< 0) { You weren't kidding about the difference in 0600 vs. 0660 permissions between the two attempts. My gut feel is that we want 0660 (that's why we invented virSetUIDGID in the first place - people like to use group permissions and membership in multiple groups as a reasonable form of access management); but I also agree with your decision to keep this patch semantically unchanged, and leave any permissions modifications for a later patch in isolation. > -/* return -errno on failure, or 0 on success */ > +/* virFileOpenForked() - an internal utility function called only by > + * virFileOpenAs(). It forks, then the child does setuid+setgid to > + * given uid:gid and attempts to open the file, while the parent just > + * calls recvfd to get the open fd back from the child. returns the > + * fd, or -errno if there is an error. */ > static int > -virFileOpenAsNoFork(const char *path, int openflags, mode_t mode, > - uid_t uid, gid_t gid, unsigned int flags) Here, I'll just paste the code post-patch and review that in isolation, rather than caring about the delta: > /* virFileOpenForked() - an internal utility function called only by > * virFileOpenAs(). It forks, then the child does setuid+setgid to > * given uid:gid and attempts to open the file, while the parent just > * calls recvfd to get the open fd back from the child. returns the > * fd, or -errno if there is an error. */ > static int > virFileOpenForked(const char *path, int openflags, mode_t mode, > uid_t uid, gid_t gid, unsigned int flags) > { > pid_t pid; > int waitret, status, ret = 0; > int fd = -1; > int pair[2] = { -1, -1 }; > int forkRet; > > /* parent is running as root, but caller requested that the > * file be created as some other user and/or group). The Comment is out-of-date; s/created/opened/ > * following dance avoids problems caused by root-squashing > * NFS servers. */ > > if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair)< 0) { > ret = -errno; > virReportSystemError(errno, > _("failed to create socket needed for '%s'"), > path); > return ret; > } > > forkRet = virFork(&pid); > if (pid< 0) > return -errno; > > if (pid) { /* parent */ > VIR_FORCE_CLOSE(pair[1]); > > do { > fd = recvfd(pair[0], 0); > } while (fd< 0&& errno == EINTR); > VIR_FORCE_CLOSE(pair[0]); /* NB: this preserves errno */ > > if (fd< 0&& errno != EACCES) { > ret = -errno; > while (waitpid(pid, NULL, 0) == -1&& errno == EINTR); Hmm, this might do better as virPidAbort(pid). > return ret; > } > And from here... > /* 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(errno, > _("failed to wait for child creating '%s'"), > path); > VIR_FORCE_CLOSE(fd); > return ret; > } > if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES || > fd == -1) { ...to here, we could probably replace a good chunk of code with the simpler virPidWait(pid,&status). But both of those should be separate cleanups, as this part of the function was not touched by your refactoring of the public interface.

> /* fall back to the simpler method, which works better in > * some cases */ > VIR_FORCE_CLOSE(fd); > if ((fd = open(path, openflags, mode))< 0) I would tweak things to only attempt this if !(flags& VIR_FILE_OPEN_NOFORK). That is, if NOFORK was requested, the we already tried this open once, before even deciding to call this helper function, so since it failed once it will fail again. Only if NOFORK was not requested is it worth this fallback in the parent, but then you have the issue that the parent needs to honor fchmod() somehow.

> return -errno; > } > return fd; > } > > /* child */ > > VIR_FORCE_CLOSE(pair[0]); /* preserves errno */ > if (forkRet< 0) { > /* error encountered and logged in virFork() after the fork. */ > ret = -errno; > goto childerror; > } > > /* set desired uid/gid, then attempt to create the file */ > > if (virSetUIDGID(uid, gid)< 0) { > ret = -errno; > goto childerror; > } > > if ((fd = open(path, openflags, mode))< 0) { > ret = -errno; > virReportSystemError(errno, _("child process failed to create file '%s'"), > path); Repeating myself, logging in the child needs fixing someday, but it is no worse than the code you were refactoring, so I'm okay with leaving things as they were and keeping this patch focused on the interface. > goto childerror; > } > > /* File is successfully open. Set permissions if requested. */ > if ((flags& VIR_FILE_OPEN_FORCE_MODE) > && (fchmod(fd, mode)< 0)) { > ret = -errno; > virReportSystemError(errno, > _("child process cannot set mode of '%s' to %04o"), > path, mode); > goto childerror; > } > > do { > ret = sendfd(pair[1], fd); > } while (ret< 0&& errno == EINTR); > > if (ret< 0) { > ret = -errno; > virReportSystemError(errno, "%s", > _("child process failed to send fd to parent")); > goto childerror; > } > > childerror: > /* ret tracks -errno on failure, but exit value must be positive. > * If the child exits with EACCES, then the parent tries again. */ > /* XXX This makes assumptions about errno being< 255, which is > * not true on Hurd. */ > VIR_FORCE_CLOSE(pair[1]); > if (ret< 0) { > VIR_FORCE_CLOSE(fd); > } > ret = -ret; > if ((ret& 0xff) != ret) { > VIR_WARN("unable to pass desired return value %d", ret); > ret = 0xff; > } > _exit(ret); If we do any cleanups to fix that XXX comment, they can also be as separate patches. > } > > /** > * virFileOpenAs: > * @path: file to open or create > * @openflags: flags to pass to open > * @mode: mode to use on creation or when forcing permissions > * @uid: uid that should own file on creation > * @gid: gid that should own file > * @flags: bit-wise or of VIR_FILE_OPEN_* flags > * > * Open @path, and return an fd to the open file. @openflags are the flags > * normally passed to open(2), while @flags is used internally. If s/is/are/ > * @flags includes VIR_FILE_OPEN_NOFORK, then try opening the file > * while executing with the current uid:gid (i.e. don't > * fork+setuid+setgid before the call to open()). If @flags includes > * VIR_FILE_OPEN_FORK, then try opening the file while the effective > * user id is @uid (by forking a child process); this allows one to > * bypass root-squashing NFS issues ; NOFORK is always tried before s/ ;/;/ > * FORK (the absence of both flags is treated identically to > * (VIR_FILE_OPEN_NOFORK | VIR_FILE_OPEN_FORK)). If @flags includes > * VIR_FILE_OPEN_FORCE_OWNER, then ensure that @path is owned by > * uid:gid before returning (even if it already existed with a > * different owner). If @flags includes VIR_FILE_OPEN_FORCE_MODE, > * ensure it has those permissions before returning (again, even if > * the file already existed with different permissions). The return > * value (if non-negative) is the file descriptor, left open. Returns > * -errno on failure. */ > int > virFileOpenAs(const char *path, int openflags, mode_t mode, > uid_t uid, gid_t gid, unsigned int flags) > { > int ret = 0, fd = -1; > > /* allow using -1 to mean "current value" */ > if (uid == (uid_t) -1) > uid = getuid(); > if (gid == (gid_t) -1) > gid = getgid(); > > /* treat absence of both flags as presence of both for simpler > * calling. */ > if (!(flags& (VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK))) > flags |= VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK; > > if ((flags& VIR_FILE_OPEN_NOFORK) > || (getuid() != 0) > || ((uid == 0)&& (gid == 0))) { > > if ((fd = open(path, openflags, mode))< 0) { > ret = -errno; > } else { > if (flags& VIR_FILE_OPEN_FORCE_OWNER) { > /* successfully opened as current user. Try fchown if > * appropriate. */ > struct stat st; > > if (fstat(fd,&st) == -1) { > ret = -errno; > virReportSystemError(errno, _("stat of '%s' failed"), path); > goto error; > } > if (((st.st_uid != uid) || (st.st_gid != gid)) Safe, since we know uid and gid are not -1 at this point. (It took me two tries to pick up on that point, though, so adding a comment might be useful).

> && (fchown(fd, uid, gid)< 0)) { > ret = -errno; > virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), > path, (unsigned int) uid, > (unsigned int) gid); > goto error; > } > } > /* File is successfully open. Set permissions if requested. */ > if ((flags& VIR_FILE_OPEN_FORCE_MODE) > && (fchmod(fd, mode)< 0)) { I'm wondering if we should hoist the fstat, and only attempt the fchmod if needed. That is, arrange the logic: } else { // fd is open if (flags& (force_owner | force_mode)) { if fstat fails error if (flags& force_owner&& owner is wrong) { if fchown fails error } if (flags& force_mode&& mode is wrong) { if fchmod fails error } } }

> ret = -errno; > virReportSystemError(errno, > _("cannot set mode of '%s' to %04o"), > path, mode); > goto error; > } > } > } > > /* If we either 1) didn't try opening as current user at all, or > * 2) failed, and errno/virStorageFileIsSharedFS indicate we might > * be successful if we try as a different uid, then try doing > * fork+setuid+setgid before opening. > */ > if ((fd< 0)&& (flags& VIR_FILE_OPEN_FORK)) { > > if (ret< 0) { > /* An open(2) that failed due to insufficient permissions > * could return one or the other of these depending on OS > * version and circumstances. Any other errno indicates a > * problem that couldn't be remedied by fork+setuid > * anyway. */ > if (ret != -EACCES&& ret != -EPERM) > goto error; > > /* On Linux we can also verify the FS-type of the > * directory. (this is a NOP on other platforms). */ > switch (virStorageFileIsSharedFS(path)) { > case 1: > /* it was on a network share, so we'll re-try */ > break; > case -1: > /* failure detecting fstype */ > virReportSystemError(errno, _("couldn't determine fs type of" > "of mount containing '%s'"), path); > goto error; > case 0: > default: > /* file isn't on a recognized network FS */ > goto error; > } > } > > /* passed all prerequisites - retry the open w/fork+setuid */ > if ((fd = virFileOpenForked(path, openflags, mode, uid, gid, flags))< 0) { > ret = fd; > fd = -1; > goto error; Hmm, this means that force_mode has to be done in the helper function. I'm almost thinking you should move the fchmod/fchown code into a helper routine, and call it in two places: in virFileOpenForked, in the child, after fd is successfully opened in virFileOpenAs, _after_ you have a working fd (whether from NOFORK or FORK code paths) and if the fchmod/fchown code is a no-op when modes are already correct, then you won't have issues with root-squash being unable to fchmod in the parent.

> } > } > > /* File is successfully opened */ > return fd; > > error: > if (fd< 0) { > /* whoever failed the open last has already set ret = -errno */ > virReportSystemError(-ret, openflags& O_CREAT > ? _("failed to create file '%s'") > : _("failed to open file '%s'"), > path); > } else { > /* some other failure after the open succeeded */ > VIR_FORCE_CLOSE(fd); Hmm, should we unlink() the file if O_CREAT was specified and we were the ones that ended up creating it? That's a tough call.

> } > return ret; > } > I still think there's a bit of work needed; are you up for trying a v4, or should I try it on top of what you have?