[libvirt] [PATCHv2 1/2] util: refactor virFileOpenAs
Laine Stump
laine at laine.org
Fri Jan 27 16:04:07 UTC 2012
On 01/26/2012 07:33 PM, Eric Blake wrote:
> On 01/17/2012 06:44 PM, Laine Stump wrote:
>> * Reduce what is executed in the context of the child process to just
>> the bare minimum necessary to open the file and send its fd back to
>> the parent. In particular, the older version had also done fchmod()
> Hmm. I'm not familiar enough with root-squash to know the answer
> off-hand. I know that root-squash prevents uid 0 from open()ing a
> remote file (by instead opening the file under the nobody account), but
> once the file is open, can root proceed to fchown() or even fchmod() the
> file? fchmod() is unlikely to succeed except in the case of swapping
> the gid_t ownership if done as non-root, so I can understand pulling
> fchown() out to the public function after the child has already passed
> the fd. And hopefully root can fchmod() an fd that it obtained from a
> child; but if not, then you have to move the fchmod() back into the
> child code.
I had ASSumed too much, and was wrong. I just artificially created a
situation where I would need to fchmod and it failed. So, as you say, I
need to move the fchmod() back into the child process code.
>> (and had code to call fchown(), although that would never actually
>> be executed in the case of forking prior to the open). This code is
>> now in the main public function, and also executed in the context of
>> the parent process, thus reducing the chance that we may end up
>> trying to call some async-unsafe function from the child.
> fchown and fchmod are async-safe. (Most syscalls in 'man 2' fall in
> this category; it's when you get to 'man 3' that you are likely to be
> introducing library functions that hold locks and aren't atomic where
> you are no longer async-safe).
I was more concerned about the logging code that's called in case of an
error.
>> * Allow a single call to virFileOpenAs() to first attempt the open as
>> the current user, and if that fails to automatically re-try after
>> doing fork+setuid (if deemed appropriate, i.e. errno indicates it
>> would now be successful, and the file is on a networkFS). This makes
>> it possible (in many, but possibly not all, cases) to drop-in
>> virFileOpenAs() as a replacement for open(2).
> Sounds like a nice cleanup, especially since we previously had several
> call sites that had to dance around virFileOpenAs twice to get the same
> behavior. Refactoring the double attempt into the common code is good.
I did it more for new uses of virFileOpenAs in the future, rather than
the existing dual use (which is used by several consumers, but
consolidated into qemuOpenFile(). The problem is that it uses a
different mode depending on whether it opens the file as root, or as the
qemu user, and although that usage doesn't force the file permissions
when opening an existing file, it would end up producing different
results when creating a new file (with the existing code, a file created
w/o forking would be mode 600 and one requiring fork+setuid would be
created with mod 660). To reduce it to a single call, we would need to
choose either 600 or 660 for both cases - for security reasons, we
wouldn't want to choose 660; 600 would be okay as long as nobody was
trying to access the newly created file outside of libvirt or qemu, and
trying to do so from a process running with group qemu, but some
different user - that's the thing that I mentioned needs at least some
discussion before changing behavior.
>> (NB: currently qemuOpenFile() calls virFileOpenAs() twice, once
>> without forking, then again with forking. That unfortunately can't
>> be changed without at least some discussion of the ramifications,
>> because the requested file permissions are different in each case,
>> which is something that a single call to virFileOpenAs() can't deal
>> with.)
> For a pre-existing file, all that matters is that you get an fd open; it
> doesn't matter who opened it. (Unless we really _do_ want to change
> ownerships as part of opening a file.)
>
> For creating a new file, it matters that the resulting fd is owned by
> the correct user. If root creates the file, but the file will be passed
> to qemu, then we must change ownership (fchmod or grant an ACL); or we
> must open the file as qemu in the first place. The former will be
> prevented by root-squash NFS, so NFS already has the right behavior;
That is all true, as long as nobody expects to access the file outside
libvirt+qemu.
> anywhere else, root is powerful enough to be able to fchown the file
> over to the right user.
>
> I guess I'll see more on this as I continue reviewing.
>
>> * Add a flag so that any fchown() of the file to a different uid:gid
>> is explicitly requested when the function is called, rather than it
>> being implied by the presence of the O_CREAT flag. This just makes
>> for less subtle surprises to consumers.
> Yes, I think that makes sense. We really are doing two things at once:
> 'give me an fd', and 'ensure that the right people down the road can
> call open and get a new fd to the same file'; separating the fchown and
> making it only happen when explicitly requested means that by default we
> just get the fd.
>
> Callers passing O_CREAT | O_TRUNC don't care if the file previously
> existed, so they probably won't pass the ownership flag. Callers
> passing O_CREAT | O_EXCL are trying to create a new file, so they
> probably care about ownership on the new file. But making them be
> explicit about it makes it easier to review.
>
>> (Commit
>> b1643dc15c5de886fefe56ad18608d65f1325a2c added the check for O_CREAT
>> before forcing ownership. This patch just makes that restriction
>> more explicit.)
>>
>> * If either the uid or gid is specified as "-1", virFileOpenAs will
>> interpret this to mean "the current [gu]id".
>>
>> All current consumers of virFileOpenAs should retain their present
>> behavior (after a few minor changes to their setup code and
>> arguments).
>> ---
>> src/libxl/libxl_driver.c | 4 +-
>> src/qemu/qemu_driver.c | 8 +-
>> src/storage/storage_backend.c | 12 +-
>> src/util/util.c | 341 ++++++++++++++++++++++++-----------------
>> src/util/util.h | 6 +-
>> 5 files changed, 217 insertions(+), 154 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 0500ed0..d7325c3 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -216,7 +216,7 @@ libxlSaveImageOpen(libxlDriverPrivatePtr driver, const char *from,
>> libxlSavefileHeader hdr;
>> char *xml = NULL;
>>
>> - if ((fd = virFileOpenAs(from, O_RDONLY, 0, getuid(), getgid(), 0))< 0) {
>> + if ((fd = virFileOpenAs(from, O_RDONLY, 0, -1, -1, 0))< 0) {
> Based solely on the commit message, this is a valid conversion (I guess
> when I get to the actual function, to see if the new implementation
> matches the commit, determines whether I have to change my mind :)
>
>> @@ -1827,7 +1827,7 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>> }
>>
>> if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR,
>> - getuid(), getgid(), 0))< 0) {
>> + -1, -1, 0))< 0) {
> Here, we're creating a file, but I guess we're okay if it ends up being
> owned by root and not some other user.
That's actually a NOP change - previously it sent getuid() as the user,
now it sends -1, which is translated to getuid() by virFileOpenAs.
>> +++ b/src/qemu/qemu_driver.c
>> @@ -2306,6 +2306,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>> bool is_reg = true;
>> bool need_unlink = false;
>> bool bypass_security = false;
>> + unsigned int vfoflags = 0;
>> int fd = -1;
>> uid_t uid = getuid();
>> gid_t gid = getgid();
>> @@ -2315,6 +2316,7 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>> * in the failure case */
>> if (oflags& O_CREAT) {
>> need_unlink = true;
>> + vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
> Per the commit message, this is preserving semantics of the old behavior.
>
>> if (stat(path,&sb) == 0) {
>> is_reg = !!S_ISREG(sb.st_mode);
>> /* If the path is regular file which exists
>> @@ -2335,8 +2337,8 @@ qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
>> goto cleanup;
>> }
>> } 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
>> @@ -2380,7 +2382,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) {
> And this keeps things as two calls for now, for minimal code churn in
> the clients, so that this patch focuses on the implementation (I'm
> guessing 2/2 simplifies the clients).
Actually (as you'll find out) 2/2 just uses virFileOpenAs in a new place
to fix a bug. Reducing the usage in qemuOpenFile() to a single call was
left alone due to the issue outlined above. I'm starting to feel more
brave about making that change, though :-) (Maybe now is the time to
make it, giving us more testing time to see if it breaks anyone's usage).
>> @@ -393,15 +391,15 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
>> goto cleanup;
>> }
>>
>> - uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
>> - gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
>> - operation_flags = VIR_FILE_OPEN_FORCE_PERMS;
>> + operation_flags = VIR_FILE_OPEN_FORCE_MODE | VIR_FILE_OPEN_FORCE_OWNER;
>> if (pool->def->type == VIR_STORAGE_POOL_NETFS)
>> - operation_flags |= VIR_FILE_OPEN_AS_UID;
>> + operation_flags |= VIR_FILE_OPEN_FORK;
>>
>> if ((fd = virFileOpenAs(vol->target.path,
>> O_RDWR | O_CREAT | O_EXCL,
>> - vol->target.perms.mode, uid, gid,
>> + vol->target.perms.mode,
>> + vol->target.perms.uid,
>> + vol->target.perms.gid,
> Hmm, we still have the problem that we are storing _virStoragePerms as
> int mode, int uid, and int gid, instead of the better mode_t mode, uid_t
> uid, and gid_t gid (and given our recent problems with 64-bit pid_t,
> it's not too much of a stretch to imagine a system with 64-bit uid_t,
> even though mingw64 is thankfully not such a system). But that's a
> separate cleanup, and I'll probably be working on it in parallel with my
> pid_t cleanups. So I'm okay with this change.
>
>>
>> -/* 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
> I'm going to review this more on the basis of post-patch, rather than on
> the diff (as you mentioned, it is enough of a change to make the diff
> difficult). But unfortunately, I've run out of time at the moment; I'll
> see if I can get back to the review later tonight, otherwise it will be
> tomorrow morning.
You'll probably notice what I just noticed:
- ret = virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
- if (ret< 0)
+ if ((fd = open(path, openflags, mode))< 0) {
+ ret = --errno;
goto childerror;
- fd = ret;
+ }
So it will return EACCES-1 instead of -EACCES :-P
>
>> +++ b/src/util/util.h
>> @@ -90,8 +90,10 @@ char *virFileSanitizePath(const char *path);
>>
>> enum {
>> VIR_FILE_OPEN_NONE = 0,
>> - VIR_FILE_OPEN_AS_UID = (1<< 0),
>> - VIR_FILE_OPEN_FORCE_PERMS = (1<< 1),
>> + VIR_FILE_OPEN_NOFORK = (1<< 0),
>> + VIR_FILE_OPEN_FORK = (1<< 1),
>> + VIR_FILE_OPEN_FORCE_MODE = (1<< 2),
>> + VIR_FILE_OPEN_FORCE_OWNER = (1<< 3),
> The new flag names make sense. It looked like in this round, you used
> NOFORK and FORK as mutually exclusive flags; I'm guessing that the next
> patch mixes the two as the way to state that you want to use a
> double-open attempt,
Correct. And if you specify neither, it equates that to having set them
both, just to make the most common use case simpler to write.
> And to some degree, FORCE_PERMS maps to
> FORCE_MODE, and AS_UID maps to FORCE_OWNER, but with clearer semantics -
AS_UID == FORK, FORCE_OWNER is a separate issue that was previously
implied by setting O_CREAT.
> if the flag is present, guarantee an fchmod() or fchown(), whether or
> not a file was created; if the flag is not present, focus only on
> getting the fd.
Correct.
More information about the libvir-list
mailing list