On 6/5/2015 4:27 AM, J. Bruce Fields wrote: > On Thu, Jun 04, 2015 at 08:43:14PM +0800, Kinglong Mee wrote: >> On 6/3/2015 12:09 AM, Stanislav Kholmanskikh wrote: >>> On 06/02/2015 12:23 AM, bfields@xxxxxxxxxxxx wrote: >>>> On Mon, Jun 01, 2015 at 06:01:02PM +0300, Stanislav Kholmanskikh wrote: >>>>> Hello. >>>>> >>>>> As the man page for utime/utimes states [1], EPERM is returned if >>>>> the second argument of utime/utimes is not NULL and: >>>>> * the caller's effective user id does not match the owner of the file >>>>> * the caller does not have write access to the file >>>>> * the caller is not privileged >>>>> >>>>> However, I don't see this behavior with NFS, I see EACCES is >>>>> generated instead. >>>> >>>> Agreed that it's probably a server bug. (Have you run across a case >>>> where this makes a difference?) >>> >>> Thank you. >>> >>> No, I've not seen such a real-word scenario. >>> >>> I have these LTP test cases failing: >>> >>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utime/utime06.c >>> * https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/utimes/utimes01.c >>> >>> and it makes me a bit nervous :) >>> >>>> >>>> Looking at nfsd_setattr().... The main work is done by notify_change(), >>>> which is probably doing the right thing. But before that there's an >>>> fh_verify()--looks like that is expected to fail in your case. I bet >>>> that's the cause. >> >> Yes, it is. >> >> nfsd do the permission checking before notify_change() as, >> >> /* This assumes NFSD_MAY_{READ,WRITE,EXEC} == MAY_{READ,WRITE,EXEC} */ >> err = inode_permission(inode, acc & (MAY_READ|MAY_WRITE|MAY_EXEC)); >> >> return -EACCES for non-owner user. >> >>> >>> I doubt I can fix it by myself (at least quickly). So I am happy if anyone more experienced will look at it as well :) >>> >>> Anyway, if nobody is interested, I'll give it a try, but later. >> >> Here is a diff patch for this problem, please try testing. >> If okay, I will send an official patch. >> >> Note: must apply the following patch first in the url, >> http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=cc265089ce1b176dde963c74b53593446ee7f99a > > We could do that if we have to. > > I wonder if we could instead skip the fh_verify's inode_permission call > entirely? Most callers of fh_verify don't need the inode_permission > call at all as far as I can tell, because the following vfs operation > does permission checking already. > > We still need some nfs-specific checking, I guess (e.g. to make sure we > aren't allowing setattr on a read-only export of a writeable mount), but > the inode_permission itself I think we can usually skip. I have made a draft for fh_verify without inode_permisson as following. 1. rename the check_nfsd_access() to rqst_check_access() that only checking the request's flavor. (Maybe a split patch is better) 2. split a new helper nfsd_check_access() from nfsd_permission() for checking nfsd access. 3. nfsd_permission() is not changed by call nfsd_check_access(). 4. let fh_verify() call nfsd_check_access(), not nfsd_permission(). 5. add nfsd_permission() for do_open_permission(). I just test with pynfs, and the result is same as the old. > > Andreas Gruenbacher has also been complaining that the redundant > inode_permission calls make the richacl work more difficult, I can't > remember why exactly. I will check those patch and discuss of richacl, but maybe later. thanks, Kinglong Mee ----------------------------------------------------------------------------- diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c index f79521a..3199fec 100644 --- a/fs/nfsd/export.c +++ b/fs/nfsd/export.c @@ -935,7 +935,7 @@ static struct svc_export *exp_find(struct cache_detail *cd, return exp; } -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp) +__be32 rqst_check_access(struct svc_export *exp, struct svc_rqst *rqstp) { struct exp_flavor_info *f; struct exp_flavor_info *end = exp->ex_flavors + exp->ex_nflavors; diff --git a/fs/nfsd/export.h b/fs/nfsd/export.h index 1f52bfc..20a50c5 100644 --- a/fs/nfsd/export.h +++ b/fs/nfsd/export.h @@ -80,7 +80,7 @@ struct svc_expkey { #define EX_WGATHER(exp) ((exp)->ex_flags & NFSEXP_GATHERED_WRITES) int nfsexp_flags(struct svc_rqst *rqstp, struct svc_export *exp); -__be32 check_nfsd_access(struct svc_export *exp, struct svc_rqst *rqstp); +__be32 rqst_check_access(struct svc_export *exp, struct svc_rqst *rqstp); /* * Function declarations diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index 864e200..d85d620 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -203,8 +203,11 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs accmode |= NFSD_MAY_WRITE; status = fh_verify(rqstp, current_fh, S_IFREG, accmode); + if (status) + return status; - return status; + return nfsd_permission(rqstp, current_fh->fh_export, + current_fh->fh_dentry, accmode); } static __be32 nfsd_check_obj_isreg(struct svc_fh *fh) @@ -1708,7 +1711,7 @@ nfsd4_proc_compound(struct svc_rqst *rqstp, clear_current_stateid(cstate); if (need_wrongsec_check(rqstp)) - op->status = check_nfsd_access(current_fh->fh_export, rqstp); + op->status = rqst_check_access(current_fh->fh_export, rqstp); } encode_op: diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c index 158badf..33c6c14 100644 --- a/fs/nfsd/nfs4xdr.c +++ b/fs/nfsd/nfs4xdr.c @@ -2843,7 +2843,7 @@ nfsd4_encode_dirent_fattr(struct xdr_stream *xdr, struct nfsd4_readdir *cd, nfserr = nfserrno(err); goto out_put; } - nfserr = check_nfsd_access(exp, cd->rd_rqstp); + nfserr = rqst_check_access(exp, cd->rd_rqstp); if (nfserr) goto out_put; diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index 350041a..f3b04e5 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -360,13 +360,13 @@ fh_verify(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, int access) && exp->ex_path.dentry == dentry) goto skip_pseudoflavor_check; - error = check_nfsd_access(exp, rqstp); + error = rqst_check_access(exp, rqstp); if (error) goto out; skip_pseudoflavor_check: /* Finally, check access permissions. */ - error = nfsd_permission(rqstp, exp, dentry, access); + error = nfsd_check_access(rqstp, exp, dentry, access); if (error) { dprintk("fh_verify: %pd2 permission failure, " diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 84d770b..5a12d2d 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -262,7 +262,7 @@ nfsd_lookup(struct svc_rqst *rqstp, struct svc_fh *fhp, const char *name, err = nfsd_lookup_dentry(rqstp, fhp, name, len, &exp, &dentry); if (err) return err; - err = check_nfsd_access(exp, rqstp); + err = rqst_check_access(exp, rqstp); if (err) goto out; /* @@ -2012,11 +2012,10 @@ static int exp_rdonly(struct svc_rqst *rqstp, struct svc_export *exp) * Check for a user's access permissions to this inode. */ __be32 -nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp, +nfsd_check_access(struct svc_rqst *rqstp, struct svc_export *exp, struct dentry *dentry, int acc) { struct inode *inode = d_inode(dentry); - int err; if ((acc & NFSD_MAY_MASK) == NFSD_MAY_NOP) return 0; @@ -2052,6 +2051,18 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp, } if ((acc & NFSD_MAY_TRUNC) && IS_APPEND(inode)) return nfserr_perm; + return 0; +} +__be32 +nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp, + struct dentry *dentry, int acc) +{ + struct inode *inode = d_inode(dentry); + int err; + + err = nfsd_check_access(rqstp, exp, dentry, acc); + if (err) + return err; if (acc & NFSD_MAY_LOCK) { /* If we cannot rely on authentication in NLM requests, diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index 2050cb0..e3e8eed 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -101,6 +101,8 @@ __be32 nfsd_readdir(struct svc_rqst *, struct svc_fh *, __be32 nfsd_statfs(struct svc_rqst *, struct svc_fh *, struct kstatfs *, int access); +__be32 nfsd_check_access(struct svc_rqst *, struct svc_export *, + struct dentry *, int); __be32 nfsd_permission(struct svc_rqst *, struct svc_export *, struct dentry *, int); -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
