[PATCH v2 0/2] nfsd: Clean up nfs4_make_rec_clidname()

[PATCH v2 0/2] nfsd: Clean up nfs4_make_rec_clidname()

[Eric Biggers]

Two cleanups for the MD5 calculation and formatting in
nfs4_make_rec_clidname().

Eric Biggers (2):
  nfsd: Replace open-coded conversion of bytes to hex
  nfsd: Eliminate an allocation in nfs4_make_rec_clidname()

 fs/nfsd/nfs4recover.c | 31 +++++--------------------------
 fs/nfsd/state.h       |  4 +++-
 2 files changed, 8 insertions(+), 27 deletions(-)


base-commit: 35a813e010b99894bb4706c56c16a580bf7959c2
-- 
2.50.1.565.gc32cd1483b-goog

[PATCH v2 1/2] nfsd: Replace open-coded conversion of bytes to hex

[Eric Biggers]

Since the Linux kernel's sprintf() has conversion to hex built-in via
"%*phN", delete md5_to_hex() and just use that.  Also add an explicit
array bound to the dname parameter of nfs4_make_rec_clidname() to make
its size clear.  No functional change.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 fs/nfsd/nfs4recover.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 2231192ec33f..54f5e5392ef9 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -90,26 +90,12 @@ static void
 nfs4_reset_creds(const struct cred *original)
 {
 	put_cred(revert_creds(original));
 }
 
-static void
-md5_to_hex(char *out, char *md5)
-{
-	int i;
-
-	for (i=0; i<16; i++) {
-		unsigned char c = md5[i];
-
-		*out++ = '0' + ((c&0xf0)>>4) + (c>=0xa0)*('a'-'9'-1);
-		*out++ = '0' + (c&0x0f) + ((c&0x0f)>=0x0a)*('a'-'9'-1);
-	}
-	*out = '\0';
-}
-
 static int
-nfs4_make_rec_clidname(char *dname, const struct xdr_netobj *clname)
+nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
 {
 	struct xdr_netobj cksum;
 	struct crypto_shash *tfm;
 	int status;
 
@@ -131,11 +117,11 @@ nfs4_make_rec_clidname(char *dname, const struct xdr_netobj *clname)
 	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
 					 cksum.data);
 	if (status)
 		goto out;
 
-	md5_to_hex(dname, cksum.data);
+	sprintf(dname, "%*phN", 16, cksum.data);
 
 	status = 0;
 out:
 	kfree(cksum.data);
 	crypto_free_shash(tfm);
-- 
2.50.1.565.gc32cd1483b-goog

Re: [PATCH v2 1/2] nfsd: Replace open-coded conversion of bytes to hex

[Andy Shevchenko]

On Mon, Aug 04, 2025 at 10:46:59PM +0000, Eric Biggers wrote:
> Since the Linux kernel's sprintf() has conversion to hex built-in via
> "%*phN", delete md5_to_hex() and just use that.  Also add an explicit
> array bound to the dname parameter of nfs4_make_rec_clidname() to make
> its size clear.  No functional change.

...

> +	sprintf(dname, "%*phN", 16, cksum.data);

FWIW, since the length is static no need to use stack for that:

	sprintf(dname, "%16phN", cksum.data);

will be better.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/2] nfsd: Eliminate an allocation in nfs4_make_rec_clidname()

[Eric Biggers]

Since MD5 digests are fixed-size, make nfs4_make_rec_clidname() store
the digest in a stack buffer instead of a dynamically allocated buffer.
Use MD5_DIGEST_SIZE instead of a hard-coded value, both in
nfs4_make_rec_clidname() and in the definition of HEXDIR_LEN.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 fs/nfsd/nfs4recover.c | 15 ++++-----------
 fs/nfsd/state.h       |  4 +++-
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 54f5e5392ef9..e2b9472e5c78 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -93,11 +93,11 @@ nfs4_reset_creds(const struct cred *original)
 }
 
 static int
 nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
 {
-	struct xdr_netobj cksum;
+	u8 digest[MD5_DIGEST_SIZE];
 	struct crypto_shash *tfm;
 	int status;
 
 	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
 			clname->len, clname->data);
@@ -105,27 +105,20 @@ nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
 	if (IS_ERR(tfm)) {
 		status = PTR_ERR(tfm);
 		goto out_no_tfm;
 	}
 
-	cksum.len = crypto_shash_digestsize(tfm);
-	cksum.data = kmalloc(cksum.len, GFP_KERNEL);
-	if (cksum.data == NULL) {
-		status = -ENOMEM;
- 		goto out;
-	}
-
 	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
-					 cksum.data);
+					 digest);
 	if (status)
 		goto out;
 
-	sprintf(dname, "%*phN", 16, cksum.data);
+	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
+	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
 
 	status = 0;
 out:
-	kfree(cksum.data);
 	crypto_free_shash(tfm);
 out_no_tfm:
 	return status;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 8adc2550129e..b7d4c6d6f3d4 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -33,10 +33,11 @@
  */
 
 #ifndef _NFSD4_STATE_H
 #define _NFSD4_STATE_H
 
+#include <crypto/md5.h>
 #include <linux/idr.h>
 #include <linux/refcount.h>
 #include <linux/sunrpc/svc_xprt.h>
 #include "nfsfh.h"
 #include "nfsd.h"
@@ -379,11 +380,12 @@ struct nfsd4_sessionid {
 	clientid_t	clientid;
 	u32		sequence;
 	u32		reserved;
 };
 
-#define HEXDIR_LEN     33 /* hex version of 16 byte md5 of cl_name plus '\0' */
+/* Length of MD5 digest as hex, plus terminating '\0' */
+#define HEXDIR_LEN	(2 * MD5_DIGEST_SIZE + 1)
 
 /*
  *       State                Meaning                  Where set
  * --------------------------------------------------------------------------
  * | NFSD4_ACTIVE      | Confirmed, active    | Default                     |
-- 
2.50.1.565.gc32cd1483b-goog

Re: [PATCH v2 2/2] nfsd: Eliminate an allocation in nfs4_make_rec_clidname()

[Jeff Layton]

On Mon, 2025-08-04 at 22:47 +0000, Eric Biggers wrote:
> Since MD5 digests are fixed-size, make nfs4_make_rec_clidname() store
> the digest in a stack buffer instead of a dynamically allocated buffer.
> Use MD5_DIGEST_SIZE instead of a hard-coded value, both in
> nfs4_make_rec_clidname() and in the definition of HEXDIR_LEN.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
>  fs/nfsd/nfs4recover.c | 15 ++++-----------
>  fs/nfsd/state.h       |  4 +++-
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 54f5e5392ef9..e2b9472e5c78 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -93,11 +93,11 @@ nfs4_reset_creds(const struct cred *original)
>  }
>  
>  static int
>  nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
>  {
> -	struct xdr_netobj cksum;
> +	u8 digest[MD5_DIGEST_SIZE];
>  	struct crypto_shash *tfm;
>  	int status;
>  
>  	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
>  			clname->len, clname->data);
> @@ -105,27 +105,20 @@ nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
>  	if (IS_ERR(tfm)) {
>  		status = PTR_ERR(tfm);
>  		goto out_no_tfm;
>  	}
>  
> -	cksum.len = crypto_shash_digestsize(tfm);
> -	cksum.data = kmalloc(cksum.len, GFP_KERNEL);
> -	if (cksum.data == NULL) {
> -		status = -ENOMEM;
> - 		goto out;
> -	}
> -
>  	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
> -					 cksum.data);
> +					 digest);
>  	if (status)
>  		goto out;
>  
> -	sprintf(dname, "%*phN", 16, cksum.data);
> +	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
> +	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
>  
>  	status = 0;
>  out:
> -	kfree(cksum.data);
>  	crypto_free_shash(tfm);
>  out_no_tfm:
>  	return status;
>  }
>  
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 8adc2550129e..b7d4c6d6f3d4 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -33,10 +33,11 @@
>   */
>  
>  #ifndef _NFSD4_STATE_H
>  #define _NFSD4_STATE_H
>  
> +#include <crypto/md5.h>
>  #include <linux/idr.h>
>  #include <linux/refcount.h>
>  #include <linux/sunrpc/svc_xprt.h>
>  #include "nfsfh.h"
>  #include "nfsd.h"
> @@ -379,11 +380,12 @@ struct nfsd4_sessionid {
>  	clientid_t	clientid;
>  	u32		sequence;
>  	u32		reserved;
>  };
>  
> -#define HEXDIR_LEN     33 /* hex version of 16 byte md5 of cl_name plus '\0' */
> +/* Length of MD5 digest as hex, plus terminating '\0' */
> +#define HEXDIR_LEN	(2 * MD5_DIGEST_SIZE + 1)
>  
>  /*
>   *       State                Meaning                  Where set
>   * --------------------------------------------------------------------------
>   * | NFSD4_ACTIVE      | Confirmed, active    | Default                     |

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

Re: [PATCH v2 0/2] nfsd: Clean up nfs4_make_rec_clidname()

[Chuck Lever]

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

On Mon, 04 Aug 2025 22:46:58 +0000, Eric Biggers wrote:
> Two cleanups for the MD5 calculation and formatting in
> nfs4_make_rec_clidname().
> 
> Eric Biggers (2):
>   nfsd: Replace open-coded conversion of bytes to hex
>   nfsd: Eliminate an allocation in nfs4_make_rec_clidname()
> 
> [...]

Applied v2 to nfsd-testing, thanks!

[1/2] nfsd: Replace open-coded conversion of bytes to hex
      commit: 9093f8308442fc5096e973a46107f8623e2b336b
[2/2] nfsd: Eliminate an allocation in nfs4_make_rec_clidname()
      commit: b0a26370a4129a7e3b477f80c39b08ed5faf1422

--
Chuck Lever