public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nfsd: Use MD5 library instead of crypto_shash
@ 2025-10-11 18:52 Eric Biggers
  2025-10-12 11:12 ` Jeff Layton
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Eric Biggers @ 2025-10-11 18:52 UTC (permalink / raw)
  To: linux-nfs, Chuck Lever, Jeff Layton
  Cc: NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, linux-crypto,
	linux-kernel, Eric Biggers

Update NFSD's support for "legacy client tracking" (which uses MD5) to
use the MD5 library instead of crypto_shash.  This has several benefits:

- Simpler code.  Notably, much of the error-handling code is no longer
  needed, since the library functions can't fail.

- Improved performance due to reduced overhead.  A microbenchmark of
  nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.

- The MD5 code can now safely be built as a loadable module when nfsd is
  built as a loadable module.  (Previously, nfsd forced the MD5 code to
  built-in, presumably to work around the unreliablity of the name-based
  loading.)  Thus, select MD5 from the tristate option NFSD if
  NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.

To preserve the existing behavior of legacy client tracking support
being disabled when the kernel is booted with "fips=1", make
nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
know if this is truly needed, but it preserves the existing behavior.

Signed-off-by: Eric Biggers <ebiggers@kernel.org>
---
 fs/nfsd/Kconfig       |  3 +-
 fs/nfsd/nfs4recover.c | 82 ++++++++-----------------------------------
 2 files changed, 16 insertions(+), 69 deletions(-)

diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
index e134dce45e350..380a4caa33a73 100644
--- a/fs/nfsd/Kconfig
+++ b/fs/nfsd/Kconfig
@@ -3,10 +3,11 @@ config NFSD
 	tristate "NFS server support"
 	depends on INET
 	depends on FILE_LOCKING
 	depends on FSNOTIFY
 	select CRC32
+	select CRYPTO_LIB_MD5 if NFSD_LEGACY_CLIENT_TRACKING
 	select CRYPTO_LIB_SHA256 if NFSD_V4
 	select LOCKD
 	select SUNRPC
 	select EXPORTFS
 	select NFS_COMMON
@@ -75,12 +76,10 @@ config NFSD_V3_ACL
 config NFSD_V4
 	bool "NFS server support for NFS version 4"
 	depends on NFSD && PROC_FS
 	select FS_POSIX_ACL
 	select RPCSEC_GSS_KRB5
-	select CRYPTO
-	select CRYPTO_MD5
 	select GRACE_PERIOD
 	select NFS_V4_2_SSC_HELPER if NFS_V4_2
 	help
 	  This option enables support in your system's NFS server for
 	  version 4 of the NFS protocol (RFC 3530).
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index e2b9472e5c78c..dbc0aecef95e3 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -30,13 +30,14 @@
 *  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 *
 */
 
-#include <crypto/hash.h>
+#include <crypto/md5.h>
 #include <crypto/sha2.h>
 #include <linux/file.h>
+#include <linux/fips.h>
 #include <linux/slab.h>
 #include <linux/namei.h>
 #include <linux/sched.h>
 #include <linux/fs.h>
 #include <linux/module.h>
@@ -90,61 +91,22 @@ static void
 nfs4_reset_creds(const struct cred *original)
 {
 	put_cred(revert_creds(original));
 }
 
-static int
+static void
 nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
 {
 	u8 digest[MD5_DIGEST_SIZE];
-	struct crypto_shash *tfm;
-	int status;
 
 	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
 			clname->len, clname->data);
-	tfm = crypto_alloc_shash("md5", 0, 0);
-	if (IS_ERR(tfm)) {
-		status = PTR_ERR(tfm);
-		goto out_no_tfm;
-	}
 
-	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
-					 digest);
-	if (status)
-		goto out;
+	md5(clname->data, clname->len, digest);
 
 	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
 	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
-
-	status = 0;
-out:
-	crypto_free_shash(tfm);
-out_no_tfm:
-	return status;
-}
-
-/*
- * If we had an error generating the recdir name for the legacy tracker
- * then warn the admin. If the error doesn't appear to be transient,
- * then disable recovery tracking.
- */
-static void
-legacy_recdir_name_error(struct nfs4_client *clp, int error)
-{
-	printk(KERN_ERR "NFSD: unable to generate recoverydir "
-			"name (%d).\n", error);
-
-	/*
-	 * if the algorithm just doesn't exist, then disable the recovery
-	 * tracker altogether. The crypto libs will generally return this if
-	 * FIPS is enabled as well.
-	 */
-	if (error == -ENOENT) {
-		printk(KERN_ERR "NFSD: disabling legacy clientid tracking. "
-			"Reboot recovery will not function correctly!\n");
-		nfsd4_client_tracking_exit(clp->net);
-	}
 }
 
 static void
 __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
 		const char *dname, int len, struct nfsd_net *nn)
@@ -180,13 +142,11 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	if (test_and_set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return;
 	if (!nn->rec_file)
 		return;
 
-	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
-	if (status)
-		return legacy_recdir_name_error(clp, status);
+	nfs4_make_rec_clidname(dname, &clp->cl_name);
 
 	status = nfs4_save_creds(&original_cred);
 	if (status < 0)
 		return;
 
@@ -374,13 +334,11 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 
 	if (!nn->rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return;
 
-	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
-	if (status)
-		return legacy_recdir_name_error(clp, status);
+	nfs4_make_rec_clidname(dname, &clp->cl_name);
 
 	status = mnt_want_write_file(nn->rec_file);
 	if (status)
 		goto out;
 	clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
@@ -601,10 +559,15 @@ nfsd4_legacy_tracking_init(struct net *net)
 	if (net != &init_net) {
 		pr_warn("NFSD: attempt to initialize legacy client tracking in a container ignored.\n");
 		return -EINVAL;
 	}
 
+	if (fips_enabled) {
+		pr_warn("NFSD: legacy client tracking is disabled due to FIPS\n");
+		return -EINVAL;
+	}
+
 	status = nfs4_legacy_state_init(net);
 	if (status)
 		return status;
 
 	status = nfsd4_load_reboot_recovery_data(net);
@@ -657,25 +620,20 @@ nfs4_recoverydir(void)
 }
 
 static int
 nfsd4_check_legacy_client(struct nfs4_client *clp)
 {
-	int status;
 	char dname[HEXDIR_LEN];
 	struct nfs4_client_reclaim *crp;
 	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
 	struct xdr_netobj name;
 
 	/* did we already find that this client is stable? */
 	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return 0;
 
-	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
-	if (status) {
-		legacy_recdir_name_error(clp, status);
-		return status;
-	}
+	nfs4_make_rec_clidname(dname, &clp->cl_name);
 
 	/* look for it in the reclaim hashtable otherwise */
 	name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
 	if (!name.data) {
 		dprintk("%s: failed to allocate memory for name.data!\n",
@@ -1264,17 +1222,14 @@ nfsd4_cld_check(struct nfs4_client *clp)
 	if (crp)
 		goto found;
 
 #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
 	if (nn->cld_net->cn_has_legacy) {
-		int status;
 		char dname[HEXDIR_LEN];
 		struct xdr_netobj name;
 
-		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
-		if (status)
-			return -ENOENT;
+		nfs4_make_rec_clidname(dname, &clp->cl_name);
 
 		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
 		if (!name.data) {
 			dprintk("%s: failed to allocate memory for name.data!\n",
 				__func__);
@@ -1315,15 +1270,12 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
 
 #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
 	if (cn->cn_has_legacy) {
 		struct xdr_netobj name;
 		char dname[HEXDIR_LEN];
-		int status;
 
-		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
-		if (status)
-			return -ENOENT;
+		nfs4_make_rec_clidname(dname, &clp->cl_name);
 
 		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
 		if (!name.data) {
 			dprintk("%s: failed to allocate memory for name.data\n",
 					__func__);
@@ -1692,15 +1644,11 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
 		/* just return nothing if output will be truncated */
 		kfree(result);
 		return NULL;
 	}
 
-	copied = nfs4_make_rec_clidname(result + copied, name);
-	if (copied) {
-		kfree(result);
-		return NULL;
-	}
+	nfs4_make_rec_clidname(result + copied, name);
 
 	return result;
 }
 
 static char *

base-commit: 0739473694c4878513031006829f1030ec850bc2
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-11 18:52 [PATCH] nfsd: Use MD5 library instead of crypto_shash Eric Biggers
@ 2025-10-12 11:12 ` Jeff Layton
  2025-10-12 17:00   ` Eric Biggers
  2025-10-12 15:59 ` Chuck Lever
  2025-10-14  7:56 ` Ard Biesheuvel
  2 siblings, 1 reply; 12+ messages in thread
From: Jeff Layton @ 2025-10-12 11:12 UTC (permalink / raw)
  To: Eric Biggers, linux-nfs, Chuck Lever
  Cc: NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, linux-crypto,
	linux-kernel

On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
> Update NFSD's support for "legacy client tracking" (which uses MD5) to
> use the MD5 library instead of crypto_shash.  This has several benefits:
> 
> - Simpler code.  Notably, much of the error-handling code is no longer
>   needed, since the library functions can't fail.
> 
> - Improved performance due to reduced overhead.  A microbenchmark of
>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> 
> - The MD5 code can now safely be built as a loadable module when nfsd is
>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
>   built-in, presumably to work around the unreliablity of the name-based
>   loading.)  Thus, select MD5 from the tristate option NFSD if
>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> 
> To preserve the existing behavior of legacy client tracking support
> being disabled when the kernel is booted with "fips=1", make
> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> know if this is truly needed, but it preserves the existing behavior.
> 

FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
a cryptographically significant way here, but the FIPS gods won't bless
a kernel that uses MD5 at all, so I think it is needed.


> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
>  fs/nfsd/Kconfig       |  3 +-
>  fs/nfsd/nfs4recover.c | 82 ++++++++-----------------------------------
>  2 files changed, 16 insertions(+), 69 deletions(-)
> 
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index e134dce45e350..380a4caa33a73 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -3,10 +3,11 @@ config NFSD
>  	tristate "NFS server support"
>  	depends on INET
>  	depends on FILE_LOCKING
>  	depends on FSNOTIFY
>  	select CRC32
> +	select CRYPTO_LIB_MD5 if NFSD_LEGACY_CLIENT_TRACKING
>  	select CRYPTO_LIB_SHA256 if NFSD_V4
>  	select LOCKD
>  	select SUNRPC
>  	select EXPORTFS
>  	select NFS_COMMON
> @@ -75,12 +76,10 @@ config NFSD_V3_ACL
>  config NFSD_V4
>  	bool "NFS server support for NFS version 4"
>  	depends on NFSD && PROC_FS
>  	select FS_POSIX_ACL
>  	select RPCSEC_GSS_KRB5
> -	select CRYPTO
> -	select CRYPTO_MD5
>  	select GRACE_PERIOD
>  	select NFS_V4_2_SSC_HELPER if NFS_V4_2
>  	help
>  	  This option enables support in your system's NFS server for
>  	  version 4 of the NFS protocol (RFC 3530).
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index e2b9472e5c78c..dbc0aecef95e3 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -30,13 +30,14 @@
>  *  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  *
>  */
>  
> -#include <crypto/hash.h>
> +#include <crypto/md5.h>
>  #include <crypto/sha2.h>
>  #include <linux/file.h>
> +#include <linux/fips.h>
>  #include <linux/slab.h>
>  #include <linux/namei.h>
>  #include <linux/sched.h>
>  #include <linux/fs.h>
>  #include <linux/module.h>
> @@ -90,61 +91,22 @@ static void
>  nfs4_reset_creds(const struct cred *original)
>  {
>  	put_cred(revert_creds(original));
>  }
>  
> -static int
> +static void
>  nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
>  {
>  	u8 digest[MD5_DIGEST_SIZE];
> -	struct crypto_shash *tfm;
> -	int status;
>  
>  	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
>  			clname->len, clname->data);
> -	tfm = crypto_alloc_shash("md5", 0, 0);
> -	if (IS_ERR(tfm)) {
> -		status = PTR_ERR(tfm);
> -		goto out_no_tfm;
> -	}
>  
> -	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
> -					 digest);
> -	if (status)
> -		goto out;
> +	md5(clname->data, clname->len, digest);
>  
>  	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
>  	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
> -
> -	status = 0;
> -out:
> -	crypto_free_shash(tfm);
> -out_no_tfm:
> -	return status;
> -}
> -
> -/*
> - * If we had an error generating the recdir name for the legacy tracker
> - * then warn the admin. If the error doesn't appear to be transient,
> - * then disable recovery tracking.
> - */
> -static void
> -legacy_recdir_name_error(struct nfs4_client *clp, int error)
> -{
> -	printk(KERN_ERR "NFSD: unable to generate recoverydir "
> -			"name (%d).\n", error);
> -
> -	/*
> -	 * if the algorithm just doesn't exist, then disable the recovery
> -	 * tracker altogether. The crypto libs will generally return this if
> -	 * FIPS is enabled as well.
> -	 */
> -	if (error == -ENOENT) {
> -		printk(KERN_ERR "NFSD: disabling legacy clientid tracking. "
> -			"Reboot recovery will not function correctly!\n");
> -		nfsd4_client_tracking_exit(clp->net);
> -	}
>  }
>  
>  static void
>  __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
>  		const char *dname, int len, struct nfsd_net *nn)
> @@ -180,13 +142,11 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>  	if (test_and_set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return;
>  	if (!nn->rec_file)
>  		return;
>  
> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -	if (status)
> -		return legacy_recdir_name_error(clp, status);
> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  	status = nfs4_save_creds(&original_cred);
>  	if (status < 0)
>  		return;
>  
> @@ -374,13 +334,11 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  
>  	if (!nn->rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return;
>  
> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -	if (status)
> -		return legacy_recdir_name_error(clp, status);
> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  	status = mnt_want_write_file(nn->rec_file);
>  	if (status)
>  		goto out;
>  	clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> @@ -601,10 +559,15 @@ nfsd4_legacy_tracking_init(struct net *net)
>  	if (net != &init_net) {
>  		pr_warn("NFSD: attempt to initialize legacy client tracking in a container ignored.\n");
>  		return -EINVAL;
>  	}
>  
> +	if (fips_enabled) {
> +		pr_warn("NFSD: legacy client tracking is disabled due to FIPS\n");
> +		return -EINVAL;
> +	}
> +
>  	status = nfs4_legacy_state_init(net);
>  	if (status)
>  		return status;
>  
>  	status = nfsd4_load_reboot_recovery_data(net);
> @@ -657,25 +620,20 @@ nfs4_recoverydir(void)
>  }
>  
>  static int
>  nfsd4_check_legacy_client(struct nfs4_client *clp)
>  {
> -	int status;
>  	char dname[HEXDIR_LEN];
>  	struct nfs4_client_reclaim *crp;
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	struct xdr_netobj name;
>  
>  	/* did we already find that this client is stable? */
>  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return 0;
>  
> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -	if (status) {
> -		legacy_recdir_name_error(clp, status);
> -		return status;
> -	}
> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  	/* look for it in the reclaim hashtable otherwise */
>  	name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>  	if (!name.data) {
>  		dprintk("%s: failed to allocate memory for name.data!\n",
> @@ -1264,17 +1222,14 @@ nfsd4_cld_check(struct nfs4_client *clp)
>  	if (crp)
>  		goto found;
>  
>  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	if (nn->cld_net->cn_has_legacy) {
> -		int status;
>  		char dname[HEXDIR_LEN];
>  		struct xdr_netobj name;
>  
> -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -		if (status)
> -			return -ENOENT;
> +		nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>  		if (!name.data) {
>  			dprintk("%s: failed to allocate memory for name.data!\n",
>  				__func__);
> @@ -1315,15 +1270,12 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
>  
>  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	if (cn->cn_has_legacy) {
>  		struct xdr_netobj name;
>  		char dname[HEXDIR_LEN];
> -		int status;
>  
> -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -		if (status)
> -			return -ENOENT;
> +		nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>  		if (!name.data) {
>  			dprintk("%s: failed to allocate memory for name.data\n",
>  					__func__);
> @@ -1692,15 +1644,11 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
>  		/* just return nothing if output will be truncated */
>  		kfree(result);
>  		return NULL;
>  	}
>  
> -	copied = nfs4_make_rec_clidname(result + copied, name);
> -	if (copied) {
> -		kfree(result);
> -		return NULL;
> -	}
> +	nfs4_make_rec_clidname(result + copied, name);
>  
>  	return result;
>  }
>  
>  static char *
> 
> base-commit: 0739473694c4878513031006829f1030ec850bc2

Seems fine to me and I like the diffstat. Looking forward to eventually
removing this code altogether.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-11 18:52 [PATCH] nfsd: Use MD5 library instead of crypto_shash Eric Biggers
  2025-10-12 11:12 ` Jeff Layton
@ 2025-10-12 15:59 ` Chuck Lever
  2025-10-13 10:46   ` Scott Mayhew
  2025-10-14  7:56 ` Ard Biesheuvel
  2 siblings, 1 reply; 12+ messages in thread
From: Chuck Lever @ 2025-10-12 15:59 UTC (permalink / raw)
  To: Eric Biggers, Scott Mayhew, Jeff Layton
  Cc: NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey, linux-crypto,
	linux-kernel, linux-nfs

On 10/11/25 2:52 PM, Eric Biggers wrote:
> Update NFSD's support for "legacy client tracking" (which uses MD5) to
> use the MD5 library instead of crypto_shash.  This has several benefits:
> 
> - Simpler code.  Notably, much of the error-handling code is no longer
>   needed, since the library functions can't fail.
> 
> - Improved performance due to reduced overhead.  A microbenchmark of
>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> 
> - The MD5 code can now safely be built as a loadable module when nfsd is
>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
>   built-in, presumably to work around the unreliablity of the name-based
>   loading.)  Thus, select MD5 from the tristate option NFSD if
>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> 
> To preserve the existing behavior of legacy client tracking support
> being disabled when the kernel is booted with "fips=1", make
> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> know if this is truly needed, but it preserves the existing behavior.
> 
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>

No objection, but let's cross our t's and dot our i's. Scott, when you
have recovered from bake-a-thon, can you have a look at this one?

Thanks!


> ---
>  fs/nfsd/Kconfig       |  3 +-
>  fs/nfsd/nfs4recover.c | 82 ++++++++-----------------------------------
>  2 files changed, 16 insertions(+), 69 deletions(-)
> 
> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index e134dce45e350..380a4caa33a73 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -3,10 +3,11 @@ config NFSD
>  	tristate "NFS server support"
>  	depends on INET
>  	depends on FILE_LOCKING
>  	depends on FSNOTIFY
>  	select CRC32
> +	select CRYPTO_LIB_MD5 if NFSD_LEGACY_CLIENT_TRACKING
>  	select CRYPTO_LIB_SHA256 if NFSD_V4
>  	select LOCKD
>  	select SUNRPC
>  	select EXPORTFS
>  	select NFS_COMMON
> @@ -75,12 +76,10 @@ config NFSD_V3_ACL
>  config NFSD_V4
>  	bool "NFS server support for NFS version 4"
>  	depends on NFSD && PROC_FS
>  	select FS_POSIX_ACL
>  	select RPCSEC_GSS_KRB5
> -	select CRYPTO
> -	select CRYPTO_MD5
>  	select GRACE_PERIOD
>  	select NFS_V4_2_SSC_HELPER if NFS_V4_2
>  	help
>  	  This option enables support in your system's NFS server for
>  	  version 4 of the NFS protocol (RFC 3530).
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index e2b9472e5c78c..dbc0aecef95e3 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -30,13 +30,14 @@
>  *  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  *
>  */
>  
> -#include <crypto/hash.h>
> +#include <crypto/md5.h>
>  #include <crypto/sha2.h>
>  #include <linux/file.h>
> +#include <linux/fips.h>
>  #include <linux/slab.h>
>  #include <linux/namei.h>
>  #include <linux/sched.h>
>  #include <linux/fs.h>
>  #include <linux/module.h>
> @@ -90,61 +91,22 @@ static void
>  nfs4_reset_creds(const struct cred *original)
>  {
>  	put_cred(revert_creds(original));
>  }
>  
> -static int
> +static void
>  nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
>  {
>  	u8 digest[MD5_DIGEST_SIZE];
> -	struct crypto_shash *tfm;
> -	int status;
>  
>  	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
>  			clname->len, clname->data);
> -	tfm = crypto_alloc_shash("md5", 0, 0);
> -	if (IS_ERR(tfm)) {
> -		status = PTR_ERR(tfm);
> -		goto out_no_tfm;
> -	}
>  
> -	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
> -					 digest);
> -	if (status)
> -		goto out;
> +	md5(clname->data, clname->len, digest);
>  
>  	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
>  	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
> -
> -	status = 0;
> -out:
> -	crypto_free_shash(tfm);
> -out_no_tfm:
> -	return status;
> -}
> -
> -/*
> - * If we had an error generating the recdir name for the legacy tracker
> - * then warn the admin. If the error doesn't appear to be transient,
> - * then disable recovery tracking.
> - */
> -static void
> -legacy_recdir_name_error(struct nfs4_client *clp, int error)
> -{
> -	printk(KERN_ERR "NFSD: unable to generate recoverydir "
> -			"name (%d).\n", error);
> -
> -	/*
> -	 * if the algorithm just doesn't exist, then disable the recovery
> -	 * tracker altogether. The crypto libs will generally return this if
> -	 * FIPS is enabled as well.
> -	 */
> -	if (error == -ENOENT) {
> -		printk(KERN_ERR "NFSD: disabling legacy clientid tracking. "
> -			"Reboot recovery will not function correctly!\n");
> -		nfsd4_client_tracking_exit(clp->net);
> -	}
>  }
>  
>  static void
>  __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
>  		const char *dname, int len, struct nfsd_net *nn)
> @@ -180,13 +142,11 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>  	if (test_and_set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return;
>  	if (!nn->rec_file)
>  		return;
>  
> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -	if (status)
> -		return legacy_recdir_name_error(clp, status);
> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  	status = nfs4_save_creds(&original_cred);
>  	if (status < 0)
>  		return;
>  
> @@ -374,13 +334,11 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  
>  	if (!nn->rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return;
>  
> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -	if (status)
> -		return legacy_recdir_name_error(clp, status);
> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  	status = mnt_want_write_file(nn->rec_file);
>  	if (status)
>  		goto out;
>  	clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> @@ -601,10 +559,15 @@ nfsd4_legacy_tracking_init(struct net *net)
>  	if (net != &init_net) {
>  		pr_warn("NFSD: attempt to initialize legacy client tracking in a container ignored.\n");
>  		return -EINVAL;
>  	}
>  
> +	if (fips_enabled) {
> +		pr_warn("NFSD: legacy client tracking is disabled due to FIPS\n");
> +		return -EINVAL;
> +	}
> +
>  	status = nfs4_legacy_state_init(net);
>  	if (status)
>  		return status;
>  
>  	status = nfsd4_load_reboot_recovery_data(net);
> @@ -657,25 +620,20 @@ nfs4_recoverydir(void)
>  }
>  
>  static int
>  nfsd4_check_legacy_client(struct nfs4_client *clp)
>  {
> -	int status;
>  	char dname[HEXDIR_LEN];
>  	struct nfs4_client_reclaim *crp;
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	struct xdr_netobj name;
>  
>  	/* did we already find that this client is stable? */
>  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>  		return 0;
>  
> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -	if (status) {
> -		legacy_recdir_name_error(clp, status);
> -		return status;
> -	}
> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  	/* look for it in the reclaim hashtable otherwise */
>  	name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>  	if (!name.data) {
>  		dprintk("%s: failed to allocate memory for name.data!\n",
> @@ -1264,17 +1222,14 @@ nfsd4_cld_check(struct nfs4_client *clp)
>  	if (crp)
>  		goto found;
>  
>  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	if (nn->cld_net->cn_has_legacy) {
> -		int status;
>  		char dname[HEXDIR_LEN];
>  		struct xdr_netobj name;
>  
> -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -		if (status)
> -			return -ENOENT;
> +		nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>  		if (!name.data) {
>  			dprintk("%s: failed to allocate memory for name.data!\n",
>  				__func__);
> @@ -1315,15 +1270,12 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
>  
>  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	if (cn->cn_has_legacy) {
>  		struct xdr_netobj name;
>  		char dname[HEXDIR_LEN];
> -		int status;
>  
> -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> -		if (status)
> -			return -ENOENT;
> +		nfs4_make_rec_clidname(dname, &clp->cl_name);
>  
>  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>  		if (!name.data) {
>  			dprintk("%s: failed to allocate memory for name.data\n",
>  					__func__);
> @@ -1692,15 +1644,11 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
>  		/* just return nothing if output will be truncated */
>  		kfree(result);
>  		return NULL;
>  	}
>  
> -	copied = nfs4_make_rec_clidname(result + copied, name);
> -	if (copied) {
> -		kfree(result);
> -		return NULL;
> -	}
> +	nfs4_make_rec_clidname(result + copied, name);
>  
>  	return result;
>  }
>  
>  static char *
> 
> base-commit: 0739473694c4878513031006829f1030ec850bc2


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-12 11:12 ` Jeff Layton
@ 2025-10-12 17:00   ` Eric Biggers
  2025-10-12 17:57     ` Simo Sorce
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Eric Biggers @ 2025-10-12 17:00 UTC (permalink / raw)
  To: Jeff Layton
  Cc: linux-nfs, Chuck Lever, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel

On Sun, Oct 12, 2025 at 07:12:26AM -0400, Jeff Layton wrote:
> On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
> > Update NFSD's support for "legacy client tracking" (which uses MD5) to
> > use the MD5 library instead of crypto_shash.  This has several benefits:
> > 
> > - Simpler code.  Notably, much of the error-handling code is no longer
> >   needed, since the library functions can't fail.
> > 
> > - Improved performance due to reduced overhead.  A microbenchmark of
> >   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> > 
> > - The MD5 code can now safely be built as a loadable module when nfsd is
> >   built as a loadable module.  (Previously, nfsd forced the MD5 code to
> >   built-in, presumably to work around the unreliablity of the name-based
> >   loading.)  Thus, select MD5 from the tristate option NFSD if
> >   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> > 
> > To preserve the existing behavior of legacy client tracking support
> > being disabled when the kernel is booted with "fips=1", make
> > nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> > know if this is truly needed, but it preserves the existing behavior.
> > 
> 
> FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
> a cryptographically significant way here, but the FIPS gods won't bless
> a kernel that uses MD5 at all, so I think it is needed.

If it's not being used for a security purpose, then I think you can just
drop the fips_enabled check.  People are used to the old API where MD5
was always forbidden when fips_enabled, but it doesn't actually need to
be that strict.  For this patch I wasn't certain about the use case
though, so I just opted to preserve the existing behavior for now.  A
follow-on patch to remove the check could make sense.

- Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-12 17:00   ` Eric Biggers
@ 2025-10-12 17:57     ` Simo Sorce
  2025-10-12 18:49     ` Jeff Layton
  2025-10-16 13:41     ` Chuck Lever
  2 siblings, 0 replies; 12+ messages in thread
From: Simo Sorce @ 2025-10-12 17:57 UTC (permalink / raw)
  To: Eric Biggers, Jeff Layton
  Cc: linux-nfs, Chuck Lever, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel

On Sun, 2025-10-12 at 10:00 -0700, Eric Biggers wrote:
> On Sun, Oct 12, 2025 at 07:12:26AM -0400, Jeff Layton wrote:
> > On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
> > > Update NFSD's support for "legacy client tracking" (which uses MD5) to
> > > use the MD5 library instead of crypto_shash.  This has several benefits:
> > > 
> > > - Simpler code.  Notably, much of the error-handling code is no longer
> > >   needed, since the library functions can't fail.
> > > 
> > > - Improved performance due to reduced overhead.  A microbenchmark of
> > >   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> > > 
> > > - The MD5 code can now safely be built as a loadable module when nfsd is
> > >   built as a loadable module.  (Previously, nfsd forced the MD5 code to
> > >   built-in, presumably to work around the unreliablity of the name-based
> > >   loading.)  Thus, select MD5 from the tristate option NFSD if
> > >   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> > > 
> > > To preserve the existing behavior of legacy client tracking support
> > > being disabled when the kernel is booted with "fips=1", make
> > > nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> > > know if this is truly needed, but it preserves the existing behavior.
> > > 
> > 
> > FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
> > a cryptographically significant way here, but the FIPS gods won't bless
> > a kernel that uses MD5 at all, so I think it is needed.
> 
> If it's not being used for a security purpose, then I think you can just
> drop the fips_enabled check.  People are used to the old API where MD5
> was always forbidden when fips_enabled, but it doesn't actually need to
> be that strict.  For this patch I wasn't certain about the use case
> though, so I just opted to preserve the existing behavior for now.  A
> follow-on patch to remove the check could make sense.

It would be nice to move MD5 (and reasonably soon after SHA-1 too) out
of lib/crypto and in some generic hashing utility place because they
are not cryptographic algorithms anymore and nobody should use them as
such.

That said MD5 appears to be used for cryptographic purposes (key/IV
derivation) in ecryptfs (which is pretty bad) and therefore ecryptfs
should be disabled in fips mode regardless (at least until they change
this aspect of the fs).

Specifically for this patch though I do not think you should keep
disabling nfsd4_legacy_tracking_init() in fips mode, as md5 here is not
used in a cryptographic capacity, it is just an identifier that is
easier to index.

Simo.

-- 
Simo Sorce
Distinguished Engineer
RHEL Crypto Team
Red Hat, Inc


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-12 17:00   ` Eric Biggers
  2025-10-12 17:57     ` Simo Sorce
@ 2025-10-12 18:49     ` Jeff Layton
  2025-10-16 13:41     ` Chuck Lever
  2 siblings, 0 replies; 12+ messages in thread
From: Jeff Layton @ 2025-10-12 18:49 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-nfs, Chuck Lever, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel

On Sun, 2025-10-12 at 10:00 -0700, Eric Biggers wrote:
> On Sun, Oct 12, 2025 at 07:12:26AM -0400, Jeff Layton wrote:
> > On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
> > > Update NFSD's support for "legacy client tracking" (which uses MD5) to
> > > use the MD5 library instead of crypto_shash.  This has several benefits:
> > > 
> > > - Simpler code.  Notably, much of the error-handling code is no longer
> > >   needed, since the library functions can't fail.
> > > 
> > > - Improved performance due to reduced overhead.  A microbenchmark of
> > >   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> > > 
> > > - The MD5 code can now safely be built as a loadable module when nfsd is
> > >   built as a loadable module.  (Previously, nfsd forced the MD5 code to
> > >   built-in, presumably to work around the unreliablity of the name-based
> > >   loading.)  Thus, select MD5 from the tristate option NFSD if
> > >   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> > > 
> > > To preserve the existing behavior of legacy client tracking support
> > > being disabled when the kernel is booted with "fips=1", make
> > > nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> > > know if this is truly needed, but it preserves the existing behavior.
> > > 
> > 
> > FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
> > a cryptographically significant way here, but the FIPS gods won't bless
> > a kernel that uses MD5 at all, so I think it is needed.
> 
> If it's not being used for a security purpose, then I think you can just
> drop the fips_enabled check.  People are used to the old API where MD5
> was always forbidden when fips_enabled, but it doesn't actually need to
> be that strict.  For this patch I wasn't certain about the use case
> though, so I just opted to preserve the existing behavior for now.  A
> follow-on patch to remove the check could make sense.
> 

This is all legacy code anyway. We just recently flipped the default
for CONFIG_NFSD_LEGACY_CLIENT_TRACKING to 'n', so I'm hoping that in a
year or so we should be able to just remove it altogether.

-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-12 15:59 ` Chuck Lever
@ 2025-10-13 10:46   ` Scott Mayhew
  2025-10-13 13:32     ` Chuck Lever
  0 siblings, 1 reply; 12+ messages in thread
From: Scott Mayhew @ 2025-10-13 10:46 UTC (permalink / raw)
  To: Chuck Lever
  Cc: Eric Biggers, Jeff Layton, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel, linux-nfs

On Sun, 12 Oct 2025, Chuck Lever wrote:

> On 10/11/25 2:52 PM, Eric Biggers wrote:
> > Update NFSD's support for "legacy client tracking" (which uses MD5) to
> > use the MD5 library instead of crypto_shash.  This has several benefits:
> > 
> > - Simpler code.  Notably, much of the error-handling code is no longer
> >   needed, since the library functions can't fail.
> > 
> > - Improved performance due to reduced overhead.  A microbenchmark of
> >   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> > 
> > - The MD5 code can now safely be built as a loadable module when nfsd is
> >   built as a loadable module.  (Previously, nfsd forced the MD5 code to
> >   built-in, presumably to work around the unreliablity of the name-based
> >   loading.)  Thus, select MD5 from the tristate option NFSD if
> >   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> > 
> > To preserve the existing behavior of legacy client tracking support
> > being disabled when the kernel is booted with "fips=1", make
> > nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> > know if this is truly needed, but it preserves the existing behavior.
> > 
> > Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> 
> No objection, but let's cross our t's and dot our i's. Scott, when you
> have recovered from bake-a-thon, can you have a look at this one?
> 
> Thanks!

Looks fine to me.

Reviewed-by: Scott Mayhew <smayhew@redhat.com>

I agree with Jeff - it would be nice to just remove the legacy tracking.
I'm guessing it's still used in smaller/embedded setups?  RHEL and Fedora
haven't had it enabled for years.  Looking at a few other distros... it's
not enabled in OpenSUSE Leap or Tumbleweed.  It's not enabled in Debian
Sid (but it is enabled in Trixie).

-Scott
> 
> 
> > ---
> >  fs/nfsd/Kconfig       |  3 +-
> >  fs/nfsd/nfs4recover.c | 82 ++++++++-----------------------------------
> >  2 files changed, 16 insertions(+), 69 deletions(-)
> > 
> > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > index e134dce45e350..380a4caa33a73 100644
> > --- a/fs/nfsd/Kconfig
> > +++ b/fs/nfsd/Kconfig
> > @@ -3,10 +3,11 @@ config NFSD
> >  	tristate "NFS server support"
> >  	depends on INET
> >  	depends on FILE_LOCKING
> >  	depends on FSNOTIFY
> >  	select CRC32
> > +	select CRYPTO_LIB_MD5 if NFSD_LEGACY_CLIENT_TRACKING
> >  	select CRYPTO_LIB_SHA256 if NFSD_V4
> >  	select LOCKD
> >  	select SUNRPC
> >  	select EXPORTFS
> >  	select NFS_COMMON
> > @@ -75,12 +76,10 @@ config NFSD_V3_ACL
> >  config NFSD_V4
> >  	bool "NFS server support for NFS version 4"
> >  	depends on NFSD && PROC_FS
> >  	select FS_POSIX_ACL
> >  	select RPCSEC_GSS_KRB5
> > -	select CRYPTO
> > -	select CRYPTO_MD5
> >  	select GRACE_PERIOD
> >  	select NFS_V4_2_SSC_HELPER if NFS_V4_2
> >  	help
> >  	  This option enables support in your system's NFS server for
> >  	  version 4 of the NFS protocol (RFC 3530).
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index e2b9472e5c78c..dbc0aecef95e3 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -30,13 +30,14 @@
> >  *  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> >  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >  *
> >  */
> >  
> > -#include <crypto/hash.h>
> > +#include <crypto/md5.h>
> >  #include <crypto/sha2.h>
> >  #include <linux/file.h>
> > +#include <linux/fips.h>
> >  #include <linux/slab.h>
> >  #include <linux/namei.h>
> >  #include <linux/sched.h>
> >  #include <linux/fs.h>
> >  #include <linux/module.h>
> > @@ -90,61 +91,22 @@ static void
> >  nfs4_reset_creds(const struct cred *original)
> >  {
> >  	put_cred(revert_creds(original));
> >  }
> >  
> > -static int
> > +static void
> >  nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
> >  {
> >  	u8 digest[MD5_DIGEST_SIZE];
> > -	struct crypto_shash *tfm;
> > -	int status;
> >  
> >  	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
> >  			clname->len, clname->data);
> > -	tfm = crypto_alloc_shash("md5", 0, 0);
> > -	if (IS_ERR(tfm)) {
> > -		status = PTR_ERR(tfm);
> > -		goto out_no_tfm;
> > -	}
> >  
> > -	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
> > -					 digest);
> > -	if (status)
> > -		goto out;
> > +	md5(clname->data, clname->len, digest);
> >  
> >  	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
> >  	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
> > -
> > -	status = 0;
> > -out:
> > -	crypto_free_shash(tfm);
> > -out_no_tfm:
> > -	return status;
> > -}
> > -
> > -/*
> > - * If we had an error generating the recdir name for the legacy tracker
> > - * then warn the admin. If the error doesn't appear to be transient,
> > - * then disable recovery tracking.
> > - */
> > -static void
> > -legacy_recdir_name_error(struct nfs4_client *clp, int error)
> > -{
> > -	printk(KERN_ERR "NFSD: unable to generate recoverydir "
> > -			"name (%d).\n", error);
> > -
> > -	/*
> > -	 * if the algorithm just doesn't exist, then disable the recovery
> > -	 * tracker altogether. The crypto libs will generally return this if
> > -	 * FIPS is enabled as well.
> > -	 */
> > -	if (error == -ENOENT) {
> > -		printk(KERN_ERR "NFSD: disabling legacy clientid tracking. "
> > -			"Reboot recovery will not function correctly!\n");
> > -		nfsd4_client_tracking_exit(clp->net);
> > -	}
> >  }
> >  
> >  static void
> >  __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
> >  		const char *dname, int len, struct nfsd_net *nn)
> > @@ -180,13 +142,11 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
> >  	if (test_and_set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> >  		return;
> >  	if (!nn->rec_file)
> >  		return;
> >  
> > -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> > -	if (status)
> > -		return legacy_recdir_name_error(clp, status);
> > +	nfs4_make_rec_clidname(dname, &clp->cl_name);
> >  
> >  	status = nfs4_save_creds(&original_cred);
> >  	if (status < 0)
> >  		return;
> >  
> > @@ -374,13 +334,11 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
> >  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> >  
> >  	if (!nn->rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> >  		return;
> >  
> > -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> > -	if (status)
> > -		return legacy_recdir_name_error(clp, status);
> > +	nfs4_make_rec_clidname(dname, &clp->cl_name);
> >  
> >  	status = mnt_want_write_file(nn->rec_file);
> >  	if (status)
> >  		goto out;
> >  	clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
> > @@ -601,10 +559,15 @@ nfsd4_legacy_tracking_init(struct net *net)
> >  	if (net != &init_net) {
> >  		pr_warn("NFSD: attempt to initialize legacy client tracking in a container ignored.\n");
> >  		return -EINVAL;
> >  	}
> >  
> > +	if (fips_enabled) {
> > +		pr_warn("NFSD: legacy client tracking is disabled due to FIPS\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	status = nfs4_legacy_state_init(net);
> >  	if (status)
> >  		return status;
> >  
> >  	status = nfsd4_load_reboot_recovery_data(net);
> > @@ -657,25 +620,20 @@ nfs4_recoverydir(void)
> >  }
> >  
> >  static int
> >  nfsd4_check_legacy_client(struct nfs4_client *clp)
> >  {
> > -	int status;
> >  	char dname[HEXDIR_LEN];
> >  	struct nfs4_client_reclaim *crp;
> >  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> >  	struct xdr_netobj name;
> >  
> >  	/* did we already find that this client is stable? */
> >  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> >  		return 0;
> >  
> > -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> > -	if (status) {
> > -		legacy_recdir_name_error(clp, status);
> > -		return status;
> > -	}
> > +	nfs4_make_rec_clidname(dname, &clp->cl_name);
> >  
> >  	/* look for it in the reclaim hashtable otherwise */
> >  	name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
> >  	if (!name.data) {
> >  		dprintk("%s: failed to allocate memory for name.data!\n",
> > @@ -1264,17 +1222,14 @@ nfsd4_cld_check(struct nfs4_client *clp)
> >  	if (crp)
> >  		goto found;
> >  
> >  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
> >  	if (nn->cld_net->cn_has_legacy) {
> > -		int status;
> >  		char dname[HEXDIR_LEN];
> >  		struct xdr_netobj name;
> >  
> > -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> > -		if (status)
> > -			return -ENOENT;
> > +		nfs4_make_rec_clidname(dname, &clp->cl_name);
> >  
> >  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
> >  		if (!name.data) {
> >  			dprintk("%s: failed to allocate memory for name.data!\n",
> >  				__func__);
> > @@ -1315,15 +1270,12 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
> >  
> >  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
> >  	if (cn->cn_has_legacy) {
> >  		struct xdr_netobj name;
> >  		char dname[HEXDIR_LEN];
> > -		int status;
> >  
> > -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
> > -		if (status)
> > -			return -ENOENT;
> > +		nfs4_make_rec_clidname(dname, &clp->cl_name);
> >  
> >  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
> >  		if (!name.data) {
> >  			dprintk("%s: failed to allocate memory for name.data\n",
> >  					__func__);
> > @@ -1692,15 +1644,11 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
> >  		/* just return nothing if output will be truncated */
> >  		kfree(result);
> >  		return NULL;
> >  	}
> >  
> > -	copied = nfs4_make_rec_clidname(result + copied, name);
> > -	if (copied) {
> > -		kfree(result);
> > -		return NULL;
> > -	}
> > +	nfs4_make_rec_clidname(result + copied, name);
> >  
> >  	return result;
> >  }
> >  
> >  static char *
> > 
> > base-commit: 0739473694c4878513031006829f1030ec850bc2
> 
> 
> -- 
> Chuck Lever
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-13 10:46   ` Scott Mayhew
@ 2025-10-13 13:32     ` Chuck Lever
  0 siblings, 0 replies; 12+ messages in thread
From: Chuck Lever @ 2025-10-13 13:32 UTC (permalink / raw)
  To: Scott Mayhew
  Cc: Eric Biggers, Jeff Layton, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel, linux-nfs

On 10/13/25 6:46 AM, Scott Mayhew wrote:
> On Sun, 12 Oct 2025, Chuck Lever wrote:
> 
>> On 10/11/25 2:52 PM, Eric Biggers wrote:
>>> Update NFSD's support for "legacy client tracking" (which uses MD5) to
>>> use the MD5 library instead of crypto_shash.  This has several benefits:
>>>
>>> - Simpler code.  Notably, much of the error-handling code is no longer
>>>   needed, since the library functions can't fail.
>>>
>>> - Improved performance due to reduced overhead.  A microbenchmark of
>>>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
>>>
>>> - The MD5 code can now safely be built as a loadable module when nfsd is
>>>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
>>>   built-in, presumably to work around the unreliablity of the name-based
>>>   loading.)  Thus, select MD5 from the tristate option NFSD if
>>>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
>>>
>>> To preserve the existing behavior of legacy client tracking support
>>> being disabled when the kernel is booted with "fips=1", make
>>> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
>>> know if this is truly needed, but it preserves the existing behavior.
>>>
>>> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
>>
>> No objection, but let's cross our t's and dot our i's. Scott, when you
>> have recovered from bake-a-thon, can you have a look at this one?
>>
>> Thanks!
> 
> Looks fine to me.
> 
> Reviewed-by: Scott Mayhew <smayhew@redhat.com>

Thank you, sir.


> I agree with Jeff - it would be nice to just remove the legacy tracking.

We're following the common deprecation process. The default setting for
LEGACY_CLIENT_TRACKING is now N.


> I'm guessing it's still used in smaller/embedded setups?  RHEL and Fedora
> haven't had it enabled for years.  Looking at a few other distros... it's
> not enabled in OpenSUSE Leap or Tumbleweed.  It's not enabled in Debian
> Sid (but it is enabled in Trixie).

Hard to say with certainty who still needs it until it is removed. We
could decide here and now to accelerate the deprecation process and pull
it out of v6.19.


>>> ---
>>>  fs/nfsd/Kconfig       |  3 +-
>>>  fs/nfsd/nfs4recover.c | 82 ++++++++-----------------------------------
>>>  2 files changed, 16 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
>>> index e134dce45e350..380a4caa33a73 100644
>>> --- a/fs/nfsd/Kconfig
>>> +++ b/fs/nfsd/Kconfig
>>> @@ -3,10 +3,11 @@ config NFSD
>>>  	tristate "NFS server support"
>>>  	depends on INET
>>>  	depends on FILE_LOCKING
>>>  	depends on FSNOTIFY
>>>  	select CRC32
>>> +	select CRYPTO_LIB_MD5 if NFSD_LEGACY_CLIENT_TRACKING
>>>  	select CRYPTO_LIB_SHA256 if NFSD_V4
>>>  	select LOCKD
>>>  	select SUNRPC
>>>  	select EXPORTFS
>>>  	select NFS_COMMON
>>> @@ -75,12 +76,10 @@ config NFSD_V3_ACL
>>>  config NFSD_V4
>>>  	bool "NFS server support for NFS version 4"
>>>  	depends on NFSD && PROC_FS
>>>  	select FS_POSIX_ACL
>>>  	select RPCSEC_GSS_KRB5
>>> -	select CRYPTO
>>> -	select CRYPTO_MD5
>>>  	select GRACE_PERIOD
>>>  	select NFS_V4_2_SSC_HELPER if NFS_V4_2
>>>  	help
>>>  	  This option enables support in your system's NFS server for
>>>  	  version 4 of the NFS protocol (RFC 3530).
>>> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
>>> index e2b9472e5c78c..dbc0aecef95e3 100644
>>> --- a/fs/nfsd/nfs4recover.c
>>> +++ b/fs/nfsd/nfs4recover.c
>>> @@ -30,13 +30,14 @@
>>>  *  NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>>>  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>>>  *
>>>  */
>>>  
>>> -#include <crypto/hash.h>
>>> +#include <crypto/md5.h>
>>>  #include <crypto/sha2.h>
>>>  #include <linux/file.h>
>>> +#include <linux/fips.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/namei.h>
>>>  #include <linux/sched.h>
>>>  #include <linux/fs.h>
>>>  #include <linux/module.h>
>>> @@ -90,61 +91,22 @@ static void
>>>  nfs4_reset_creds(const struct cred *original)
>>>  {
>>>  	put_cred(revert_creds(original));
>>>  }
>>>  
>>> -static int
>>> +static void
>>>  nfs4_make_rec_clidname(char dname[HEXDIR_LEN], const struct xdr_netobj *clname)
>>>  {
>>>  	u8 digest[MD5_DIGEST_SIZE];
>>> -	struct crypto_shash *tfm;
>>> -	int status;
>>>  
>>>  	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
>>>  			clname->len, clname->data);
>>> -	tfm = crypto_alloc_shash("md5", 0, 0);
>>> -	if (IS_ERR(tfm)) {
>>> -		status = PTR_ERR(tfm);
>>> -		goto out_no_tfm;
>>> -	}
>>>  
>>> -	status = crypto_shash_tfm_digest(tfm, clname->data, clname->len,
>>> -					 digest);
>>> -	if (status)
>>> -		goto out;
>>> +	md5(clname->data, clname->len, digest);
>>>  
>>>  	static_assert(HEXDIR_LEN == 2 * MD5_DIGEST_SIZE + 1);
>>>  	sprintf(dname, "%*phN", MD5_DIGEST_SIZE, digest);
>>> -
>>> -	status = 0;
>>> -out:
>>> -	crypto_free_shash(tfm);
>>> -out_no_tfm:
>>> -	return status;
>>> -}
>>> -
>>> -/*
>>> - * If we had an error generating the recdir name for the legacy tracker
>>> - * then warn the admin. If the error doesn't appear to be transient,
>>> - * then disable recovery tracking.
>>> - */
>>> -static void
>>> -legacy_recdir_name_error(struct nfs4_client *clp, int error)
>>> -{
>>> -	printk(KERN_ERR "NFSD: unable to generate recoverydir "
>>> -			"name (%d).\n", error);
>>> -
>>> -	/*
>>> -	 * if the algorithm just doesn't exist, then disable the recovery
>>> -	 * tracker altogether. The crypto libs will generally return this if
>>> -	 * FIPS is enabled as well.
>>> -	 */
>>> -	if (error == -ENOENT) {
>>> -		printk(KERN_ERR "NFSD: disabling legacy clientid tracking. "
>>> -			"Reboot recovery will not function correctly!\n");
>>> -		nfsd4_client_tracking_exit(clp->net);
>>> -	}
>>>  }
>>>  
>>>  static void
>>>  __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
>>>  		const char *dname, int len, struct nfsd_net *nn)
>>> @@ -180,13 +142,11 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>>>  	if (test_and_set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>>>  		return;
>>>  	if (!nn->rec_file)
>>>  		return;
>>>  
>>> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
>>> -	if (status)
>>> -		return legacy_recdir_name_error(clp, status);
>>> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>>>  
>>>  	status = nfs4_save_creds(&original_cred);
>>>  	if (status < 0)
>>>  		return;
>>>  
>>> @@ -374,13 +334,11 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
>>>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>>  
>>>  	if (!nn->rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>>>  		return;
>>>  
>>> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
>>> -	if (status)
>>> -		return legacy_recdir_name_error(clp, status);
>>> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>>>  
>>>  	status = mnt_want_write_file(nn->rec_file);
>>>  	if (status)
>>>  		goto out;
>>>  	clear_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
>>> @@ -601,10 +559,15 @@ nfsd4_legacy_tracking_init(struct net *net)
>>>  	if (net != &init_net) {
>>>  		pr_warn("NFSD: attempt to initialize legacy client tracking in a container ignored.\n");
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> +	if (fips_enabled) {
>>> +		pr_warn("NFSD: legacy client tracking is disabled due to FIPS\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>  	status = nfs4_legacy_state_init(net);
>>>  	if (status)
>>>  		return status;
>>>  
>>>  	status = nfsd4_load_reboot_recovery_data(net);
>>> @@ -657,25 +620,20 @@ nfs4_recoverydir(void)
>>>  }
>>>  
>>>  static int
>>>  nfsd4_check_legacy_client(struct nfs4_client *clp)
>>>  {
>>> -	int status;
>>>  	char dname[HEXDIR_LEN];
>>>  	struct nfs4_client_reclaim *crp;
>>>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>>>  	struct xdr_netobj name;
>>>  
>>>  	/* did we already find that this client is stable? */
>>>  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
>>>  		return 0;
>>>  
>>> -	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
>>> -	if (status) {
>>> -		legacy_recdir_name_error(clp, status);
>>> -		return status;
>>> -	}
>>> +	nfs4_make_rec_clidname(dname, &clp->cl_name);
>>>  
>>>  	/* look for it in the reclaim hashtable otherwise */
>>>  	name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>>>  	if (!name.data) {
>>>  		dprintk("%s: failed to allocate memory for name.data!\n",
>>> @@ -1264,17 +1222,14 @@ nfsd4_cld_check(struct nfs4_client *clp)
>>>  	if (crp)
>>>  		goto found;
>>>  
>>>  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>>>  	if (nn->cld_net->cn_has_legacy) {
>>> -		int status;
>>>  		char dname[HEXDIR_LEN];
>>>  		struct xdr_netobj name;
>>>  
>>> -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
>>> -		if (status)
>>> -			return -ENOENT;
>>> +		nfs4_make_rec_clidname(dname, &clp->cl_name);
>>>  
>>>  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>>>  		if (!name.data) {
>>>  			dprintk("%s: failed to allocate memory for name.data!\n",
>>>  				__func__);
>>> @@ -1315,15 +1270,12 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
>>>  
>>>  #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>>>  	if (cn->cn_has_legacy) {
>>>  		struct xdr_netobj name;
>>>  		char dname[HEXDIR_LEN];
>>> -		int status;
>>>  
>>> -		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
>>> -		if (status)
>>> -			return -ENOENT;
>>> +		nfs4_make_rec_clidname(dname, &clp->cl_name);
>>>  
>>>  		name.data = kmemdup(dname, HEXDIR_LEN, GFP_KERNEL);
>>>  		if (!name.data) {
>>>  			dprintk("%s: failed to allocate memory for name.data\n",
>>>  					__func__);
>>> @@ -1692,15 +1644,11 @@ nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
>>>  		/* just return nothing if output will be truncated */
>>>  		kfree(result);
>>>  		return NULL;
>>>  	}
>>>  
>>> -	copied = nfs4_make_rec_clidname(result + copied, name);
>>> -	if (copied) {
>>> -		kfree(result);
>>> -		return NULL;
>>> -	}
>>> +	nfs4_make_rec_clidname(result + copied, name);
>>>  
>>>  	return result;
>>>  }
>>>  
>>>  static char *
>>>
>>> base-commit: 0739473694c4878513031006829f1030ec850bc2
>>
>>
>> -- 
>> Chuck Lever
>>
> 


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-11 18:52 [PATCH] nfsd: Use MD5 library instead of crypto_shash Eric Biggers
  2025-10-12 11:12 ` Jeff Layton
  2025-10-12 15:59 ` Chuck Lever
@ 2025-10-14  7:56 ` Ard Biesheuvel
  2 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2025-10-14  7:56 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-nfs, Chuck Lever, Jeff Layton, NeilBrown, Olga Kornievskaia,
	Dai Ngo, Tom Talpey, linux-crypto, linux-kernel

On Sat, 11 Oct 2025 at 20:54, Eric Biggers <ebiggers@kernel.org> wrote:
>
> Update NFSD's support for "legacy client tracking" (which uses MD5) to
> use the MD5 library instead of crypto_shash.  This has several benefits:
>
> - Simpler code.  Notably, much of the error-handling code is no longer
>   needed, since the library functions can't fail.
>
> - Improved performance due to reduced overhead.  A microbenchmark of
>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
>
> - The MD5 code can now safely be built as a loadable module when nfsd is
>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
>   built-in, presumably to work around the unreliablity of the name-based
>   loading.)  Thus, select MD5 from the tristate option NFSD if
>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
>
> To preserve the existing behavior of legacy client tracking support
> being disabled when the kernel is booted with "fips=1", make
> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> know if this is truly needed, but it preserves the existing behavior.
>
> Signed-off-by: Eric Biggers <ebiggers@kernel.org>
> ---
>  fs/nfsd/Kconfig       |  3 +-
>  fs/nfsd/nfs4recover.c | 82 ++++++++-----------------------------------
>  2 files changed, 16 insertions(+), 69 deletions(-)
>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-12 17:00   ` Eric Biggers
  2025-10-12 17:57     ` Simo Sorce
  2025-10-12 18:49     ` Jeff Layton
@ 2025-10-16 13:41     ` Chuck Lever
  2025-10-16 18:07       ` Eric Biggers
  2 siblings, 1 reply; 12+ messages in thread
From: Chuck Lever @ 2025-10-16 13:41 UTC (permalink / raw)
  To: Eric Biggers, Jeff Layton
  Cc: linux-nfs, NeilBrown, Olga Kornievskaia, Dai Ngo, Tom Talpey,
	linux-crypto, linux-kernel

On 10/12/25 1:00 PM, Eric Biggers wrote:
> On Sun, Oct 12, 2025 at 07:12:26AM -0400, Jeff Layton wrote:
>> On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
>>> Update NFSD's support for "legacy client tracking" (which uses MD5) to
>>> use the MD5 library instead of crypto_shash.  This has several benefits:
>>>
>>> - Simpler code.  Notably, much of the error-handling code is no longer
>>>   needed, since the library functions can't fail.
>>>
>>> - Improved performance due to reduced overhead.  A microbenchmark of
>>>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
>>>
>>> - The MD5 code can now safely be built as a loadable module when nfsd is
>>>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
>>>   built-in, presumably to work around the unreliablity of the name-based
>>>   loading.)  Thus, select MD5 from the tristate option NFSD if
>>>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
>>>
>>> To preserve the existing behavior of legacy client tracking support
>>> being disabled when the kernel is booted with "fips=1", make
>>> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
>>> know if this is truly needed, but it preserves the existing behavior.
>>>
>>
>> FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
>> a cryptographically significant way here, but the FIPS gods won't bless
>> a kernel that uses MD5 at all, so I think it is needed.
> 
> If it's not being used for a security purpose, then I think you can just
> drop the fips_enabled check.  People are used to the old API where MD5
> was always forbidden when fips_enabled, but it doesn't actually need to
> be that strict.  For this patch I wasn't certain about the use case
> though, so I just opted to preserve the existing behavior for now.  A
> follow-on patch to remove the check could make sense.
Eric, were you going to follow up with a fresh revision that drops the
fips_enabled check?


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-16 13:41     ` Chuck Lever
@ 2025-10-16 18:07       ` Eric Biggers
  2025-10-16 18:12         ` Chuck Lever
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Biggers @ 2025-10-16 18:07 UTC (permalink / raw)
  To: Chuck Lever
  Cc: Jeff Layton, linux-nfs, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel

On Thu, Oct 16, 2025 at 09:41:27AM -0400, Chuck Lever wrote:
> On 10/12/25 1:00 PM, Eric Biggers wrote:
> > On Sun, Oct 12, 2025 at 07:12:26AM -0400, Jeff Layton wrote:
> >> On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
> >>> Update NFSD's support for "legacy client tracking" (which uses MD5) to
> >>> use the MD5 library instead of crypto_shash.  This has several benefits:
> >>>
> >>> - Simpler code.  Notably, much of the error-handling code is no longer
> >>>   needed, since the library functions can't fail.
> >>>
> >>> - Improved performance due to reduced overhead.  A microbenchmark of
> >>>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
> >>>
> >>> - The MD5 code can now safely be built as a loadable module when nfsd is
> >>>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
> >>>   built-in, presumably to work around the unreliablity of the name-based
> >>>   loading.)  Thus, select MD5 from the tristate option NFSD if
> >>>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
> >>>
> >>> To preserve the existing behavior of legacy client tracking support
> >>> being disabled when the kernel is booted with "fips=1", make
> >>> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
> >>> know if this is truly needed, but it preserves the existing behavior.
> >>>
> >>
> >> FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
> >> a cryptographically significant way here, but the FIPS gods won't bless
> >> a kernel that uses MD5 at all, so I think it is needed.
> > 
> > If it's not being used for a security purpose, then I think you can just
> > drop the fips_enabled check.  People are used to the old API where MD5
> > was always forbidden when fips_enabled, but it doesn't actually need to
> > be that strict.  For this patch I wasn't certain about the use case
> > though, so I just opted to preserve the existing behavior for now.  A
> > follow-on patch to remove the check could make sense.
> Eric, were you going to follow up with a fresh revision that drops the
> fips_enabled check?

Sure, if you want.  I see you're also planning to revert my prerequisite
patch "SUNRPC: Make RPCSEC_GSS_KRB5 select CRYPTO instead of depending
on it".  So I also need to work around that by keeping the
'select CRYPTO' in NFSD_V4.

- Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash
  2025-10-16 18:07       ` Eric Biggers
@ 2025-10-16 18:12         ` Chuck Lever
  0 siblings, 0 replies; 12+ messages in thread
From: Chuck Lever @ 2025-10-16 18:12 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Jeff Layton, linux-nfs, NeilBrown, Olga Kornievskaia, Dai Ngo,
	Tom Talpey, linux-crypto, linux-kernel

On 10/16/25 2:07 PM, Eric Biggers wrote:
> On Thu, Oct 16, 2025 at 09:41:27AM -0400, Chuck Lever wrote:
>> On 10/12/25 1:00 PM, Eric Biggers wrote:
>>> On Sun, Oct 12, 2025 at 07:12:26AM -0400, Jeff Layton wrote:
>>>> On Sat, 2025-10-11 at 11:52 -0700, Eric Biggers wrote:
>>>>> Update NFSD's support for "legacy client tracking" (which uses MD5) to
>>>>> use the MD5 library instead of crypto_shash.  This has several benefits:
>>>>>
>>>>> - Simpler code.  Notably, much of the error-handling code is no longer
>>>>>   needed, since the library functions can't fail.
>>>>>
>>>>> - Improved performance due to reduced overhead.  A microbenchmark of
>>>>>   nfs4_make_rec_clidname() shows a speedup from 1455 cycles to 425.
>>>>>
>>>>> - The MD5 code can now safely be built as a loadable module when nfsd is
>>>>>   built as a loadable module.  (Previously, nfsd forced the MD5 code to
>>>>>   built-in, presumably to work around the unreliablity of the name-based
>>>>>   loading.)  Thus, select MD5 from the tristate option NFSD if
>>>>>   NFSD_LEGACY_CLIENT_TRACKING, instead of from the bool option NFSD_V4.
>>>>>
>>>>> To preserve the existing behavior of legacy client tracking support
>>>>> being disabled when the kernel is booted with "fips=1", make
>>>>> nfsd4_legacy_tracking_init() return an error if fips_enabled.  I don't
>>>>> know if this is truly needed, but it preserves the existing behavior.
>>>>>
>>>>
>>>> FIPS is pretty draconian about algorithms, AIUI. We're not using MD5 in
>>>> a cryptographically significant way here, but the FIPS gods won't bless
>>>> a kernel that uses MD5 at all, so I think it is needed.
>>>
>>> If it's not being used for a security purpose, then I think you can just
>>> drop the fips_enabled check.  People are used to the old API where MD5
>>> was always forbidden when fips_enabled, but it doesn't actually need to
>>> be that strict.  For this patch I wasn't certain about the use case
>>> though, so I just opted to preserve the existing behavior for now.  A
>>> follow-on patch to remove the check could make sense.
>> Eric, were you going to follow up with a fresh revision that drops the
>> fips_enabled check?
> 
> Sure, if you want.  I see you're also planning to revert my prerequisite
> patch "SUNRPC: Make RPCSEC_GSS_KRB5 select CRYPTO instead of depending
> on it".  So I also need to work around that by keeping the
> 'select CRYPTO' in NFSD_V4.

Or we can wait until either that patch is merged again, or the legacy
NFS client tracking implementation is finally removed. Let me know your
timing requirements.


-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-10-16 18:12 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-11 18:52 [PATCH] nfsd: Use MD5 library instead of crypto_shash Eric Biggers
2025-10-12 11:12 ` Jeff Layton
2025-10-12 17:00   ` Eric Biggers
2025-10-12 17:57     ` Simo Sorce
2025-10-12 18:49     ` Jeff Layton
2025-10-16 13:41     ` Chuck Lever
2025-10-16 18:07       ` Eric Biggers
2025-10-16 18:12         ` Chuck Lever
2025-10-12 15:59 ` Chuck Lever
2025-10-13 10:46   ` Scott Mayhew
2025-10-13 13:32     ` Chuck Lever
2025-10-14  7:56 ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox