On Fri, Jul 08, 2016 at 05:53:19PM -0400, Oleg Drokin wrote: > > On Jul 8, 2016, at 4:54 PM, J. Bruce Fields wrote: > > > On Thu, Jul 07, 2016 at 09:47:46PM -0400, Oleg Drokin wrote: > >> It looks like we are bit overzealous about failing mkdir/create/mknod > >> with permission denied if the parent dir is not writeable. > >> Need to make sure the name does not exist first, because we need to > >> return EEXIST in that case. > >> > >> Signed-off-by: Oleg Drokin <gr...@linuxhacker.ru> > >> --- > >> A very similar problem exists with symlinks, but the patch is more > >> involved, so assuming this one is ok, I'll send a symlink one separately. > >> fs/nfsd/nfs4proc.c | 6 +++++- > >> fs/nfsd/vfs.c | 11 ++++++++++- > >> 2 files changed, 15 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > >> index de1ff1d..0067520 100644 > >> --- a/fs/nfsd/nfs4proc.c > >> +++ b/fs/nfsd/nfs4proc.c > >> @@ -605,8 +605,12 @@ nfsd4_create(struct svc_rqst *rqstp, struct > >> nfsd4_compound_state *cstate, > >> > >> fh_init(&resfh, NFS4_FHSIZE); > >> > >> + /* > >> + * We just check thta parent is accessible here, nfsd_* do their > >> + * own access permission checks > >> + */ > >> status = fh_verify(rqstp, &cstate->current_fh, S_IFDIR, > >> - NFSD_MAY_CREATE); > >> + NFSD_MAY_EXEC); > >> if (status) > >> return status; > >> > >> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > >> index 6fbd81e..6a45ec6 100644 > >> --- a/fs/nfsd/vfs.c > >> +++ b/fs/nfsd/vfs.c > >> @@ -1161,7 +1161,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh > >> *fhp, > >> if (isdotent(fname, flen)) > >> goto out; > >> > >> - err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_CREATE); > >> + /* > >> + * Even though it is a create, first we see if we are even allowed > >> + * to peek inside the parent > >> + */ > >> + err = fh_verify(rqstp, fhp, S_IFDIR, NFSD_MAY_EXEC); > > > > Looks like in the v3 case we haven't actually locked the directory yet > > at this point so this check is a little race-prone. > > In reality this check is not really needed, I suspect. > When we call vfs_create/mknod/mkdir later on, it has it's own permission check > anyway so if there was a race and somebody changed dir access in the middle, > there's going to be another check anyway and it would be caught. > Unless there's some weird server-side permission wiggling as well that makes > it > ineffective, but I imagine that one cannot really change in a racy way?
Yeah, I think I'll just change those NFSD_MAY_EXEC's to NFSD_MAY_NOP's. We still need the fh_verify there since it's also what does the filehandle->dentry translation, but we don't need permission checking here yet. Applying with that one change. (And I'll followup with some additional minor cleanup of the create code.) --b. > > > I wonder why the code's structured that way--it's confusing. > > Probably years of accumulated "damage" ;) > > > --b. > > > >> if (err) > >> goto out; > >> > >> @@ -1211,6 +1215,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh > >> *fhp, > >> goto out; > >> } > >> > >> + /* Now let's see if we actually have permissions to create */ > >> + err = nfsd_permission(rqstp, fhp->fh_export, dentry, NFSD_MAY_CREATE); > >> + if (err) > >> + goto out; > >> + > >> if (!(iap->ia_valid & ATTR_MODE)) > >> iap->ia_mode = 0; > >> iap->ia_mode = (iap->ia_mode & S_IALLUGO) | type; > >> -- > >> 2.7.4
