[PATCH v2 1/2] nfsd: allow nfsd_file_get to sanely handle a NULL pointer

[PATCH v2 1/2] nfsd: allow nfsd_file_get to sanely handle a NULL pointer

[Jeff Layton]

...and remove some now-useless NULL pointer checks in its callers.

Suggested-by: NeilBrown <neilb@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/filecache.c | 5 ++---
 fs/nfsd/nfs4state.c | 4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 0ef070349014..58ac93e7e680 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -452,7 +452,7 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
 struct nfsd_file *
 nfsd_file_get(struct nfsd_file *nf)
 {
-	if (likely(refcount_inc_not_zero(&nf->nf_ref)))
+	if (nf && refcount_inc_not_zero(&nf->nf_ref))
 		return nf;
 	return NULL;
 }
@@ -1096,8 +1096,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	rcu_read_lock();
 	nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
 			       nfsd_file_rhash_params);
-	if (nf)
-		nf = nfsd_file_get(nf);
+	nf = nfsd_file_get(nf);
 	rcu_read_unlock();
 
 	if (nf) {
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4809ae0f0138..655fcfec0ace 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -602,9 +602,7 @@ put_nfs4_file(struct nfs4_file *fi)
 static struct nfsd_file *
 __nfs4_get_fd(struct nfs4_file *f, int oflag)
 {
-	if (f->fi_fds[oflag])
-		return nfsd_file_get(f->fi_fds[oflag]);
-	return NULL;
+	return nfsd_file_get(f->fi_fds[oflag]);
 }
 
 static struct nfsd_file *
-- 
2.39.0

[PATCH v2 2/2] nfsd: fix potential race in nfs4_find_file

[Jeff Layton]

The WARN_ON_ONCE check is not terribly useful. It also seems possible
for nfs4_find_file to race with the destruction of an fi_deleg_file
while trying to take a reference to it.

Now that it's safe to pass nfs_get_file a NULL pointer, remove the WARN
and NULL pointer check. Take the fi_lock when fetching fi_deleg_file.

Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/nfsd/nfs4state.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 655fcfec0ace..f923bab09a31 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6415,23 +6415,26 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 static struct nfsd_file *
 nfs4_find_file(struct nfs4_stid *s, int flags)
 {
+	struct nfsd_file *ret = NULL;
+
 	if (!s)
 		return NULL;
 
 	switch (s->sc_type) {
 	case NFS4_DELEG_STID:
-		if (WARN_ON_ONCE(!s->sc_file->fi_deleg_file))
-			return NULL;
-		return nfsd_file_get(s->sc_file->fi_deleg_file);
+		spin_lock(&s->sc_file->fi_lock);
+		ret = nfsd_file_get(s->sc_file->fi_deleg_file);
+		spin_unlock(&s->sc_file->fi_lock);
+		break;
 	case NFS4_OPEN_STID:
 	case NFS4_LOCK_STID:
 		if (flags & RD_STATE)
-			return find_readable_file(s->sc_file);
+			ret = find_readable_file(s->sc_file);
 		else
-			return find_writeable_file(s->sc_file);
+			ret = find_writeable_file(s->sc_file);
 	}
 
-	return NULL;
+	return ret;
 }
 
 static __be32
-- 
2.39.0

Re: [PATCH v2 1/2] nfsd: allow nfsd_file_get to sanely handle a NULL pointer

[Chuck Lever III]

> On Jan 6, 2023, at 10:33 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> ...and remove some now-useless NULL pointer checks in its callers.
> 
> Suggested-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/nfsd/filecache.c | 5 ++---
> fs/nfsd/nfs4state.c | 4 +---
> 2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 0ef070349014..58ac93e7e680 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -452,7 +452,7 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
> struct nfsd_file *
> nfsd_file_get(struct nfsd_file *nf)
> {
> -	if (likely(refcount_inc_not_zero(&nf->nf_ref)))
> +	if (nf && refcount_inc_not_zero(&nf->nf_ref))
> 		return nf;
> 	return NULL;
> }
> @@ -1096,8 +1096,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct svc_fh *fhp,
> 	rcu_read_lock();
> 	nf = rhashtable_lookup(&nfsd_file_rhash_tbl, &key,
> 			       nfsd_file_rhash_params);
> -	if (nf)
> -		nf = nfsd_file_get(nf);
> +	nf = nfsd_file_get(nf);
> 	rcu_read_unlock();
> 
> 	if (nf) {
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4809ae0f0138..655fcfec0ace 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -602,9 +602,7 @@ put_nfs4_file(struct nfs4_file *fi)
> static struct nfsd_file *
> __nfs4_get_fd(struct nfs4_file *f, int oflag)
> {
> -	if (f->fi_fds[oflag])
> -		return nfsd_file_get(f->fi_fds[oflag]);
> -	return NULL;
> +	return nfsd_file_get(f->fi_fds[oflag]);
> }
> 
> static struct nfsd_file *
> -- 
> 2.39.0

Hi Jeff-

I've applied v2 of 1/2 and 2/2 to nfsd's for-next.

--
Chuck Lever