| Message ID | 20170728175452.49057-1-catalin.marinas@arm.com |
|---|---|
| State | New |
| Headers | show |
Anna, Trond, On Fri, Jul 28, 2017 at 06:54:52PM +0100, Catalin Marinas wrote: > --- a/fs/nfs/nfs4proc.c > +++ b/fs/nfs/nfs4proc.c > @@ -2236,7 +2236,7 @@ static int nfs4_opendata_access(struct rpc_cred *cred, > int openflags) > { > struct nfs_access_entry cache; > - u32 mask; > + int mask, cache_mask; > > /* access call failed or for some reason the server doesn't > * support any access modes -- defer access call until later */ > @@ -2259,7 +2259,8 @@ static int nfs4_opendata_access(struct rpc_cred *cred, > nfs_access_set_mask(&cache, opendata->o_res.access_result); > nfs_access_add_cache(state->inode, &cache); > > - if ((mask & ~cache.mask & (MAY_READ | MAY_EXEC)) == 0) > + cache_mask = nfs_access_calc_mask(cache.mask, state->inode->i_mode); > + if ((mask & ~cache_mask & (MAY_READ | MAY_EXEC)) == 0) > return 0; > > return -EACCES; I noticed -rc3 already has a fix here, commit 1e6f209515a0 ("NFS: Use raw NFS access mask in nfs4_opendata_access()"), without having to export nfs_access_calc_mask(). > @@ -3870,25 +3871,9 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry > .rpc_resp = &res, > .rpc_cred = entry->cred, > }; > - int mode = entry->mask; > int status = 0; > > - /* > - * Determine which access bits we want to ask for... > - */ > - if (mode & MAY_READ) > - args.access |= NFS4_ACCESS_READ; > - if (S_ISDIR(inode->i_mode)) { > - if (mode & MAY_WRITE) > - args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND | NFS4_ACCESS_DELETE; > - if (mode & MAY_EXEC) > - args.access |= NFS4_ACCESS_LOOKUP; > - } else { > - if (mode & MAY_WRITE) > - args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND; > - if (mode & MAY_EXEC) > - args.access |= NFS4_ACCESS_EXECUTE; > - } > + args.access = entry->mask; However, AFAICT, 'entry->mask' already uses the NFS4_* bits, so checking 'mode' against MAY_READ etc. to build up args.access is incorrect. Is this hunk still needed? -- Catalin
Hi Catalin, On 07/31/2017 07:19 AM, Catalin Marinas wrote: > Anna, Trond, > > On Fri, Jul 28, 2017 at 06:54:52PM +0100, Catalin Marinas wrote: >> --- a/fs/nfs/nfs4proc.c >> +++ b/fs/nfs/nfs4proc.c >> @@ -2236,7 +2236,7 @@ static int nfs4_opendata_access(struct rpc_cred *cred, >> int openflags) >> { >> struct nfs_access_entry cache; >> - u32 mask; >> + int mask, cache_mask; >> >> /* access call failed or for some reason the server doesn't >> * support any access modes -- defer access call until later */ >> @@ -2259,7 +2259,8 @@ static int nfs4_opendata_access(struct rpc_cred *cred, >> nfs_access_set_mask(&cache, opendata->o_res.access_result); >> nfs_access_add_cache(state->inode, &cache); >> >> - if ((mask & ~cache.mask & (MAY_READ | MAY_EXEC)) == 0) >> + cache_mask = nfs_access_calc_mask(cache.mask, state->inode->i_mode); >> + if ((mask & ~cache_mask & (MAY_READ | MAY_EXEC)) == 0) >> return 0; >> >> return -EACCES; > > I noticed -rc3 already has a fix here, commit 1e6f209515a0 ("NFS: Use > raw NFS access mask in nfs4_opendata_access()"), without having to > export nfs_access_calc_mask(). > >> @@ -3870,25 +3871,9 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry >> .rpc_resp = &res, >> .rpc_cred = entry->cred, >> }; >> - int mode = entry->mask; >> int status = 0; >> >> - /* >> - * Determine which access bits we want to ask for... >> - */ >> - if (mode & MAY_READ) >> - args.access |= NFS4_ACCESS_READ; >> - if (S_ISDIR(inode->i_mode)) { >> - if (mode & MAY_WRITE) >> - args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND | NFS4_ACCESS_DELETE; >> - if (mode & MAY_EXEC) >> - args.access |= NFS4_ACCESS_LOOKUP; >> - } else { >> - if (mode & MAY_WRITE) >> - args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND; >> - if (mode & MAY_EXEC) >> - args.access |= NFS4_ACCESS_EXECUTE; >> - } >> + args.access = entry->mask; > > However, AFAICT, 'entry->mask' already uses the NFS4_* bits, so checking > 'mode' against MAY_READ etc. to build up args.access is incorrect. Is > this hunk still needed? > I think the code is okay short term, because all the bits it's checking against are set in entry->mask whenever we enter this function. Long term, yes it should be updated. I have patches to do this in a generic way that can be used for both NFS v3 and NFS v4, but they'll probably go into 4.14 Thanks, Anna
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 3522b1249019..828a04924770 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -2381,7 +2381,7 @@ EXPORT_SYMBOL_GPL(nfs_access_add_cache); #define NFS_DIR_MAY_WRITE NFS_MAY_WRITE #define NFS_MAY_LOOKUP (NFS4_ACCESS_LOOKUP) #define NFS_MAY_EXECUTE (NFS4_ACCESS_EXECUTE) -static int +int nfs_access_calc_mask(u32 access_result, umode_t umode) { int mask = 0; @@ -2402,6 +2402,7 @@ nfs_access_calc_mask(u32 access_result, umode_t umode) mask |= MAY_WRITE; return mask; } +EXPORT_SYMBOL_GPL(nfs_access_calc_mask); void nfs_access_set_mask(struct nfs_access_entry *entry, u32 access_result) { diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index a0b4e1091340..b53b6ec21126 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -2236,7 +2236,7 @@ static int nfs4_opendata_access(struct rpc_cred *cred, int openflags) { struct nfs_access_entry cache; - u32 mask; + int mask, cache_mask; /* access call failed or for some reason the server doesn't * support any access modes -- defer access call until later */ @@ -2259,7 +2259,8 @@ static int nfs4_opendata_access(struct rpc_cred *cred, nfs_access_set_mask(&cache, opendata->o_res.access_result); nfs_access_add_cache(state->inode, &cache); - if ((mask & ~cache.mask & (MAY_READ | MAY_EXEC)) == 0) + cache_mask = nfs_access_calc_mask(cache.mask, state->inode->i_mode); + if ((mask & ~cache_mask & (MAY_READ | MAY_EXEC)) == 0) return 0; return -EACCES; @@ -3870,25 +3871,9 @@ static int _nfs4_proc_access(struct inode *inode, struct nfs_access_entry *entry .rpc_resp = &res, .rpc_cred = entry->cred, }; - int mode = entry->mask; int status = 0; - /* - * Determine which access bits we want to ask for... - */ - if (mode & MAY_READ) - args.access |= NFS4_ACCESS_READ; - if (S_ISDIR(inode->i_mode)) { - if (mode & MAY_WRITE) - args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND | NFS4_ACCESS_DELETE; - if (mode & MAY_EXEC) - args.access |= NFS4_ACCESS_LOOKUP; - } else { - if (mode & MAY_WRITE) - args.access |= NFS4_ACCESS_MODIFY | NFS4_ACCESS_EXTEND; - if (mode & MAY_EXEC) - args.access |= NFS4_ACCESS_EXECUTE; - } + args.access = entry->mask; res.fattr = nfs_alloc_fattr(); if (res.fattr == NULL) diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h index 5cc91d6381a3..792fd5049e91 100644 --- a/include/linux/nfs_fs.h +++ b/include/linux/nfs_fs.h @@ -340,6 +340,7 @@ extern int nfs_post_op_update_inode_force_wcc_locked(struct inode *inode, struct extern int nfs_getattr(const struct path *, struct kstat *, u32, unsigned int); extern void nfs_access_add_cache(struct inode *, struct nfs_access_entry *); extern void nfs_access_set_mask(struct nfs_access_entry *, u32); +extern int nfs_access_calc_mask(u32 access_result, umode_t umode); extern int nfs_permission(struct inode *, int); extern int nfs_open(struct inode *, struct file *); extern int nfs_attribute_cache_expired(struct inode *inode);
Commit bd8b2441742b ("NFS: Store the raw NFS access mask in the inode's access cache") changed the way access mask is stored in struct nfs_access_entry. However, there a are couple of places NFSv4 code where it still uses the generic access bits instead of the raw NFS ones (e.g. MAY_READ instead of NFS_MAY_READ). Fixes: bd8b2441742b ("NFS: Store the raw NFS access mask in the inode's access cache") Cc: Trond Myklebust <trond.myklebust@primarydata.com> Cc: Anna Schumaker <Anna.Schumaker@Netapp.com> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> --- Hi Trond, I noticed with 4.13-rc2 on an arm64 system that I get random EPERM errors with an NFS root filesystem (only tried v4). The patch below appears to fix the problem, as is reverting the above commit and the next one in your series. Looking at nfs3_proc_access(), it appears it has the same problem. Happy to fix that as well if you think that's the right way. Thanks. fs/nfs/dir.c | 3 ++- fs/nfs/nfs4proc.c | 23 ++++------------------- include/linux/nfs_fs.h | 1 + 3 files changed, 7 insertions(+), 20 deletions(-)