Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: [PATCH 1/2] NFS: change nfs_access_get_cached() to
 nfs_access_check_cached()
From:   NeilBrown <neilb@suse.de>
To:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>
Cc:     linux-nfs@vger.kernel.org
Date:   Thu, 28 Apr 2022 11:37:32 +1000
Message-ID: <165110985230.7595.10676160398710495817.stgit@noble.brown>
In-Reply-To: <165110909570.7595.8578730126480600782.stgit@noble.brown>
References: <165110909570.7595.8578730126480600782.stgit@noble.brown>
User-Agent: StGit/1.5
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

nfs_access_get_cached() reports (on success) the access modes known to
be permitted.  The caller than has to compare this against the wanted
modes.

This is replaced with nfs_access_check_cached() which is told what modes
are wanted and returns -EACCES if they are not available, 0 if they are,
or -ECHILD or -ENOENT as before.

This new interface will allow more subtlety for choose when to trust the
cache to be encoded in nfs_access_check_cached(), which is then
available to all callers.

nfs_access_calc_mask() is changed to nfs_calc_access_mask().  This new
function is given a set of Linux MAY_* flags and returns the
corresponding set of NFS_ACCESS_* flags.  This is the inverse of
nfs_access_calc_mask().

Because of this inversion there is a small change in behaviour.  For a
non-file, non-directory, the previous code would grant MAY_WRITE if any
for NFS_ACCESS_MODIFY, NFS_ACCESS_EXTEND, NFS_ACCESS_DELETE were
granted.
This new code does not support this "any" approach, and it isn't clear
it is the most correct approach.  So for non-file, non-directories,
MAY_WRITE will only be granted if NFS_ACCESS_MODIFY is granted.

The nfs_access_exit trace event now reports the NFS_ACCESS flags, not he
Linux MAY flags.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/dir.c           |   62 +++++++++++++++++++++++++-----------------------
 fs/nfs/nfs4proc.c      |   24 +++++++------------
 include/linux/nfs_fs.h |    4 ++-
 3 files changed, 43 insertions(+), 47 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index c6b263b5faf1..b4e2c6a34234 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2971,19 +2971,22 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre
 	return err;
 }
 
-int nfs_access_get_cached(struct inode *inode, const struct cred *cred,
-			  u32 *mask, bool may_block)
+int nfs_access_check_cached(struct inode *inode, const struct cred *cred,
+			    u32 want, bool may_block)
 {
+	u32 have;
 	int status;
 
-	status = nfs_access_get_cached_rcu(inode, cred, mask);
+	status = nfs_access_get_cached_rcu(inode, cred, &have);
 	if (status != 0)
-		status = nfs_access_get_cached_locked(inode, cred, mask,
-		    may_block);
+		status = nfs_access_get_cached_locked(inode, cred, &have,
+						      may_block);
 
+	if (status == 0 && (want & ~have) != 0)
+		status = -EACCES;
 	return status;
 }
-EXPORT_SYMBOL_GPL(nfs_access_get_cached);
+EXPORT_SYMBOL_GPL(nfs_access_check_cached);
 
 static void nfs_access_add_rbtree(struct inode *inode,
 				  struct nfs_access_entry *set,
@@ -3067,26 +3070,26 @@ EXPORT_SYMBOL_GPL(nfs_access_add_cache);
 #define NFS_DIR_MAY_WRITE NFS_MAY_WRITE
 #define NFS_MAY_LOOKUP (NFS_ACCESS_LOOKUP)
 #define NFS_MAY_EXECUTE (NFS_ACCESS_EXECUTE)
-static int
-nfs_access_calc_mask(u32 access_result, umode_t umode)
+static u32
+nfs_calc_access_mask(int may_mask, umode_t umode)
 {
-	int mask = 0;
+	int access_mask = 0;
 
-	if (access_result & NFS_MAY_READ)
-		mask |= MAY_READ;
+	if (may_mask & MAY_READ)
+		access_mask |= NFS_MAY_READ;
 	if (S_ISDIR(umode)) {
-		if ((access_result & NFS_DIR_MAY_WRITE) == NFS_DIR_MAY_WRITE)
-			mask |= MAY_WRITE;
-		if ((access_result & NFS_MAY_LOOKUP) == NFS_MAY_LOOKUP)
-			mask |= MAY_EXEC;
+		if (may_mask & MAY_WRITE)
+			access_mask |= NFS_DIR_MAY_WRITE;
+		if (may_mask & MAY_EXEC)
+			access_mask |= NFS_MAY_LOOKUP;
 	} else if (S_ISREG(umode)) {
-		if ((access_result & NFS_FILE_MAY_WRITE) == NFS_FILE_MAY_WRITE)
-			mask |= MAY_WRITE;
-		if ((access_result & NFS_MAY_EXECUTE) == NFS_MAY_EXECUTE)
-			mask |= MAY_EXEC;
-	} else if (access_result & NFS_MAY_WRITE)
-			mask |= MAY_WRITE;
-	return mask;
+		if (may_mask & MAY_WRITE)
+			access_mask |= NFS_FILE_MAY_WRITE;
+		if (may_mask & MAY_EXEC)
+			access_mask |= NFS_MAY_EXECUTE;
+	} else if (may_mask & MAY_WRITE)
+		access_mask |= NFS_ACCESS_MODIFY;
+	return access_mask;
 }
 
 void nfs_access_set_mask(struct nfs_access_entry *entry, u32 access_result)
@@ -3099,14 +3102,15 @@ static int nfs_do_access(struct inode *inode, const struct cred *cred, int mask)
 {
 	struct nfs_access_entry cache;
 	bool may_block = (mask & MAY_NOT_BLOCK) == 0;
-	int cache_mask = -1;
+	u32 want;
 	int status;
 
 	trace_nfs_access_enter(inode);
 
-	status = nfs_access_get_cached(inode, cred, &cache.mask, may_block);
-	if (status == 0)
-		goto out_cached;
+	want = nfs_calc_access_mask(mask, inode->i_mode);
+	status = nfs_access_check_cached(inode, cred, want, may_block);
+	if (status == 0 || status == -EACCES)
+		goto out;
 
 	status = -ECHILD;
 	if (!may_block)
@@ -3132,12 +3136,10 @@ static int nfs_do_access(struct inode *inode, const struct cred *cred, int mask)
 		goto out;
 	}
 	nfs_access_add_cache(inode, &cache, cred);
-out_cached:
-	cache_mask = nfs_access_calc_mask(cache.mask, inode->i_mode);
-	if ((mask & ~cache_mask & (MAY_READ | MAY_WRITE | MAY_EXEC)) != 0)
+	if ((want & ~cache.mask) != 0)
 		status = -EACCES;
 out:
-	trace_nfs_access_exit(inode, mask, cache_mask, status);
+	trace_nfs_access_exit(inode, want, cache.mask, status);
 	return status;
 }
 
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 16106f805ffa..1e80d88df588 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7719,7 +7719,6 @@ static int nfs4_xattr_set_nfs4_user(const struct xattr_handler *handler,
 				    const char *key, const void *buf,
 				    size_t buflen, int flags)
 {
-	u32 mask;
 	int ret;
 
 	if (!nfs_server_capable(inode, NFS_CAP_XATTR))
@@ -7734,10 +7733,9 @@ static int nfs4_xattr_set_nfs4_user(const struct xattr_handler *handler,
 	 * do a cached access check for the XA* flags to possibly avoid
 	 * doing an RPC and getting EACCES back.
 	 */
-	if (!nfs_access_get_cached(inode, current_cred(), &mask, true)) {
-		if (!(mask & NFS_ACCESS_XAWRITE))
-			return -EACCES;
-	}
+	if (nfs_access_check_cached(inode, current_cred(), NFS_ACCESS_XAWRITE,
+				    true) == -EACCES)
+		return -EACCES;
 
 	if (buf == NULL) {
 		ret = nfs42_proc_removexattr(inode, key);
@@ -7756,16 +7754,14 @@ static int nfs4_xattr_get_nfs4_user(const struct xattr_handler *handler,
 				    struct dentry *unused, struct inode *inode,
 				    const char *key, void *buf, size_t buflen)
 {
-	u32 mask;
 	ssize_t ret;
 
 	if (!nfs_server_capable(inode, NFS_CAP_XATTR))
 		return -EOPNOTSUPP;
 
-	if (!nfs_access_get_cached(inode, current_cred(), &mask, true)) {
-		if (!(mask & NFS_ACCESS_XAREAD))
-			return -EACCES;
-	}
+	if (nfs_access_check_cached(inode, current_cred(), NFS_ACCESS_XAREAD,
+				    true) == -EACCES)
+		return -EACCES;
 
 	ret = nfs_revalidate_inode(inode, NFS_INO_INVALID_CHANGE);
 	if (ret)
@@ -7788,15 +7784,13 @@ nfs4_listxattr_nfs4_user(struct inode *inode, char *list, size_t list_len)
 	ssize_t ret, size;
 	char *buf;
 	size_t buflen;
-	u32 mask;
 
 	if (!nfs_server_capable(inode, NFS_CAP_XATTR))
 		return 0;
 
-	if (!nfs_access_get_cached(inode, current_cred(), &mask, true)) {
-		if (!(mask & NFS_ACCESS_XALIST))
-			return 0;
-	}
+	if (nfs_access_check_cached(inode, current_cred(), NFS_ACCESS_XALIST,
+				    true) == -EACCES)
+		return 0;
 
 	ret = nfs_revalidate_inode(inode, NFS_INO_INVALID_CHANGE);
 	if (ret)
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index b48b9259e02c..7614f621c42d 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -530,8 +530,8 @@ extern int nfs_instantiate(struct dentry *dentry, struct nfs_fh *fh,
 			struct nfs_fattr *fattr);
 extern int nfs_may_open(struct inode *inode, const struct cred *cred, int openflags);
 extern void nfs_access_zap_cache(struct inode *inode);
-extern int nfs_access_get_cached(struct inode *inode, const struct cred *cred,
-				 u32 *mask, bool may_block);
+extern int nfs_access_check_cached(struct inode *inode, const struct cred *cred,
+				   u32 want, bool may_block);
 
 /*
  * linux/fs/nfs/symlink.c