Message-ID: <04b83f7f-8dff-4b1a-a85d-c44a6c517e5a@oracle.com>
Date: Mon, 13 Oct 2025 09:32:18 -0400
From: Chuck Lever <chuck.lever@...cle.com>
To: Scott Mayhew <smayhew@...hat.com>
Cc: Eric Biggers <ebiggers@...nel.org>, Jeff Layton <jlayton@...nel.org>,
        NeilBrown <neil@...wn.name>, Olga Kornievskaia <okorniev@...hat.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-nfs@...r.kernel.org
Subject: Re: [PATCH] nfsd: Use MD5 library instead of crypto_shash

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@...nel.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@...hat.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

Powered by blists - more mailing lists