* nfsd bug: create file with specific uid/gid
@ 2007-11-30 17:06 Roger Willcocks
2007-11-30 22:01 ` J. Bruce Fields
0 siblings, 1 reply; 4+ messages in thread
From: Roger Willcocks @ 2007-11-30 17:06 UTC (permalink / raw)
To: linux-kernel
nfsd/vfs.c:nfsd_create (the v2 version of create) says:
"Set file attributes. Mode has already been set and
setting uid/gid works only for root"
but it doesn't actually test for root-ness (which could happen if the
access is no-root-squash). There's similar code without the comment in
nfsd_create_v3. In both cases the test:
if ((iap->ia_valid &= ~(ATTR_UID|ATTR_GID|ATTR_MODE)) != 0)
should read:
if (current->fsuid != 0)
iap->ia_valid &= ~(ATTR_UID|ATTR_GID);
if ((iap->ia_valid &= ~ATTR_MODE) != 0)
although arguably they should return an EPERM error if the uid/gid bits
are set, instead of silently ignoring them.
--
Roger
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: nfsd bug: create file with specific uid/gid 2007-11-30 17:06 nfsd bug: create file with specific uid/gid Roger Willcocks @ 2007-11-30 22:01 ` J. Bruce Fields 2007-12-01 11:36 ` Roger Willcocks 0 siblings, 1 reply; 4+ messages in thread From: J. Bruce Fields @ 2007-11-30 22:01 UTC (permalink / raw) To: Roger Willcocks; +Cc: linux-kernel, nfs, Neil Brown On Fri, Nov 30, 2007 at 05:06:03PM +0000, Roger Willcocks wrote: > nfsd/vfs.c:nfsd_create (the v2 version of create) says: > > "Set file attributes. Mode has already been set and > setting uid/gid works only for root" > > but it doesn't actually test for root-ness (which could happen if the > access is no-root-squash). There's similar code without the comment in > nfsd_create_v3. In both cases the test: > > if ((iap->ia_valid &= ~(ATTR_UID|ATTR_GID|ATTR_MODE)) != 0) > > should read: > > if (current->fsuid != 0) > iap->ia_valid &= ~(ATTR_UID|ATTR_GID); > if ((iap->ia_valid &= ~ATTR_MODE) != 0) > > although arguably they should return an EPERM error if the uid/gid bits are > set, instead of silently ignoring them. Assignments (especially with things like &=) inside of conditionals always make my head hurt for some reason. So maybe something like the below? Although I'd be happier if we could get a comment from someone with a better understanding of why this hack was added in the first place. Thanks for the bug report! (And, by the way, how did you run across this?) --b. commit 38574420b8992d69f469ca86041f75b6e2283174 Author: J. Bruce Fields <bfields@citi.umich.edu> Date: Fri Nov 30 16:55:23 2007 -0500 nfsd: allow root to set uid and gid on create The server silently ignores attempts to set the uid and gid on create. Based on the comment, this appears to have been done to prevent some overly-clever IRIX client from causing itself problems. Perhaps we should remove that hack completely. For now, at least, it makes sense to allow root (when no_root_squash is set) to set uid and gid. Thanks to Roger Willcocks <roger@filmlight.ltd.uk> for the bug report and original patch on which this is based. Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 755ba43..8a8bf06 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1257,12 +1257,16 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, } - /* Set file attributes. Mode has already been set and - * setting uid/gid works only for root. Irix appears to + /* Mode has already been set: */ + iap->ia_valid &= ~ATTR_MODE; + /* + * Setting uid/gid works only for root. Irix appears to * send along the gid when it tries to implement setgid - * directories via NFS. + * directories via NFS: */ - if ((iap->ia_valid &= ~(ATTR_UID|ATTR_GID|ATTR_MODE)) != 0) { + if (current->fsuid != 0) + iap->ia_valid &= ~(ATTR_UID|ATTR_GID); + if (iap->ia_valid) { __be32 err2 = nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0); if (err2) err = err2; ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: nfsd bug: create file with specific uid/gid 2007-11-30 22:01 ` J. Bruce Fields @ 2007-12-01 11:36 ` Roger Willcocks 2007-12-03 16:17 ` J. Bruce Fields 0 siblings, 1 reply; 4+ messages in thread From: Roger Willcocks @ 2007-12-01 11:36 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-kernel, nfs, Neil Brown J. Bruce Fields wrote: > On Fri, Nov 30, 2007 at 05:06:03PM +0000, Roger Willcocks wrote: >> nfsd/vfs.c:nfsd_create (the v2 version of create) says: >> >> "Set file attributes. Mode has already been set and >> setting uid/gid works only for root" >> >> but it doesn't actually test for root-ness ... > Assignments (especially with things like &=) inside of conditionals > always make my head hurt for some reason. So maybe something like the > below? The patch looks fine to me, note that nfsd_create_v3 needs fixing too. > > Thanks for the bug report! (And, by the way, how did you run across > this?) > I'm working on an NFSp-like server for our clustered file system and my data file creates were not working as expected. -- Roger ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: nfsd bug: create file with specific uid/gid 2007-12-01 11:36 ` Roger Willcocks @ 2007-12-03 16:17 ` J. Bruce Fields 0 siblings, 0 replies; 4+ messages in thread From: J. Bruce Fields @ 2007-12-03 16:17 UTC (permalink / raw) To: Roger Willcocks; +Cc: linux-kernel, nfs, Neil Brown On Sat, Dec 01, 2007 at 11:36:18AM -0000, Roger Willcocks wrote: > J. Bruce Fields wrote: > >> On Fri, Nov 30, 2007 at 05:06:03PM +0000, Roger Willcocks wrote: >>> nfsd/vfs.c:nfsd_create (the v2 version of create) says: >>> >>> "Set file attributes. Mode has already been set and >>> setting uid/gid works only for root" >>> >>> but it doesn't actually test for root-ness > ... >> Assignments (especially with things like &=) inside of conditionals >> always make my head hurt for some reason. So maybe something like the >> below? > > The patch looks fine to me, note that nfsd_create_v3 needs fixing too. Whoops. OK, in that case we should be call the same logic from both places. New (compile-tested only) patch appended. >> Thanks for the bug report! (And, by the way, how did you run across >> this?) >> > > I'm working on an NFSp-like server for our clustered file system and my > data file creates were not working as expected. OK, thanks. (Might be interesting to hear more about the architecture if there's a description published someplace.) --b. >From 7eed8a82a42e7a1c1e63c3cbc70be7321e69d9ff Mon Sep 17 00:00:00 2001 From: J. Bruce Fields <bfields@citi.umich.edu> Date: Fri, 30 Nov 2007 16:55:23 -0500 Subject: [PATCH] nfsd: allow root to set uid and gid on create The server silently ignores attempts to set the uid and gid on create. Based on the comment, this appears to have been done to prevent some overly-clever IRIX client from causing itself problems. Perhaps we should remove that hack completely. For now, at least, it makes sense to allow root (when no_root_squash is set) to set uid and gid. While we're there, since nfsd_create and nfsd_create_v3 share the same logic, pull that out into a separate function. And spell out the individual modifications of ia_valid instead of doing them both at once inside a conditional. Thanks to Roger Willcocks <roger@filmlight.ltd.uk> for the bug report and original patch on which this is based. Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu> --- fs/nfsd/vfs.c | 47 ++++++++++++++++++++++++++++------------------- 1 files changed, 28 insertions(+), 19 deletions(-) diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 755ba43..cc75e4f 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1151,6 +1151,26 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp, } #endif /* CONFIG_NFSD_V3 */ +__be32 +nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *resfhp, + struct iattr *iap) +{ + /* + * Mode has already been set earlier in create: + */ + iap->ia_valid &= ~ATTR_MODE; + /* + * Setting uid/gid works only for root. Irix appears to + * send along the gid on create when it tries to implement + * setgid directories via NFS: + */ + if (current->fsuid != 0) + iap->ia_valid &= ~(ATTR_UID|ATTR_GID); + if (iap->ia_valid) + return nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0); + return 0; +} + /* * Create a file (regular, directory, device, fifo); UNIX sockets * not yet implemented. @@ -1167,6 +1187,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, struct dentry *dentry, *dchild = NULL; struct inode *dirp; __be32 err; + __be32 err2; int host_err; err = nfserr_perm; @@ -1257,16 +1278,9 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, } - /* Set file attributes. Mode has already been set and - * setting uid/gid works only for root. Irix appears to - * send along the gid when it tries to implement setgid - * directories via NFS. - */ - if ((iap->ia_valid &= ~(ATTR_UID|ATTR_GID|ATTR_MODE)) != 0) { - __be32 err2 = nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0); - if (err2) - err = err2; - } + err2 = nfsd_create_setattr(rqstp, resfhp, iap); + if (err2) + err = err2; /* * Update the file handle to get the new inode info. */ @@ -1295,6 +1309,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp, struct dentry *dentry, *dchild = NULL; struct inode *dirp; __be32 err; + __be32 err2; int host_err; __u32 v_mtime=0, v_atime=0; @@ -1399,16 +1414,10 @@ nfsd_create_v3(struct svc_rqst *rqstp, struct svc_fh *fhp, iap->ia_atime.tv_nsec = 0; } - /* Set file attributes. - * Irix appears to send along the gid when it tries to - * implement setgid directories via NFS. Clear out all that cruft. - */ set_attr: - if ((iap->ia_valid &= ~(ATTR_UID|ATTR_GID|ATTR_MODE)) != 0) { - __be32 err2 = nfsd_setattr(rqstp, resfhp, iap, 0, (time_t)0); - if (err2) - err = err2; - } + err2 = nfsd_create_setattr(rqstp, resfhp, iap); + if (err2) + err = err2; /* * Update the filehandle to get the new inode info. -- 1.5.3.5.561.g140d ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-12-03 16:17 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-11-30 17:06 nfsd bug: create file with specific uid/gid Roger Willcocks 2007-11-30 22:01 ` J. Bruce Fields 2007-12-01 11:36 ` Roger Willcocks 2007-12-03 16:17 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).