[RFC PATCH 0/6] Fix nits found by static analysis

[RFC PATCH 0/6] Fix nits found by static analysis

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Here are a handful of fixes for issues found by static analysis.
These are the ones I understood enough to address immediately. There
are others that will take some time to analyze and address.

Chuck Lever (6):
  NFSD: Remove dead code in nfsd4_create_session()
  NFSD: Remove a never-true comparison
  NFSD: Prevent NULL dereference in nfsd4_process_cb_update()
  NFSD: Remove unused results in nfsd4_encode_pathname4()
  NFSD: Remove unused values from nfsd4_encode_components_esc()
  NFSD: Cap the number of bytes copied by nfs4_reset_recoverydir()

 fs/nfsd/nfs4callback.c |  2 ++
 fs/nfsd/nfs4recover.c  |  3 ++-
 fs/nfsd/nfs4state.c    |  3 ---
 fs/nfsd/nfs4xdr.c      | 23 +++++++----------------
 fs/nfsd/nfsfh.c        |  2 +-
 5 files changed, 12 insertions(+), 21 deletions(-)

-- 
2.46.2

[RFC PATCH 1/6] NFSD: Remove dead code in nfsd4_create_session()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up. AFAICT, there is no way to reach the out_free_conn label
with @old set to a non-NULL value, so the expire_client(old) call
is never reached and can be removed.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2b21f2113c6e..3b16468ecbe7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3927,7 +3927,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 	return status;
 
 out_expired_error:
-	old = NULL;
 	/*
 	 * Revert the slot seq_nr change so the server will process
 	 * the client's resend instead of returning a cached response.
@@ -3942,8 +3941,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 out_free_conn:
 	spin_unlock(&nn->client_lock);
 	free_conn(conn);
-	if (old)
-		expire_client(old);
 out_free_session:
 	__free_session(new);
 out_release_drc_mem:
-- 
2.46.2

[RFC PATCH 2/6] NFSD: Remove a never-true comparison

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

fh_size is an unsigned int, thus it can never be less than 0.

Fixes: d8b26071e65e ("NFSD: simplify struct nfsfh")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfsfh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index 4c5deea0e953..641dfe54fb59 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -773,7 +773,7 @@ char * SVCFH_fmt(struct svc_fh *fhp)
 	struct knfsd_fh *fh = &fhp->fh_handle;
 	static char buf[2+1+1+64*3+1];
 
-	if (fh->fh_size < 0 || fh->fh_size> 64)
+	if (fh->fh_size > 64)
 		return "bad-fh";
 	sprintf(buf, "%d: %*ph", fh->fh_size, fh->fh_size, fh->fh_raw);
 	return buf;
-- 
2.46.2

[RFC PATCH 3/6] NFSD: Prevent NULL dereference in nfsd4_process_cb_update()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

@ses is initialized to NULL. If __nfsd4_find_backchannel() finds no
available backchannel session, setup_callback_client() will try to
dereference @ses and segfault.

Fixes: dcbeaa68dbbd ("nfsd4: allow backchannel recovery")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4callback.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index d86a7b983785..2e18f635078f 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -1500,6 +1500,8 @@ static void nfsd4_process_cb_update(struct nfsd4_callback *cb)
 		ses = c->cn_session;
 	}
 	spin_unlock(&clp->cl_lock);
+	if (!c)
+		return;
 
 	err = setup_callback_client(clp, &conn, ses);
 	if (err) {
-- 
2.46.2

[RFC PATCH 4/6] NFSD: Remove unused results in nfsd4_encode_pathname4()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up. The result of "*p++" is saved, but is not used before it
is overwritten. The result of xdr_encode_opaque() is saved each
time through the loop but is never used.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index ff897a368c01..6633fa06bc91 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2720,7 +2720,6 @@ static __be32 nfsd4_encode_pathname4(struct xdr_stream *xdr,
 				     const struct path *path)
 {
 	struct path cur = *path;
-	__be32 *p;
 	struct dentry **components = NULL;
 	unsigned int ncomponents = 0;
 	__be32 err = nfserr_jukebox;
@@ -2751,24 +2750,19 @@ static __be32 nfsd4_encode_pathname4(struct xdr_stream *xdr,
 		components[ncomponents++] = cur.dentry;
 		cur.dentry = dget_parent(cur.dentry);
 	}
-	err = nfserr_resource;
-	p = xdr_reserve_space(xdr, 4);
-	if (!p)
-		goto out_free;
-	*p++ = cpu_to_be32(ncomponents);
 
+	err = nfserr_resource;
+	if (xdr_stream_encode_u32(xdr, ncomponents) != XDR_UNIT)
+		goto out_free;
 	while (ncomponents) {
 		struct dentry *dentry = components[ncomponents - 1];
-		unsigned int len;
 
 		spin_lock(&dentry->d_lock);
-		len = dentry->d_name.len;
-		p = xdr_reserve_space(xdr, len + 4);
-		if (!p) {
+		if (xdr_stream_encode_opaque(xdr, dentry->d_name.name,
+					     dentry->d_name.len) < 0) {
 			spin_unlock(&dentry->d_lock);
 			goto out_free;
 		}
-		p = xdr_encode_opaque(p, dentry->d_name.name, len);
 		dprintk("/%pd", dentry);
 		spin_unlock(&dentry->d_lock);
 		dput(dentry);
-- 
2.46.2

[RFC PATCH 5/6] NFSD: Remove unused values from nfsd4_encode_components_esc()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up. The computed value of @p is saved each time through the
loop but is never used.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 6633fa06bc91..59de516edc2d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2673,13 +2673,10 @@ static __be32 nfsd4_encode_components_esc(struct xdr_stream *xdr, char sep,
 
 		strlen = end - str;
 		if (strlen) {
-			p = xdr_reserve_space(xdr, strlen + 4);
-			if (!p)
+			if (xdr_stream_encode_opaque(xdr, str, strlen) < 0)
 				return nfserr_resource;
-			p = xdr_encode_opaque(p, str, strlen);
 			count++;
-		}
-		else
+		} else
 			end++;
 		if (found_esc)
 			end = next;
-- 
2.46.2

[RFC PATCH 6/6] NFSD: Cap the number of bytes copied by nfs4_reset_recoverydir()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

It's only current caller already length-checks the string, but let's
be safe.

Fixes: 0964a3d3f1aa ("[PATCH] knfsd: nfsd4 reboot dirname fix")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4recover.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index b7d61eb8afe9..4a765555bf84 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -659,7 +659,8 @@ nfs4_reset_recoverydir(char *recdir)
 		return status;
 	status = -ENOTDIR;
 	if (d_is_dir(path.dentry)) {
-		strcpy(user_recovery_dirname, recdir);
+		strscpy(user_recovery_dirname, recdir,
+			sizeof(user_recovery_dirname));
 		status = 0;
 	}
 	path_put(&path);
-- 
2.46.2

Re: [RFC PATCH 0/6] Fix nits found by static analysis

[Jeff Layton]

On Thu, 2024-10-17 at 11:03 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Here are a handful of fixes for issues found by static analysis.
> These are the ones I understood enough to address immediately. There
> are others that will take some time to analyze and address.
> 
> Chuck Lever (6):
>   NFSD: Remove dead code in nfsd4_create_session()
>   NFSD: Remove a never-true comparison
>   NFSD: Prevent NULL dereference in nfsd4_process_cb_update()
>   NFSD: Remove unused results in nfsd4_encode_pathname4()
>   NFSD: Remove unused values from nfsd4_encode_components_esc()
>   NFSD: Cap the number of bytes copied by nfs4_reset_recoverydir()
> 
>  fs/nfsd/nfs4callback.c |  2 ++
>  fs/nfsd/nfs4recover.c  |  3 ++-
>  fs/nfsd/nfs4state.c    |  3 ---
>  fs/nfsd/nfs4xdr.c      | 23 +++++++----------------
>  fs/nfsd/nfsfh.c        |  2 +-
>  5 files changed, 12 insertions(+), 21 deletions(-)
> 

This all looks fine.

Reviewed-by: Jeff Layton <jlayton@kernel.org>