Message-ID: <ZSlEQLGtXRP//k2L@tissot.1015granger.net>
Date:   Fri, 13 Oct 2023 09:21:04 -0400
From:   Chuck Lever <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>
Cc:     Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nfsd: new Kconfig option for legacy client tracking

On Fri, Oct 13, 2023 at 09:03:53AM -0400, Jeff Layton wrote:
> We've had a number of attempts at different NFSv4 client tracking
> methods over the years, but now nfsdcld has emerged as the clear winner
> since the others (recoverydir and the usermodehelper upcall) are
> problematic.
> 
> As a case in point, the recoverydir backend uses MD5 hashes to encode
> long form clientid strings, which means that nfsd repeatedly gets dinged
> on FIPS audits, since MD5 isn't considered secure. Its use of MD5 is not
> cryptographically significant, so there is no danger there, but allowing
> us to compile that out allows us to sidestep the issue entirely.
> 
> As a prelude to eventually removing support for these client tracking
> methods, add a new Kconfig option that enables them. Mark it deprecated
> and make it default to N.
> 
> Acked-by: NeilBrown <neilb@...e.de>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
> Now that we've really settled on nfsdcld being the way forward for
> NFSv4 client tracking, put the legacy methods under a new Kconfig option
> that defaults to off.
> 
> This should make it easier to eventually deprecate this code and remove
> it in the future (maybe in v6.10 or so)?
> ---
>  fs/nfsd/Kconfig       | 16 +++++++++
>  fs/nfsd/nfs4recover.c | 97 +++++++++++++++++++++++++++++++++------------------
>  fs/nfsd/nfsctl.c      |  6 ++++
>  3 files changed, 85 insertions(+), 34 deletions(-)

LGTM.

I've kind of closed out 6.7 at this point for anything but fixes.
I've applied this to a private "futures" branch, planning for 6.8.

If you think this should go in earlier, let me know.


> diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> index 43b88eaf0673..272ab8d5c4d7 100644
> --- a/fs/nfsd/Kconfig
> +++ b/fs/nfsd/Kconfig
> @@ -158,3 +158,19 @@ config NFSD_V4_SECURITY_LABEL
>  
>  	If you do not wish to enable fine-grained security labels SELinux or
>  	Smack policies on NFSv4 files, say N.
> +
> +config NFSD_LEGACY_CLIENT_TRACKING
> +	bool "Support legacy NFSv4 client tracking methods (DEPRECATED)"
> +	depends on NFSD_V4
> +	default n
> +	help
> +	  The NFSv4 server needs to store a small amount of information on
> +	  stable storage in order to handle state recovery after reboot. Most
> +	  modern deployments upcall to a userland daemon for this (nfsdcld),
> +	  but older NFS servers may store information directly in a
> +	  recoverydir, or spawn a process directly using a usermodehelper
> +	  upcall.
> +
> +	  These legacy client tracking methods have proven to be probelmatic
> +	  and will be removed in the future. Say Y here if you need support
> +	  for them in the interim.
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 3509e73abe1f..2c060e0b1604 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -66,6 +66,7 @@ struct nfsd4_client_tracking_ops {
>  static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops;
>  static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops_v2;
>  
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  /* Globals */
>  static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery";
>  
> @@ -720,6 +721,7 @@ static const struct nfsd4_client_tracking_ops nfsd4_legacy_tracking_ops = {
>  	.version	= 1,
>  	.msglen		= 0,
>  };
> +#endif /* CONFIG_NFSD_LEGACY_CLIENT_TRACKING */
>  
>  /* Globals */
>  #define NFSD_PIPE_DIR		"nfsd"
> @@ -731,8 +733,10 @@ struct cld_net {
>  	spinlock_t		 cn_lock;
>  	struct list_head	 cn_list;
>  	unsigned int		 cn_xid;
> -	bool			 cn_has_legacy;
>  	struct crypto_shash	*cn_tfm;
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
> +	bool			 cn_has_legacy;
> +#endif
>  };
>  
>  struct cld_upcall {
> @@ -793,7 +797,6 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
>  	uint8_t cmd, princhashlen;
>  	struct xdr_netobj name, princhash = { .len = 0, .data = NULL };
>  	uint16_t namelen;
> -	struct cld_net *cn = nn->cld_net;
>  
>  	if (get_user(cmd, &cmsg->cm_cmd)) {
>  		dprintk("%s: error when copying cmd from userspace", __func__);
> @@ -833,11 +836,15 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
>  				return PTR_ERR(name.data);
>  			name.len = namelen;
>  		}
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  		if (name.len > 5 && memcmp(name.data, "hash:", 5) == 0) {
> +			struct cld_net *cn = nn->cld_net;
> +
>  			name.len = name.len - 5;
>  			memmove(name.data, name.data + 5, name.len);
>  			cn->cn_has_legacy = true;
>  		}
> +#endif
>  		if (!nfs4_client_to_reclaim(name, princhash, nn)) {
>  			kfree(name.data);
>  			kfree(princhash.data);
> @@ -1010,7 +1017,9 @@ __nfsd4_init_cld_pipe(struct net *net)
>  	}
>  
>  	cn->cn_pipe->dentry = dentry;
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	cn->cn_has_legacy = false;
> +#endif
>  	nn->cld_net = cn;
>  	return 0;
>  
> @@ -1282,10 +1291,6 @@ nfsd4_cld_check(struct nfs4_client *clp)
>  {
>  	struct nfs4_client_reclaim *crp;
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> -	struct cld_net *cn = nn->cld_net;
> -	int status;
> -	char dname[HEXDIR_LEN];
> -	struct xdr_netobj name;
>  
>  	/* did we already find that this client is stable? */
>  	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
> @@ -1296,7 +1301,12 @@ nfsd4_cld_check(struct nfs4_client *clp)
>  	if (crp)
>  		goto found;
>  
> -	if (cn->cn_has_legacy) {
> +#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;
> @@ -1314,6 +1324,7 @@ nfsd4_cld_check(struct nfs4_client *clp)
>  			goto found;
>  
>  	}
> +#endif
>  	return -ENOENT;
>  found:
>  	crp->cr_clp = clp;
> @@ -1327,8 +1338,6 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
>  	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
>  	struct cld_net *cn = nn->cld_net;
>  	int status;
> -	char dname[HEXDIR_LEN];
> -	struct xdr_netobj name;
>  	struct crypto_shash *tfm = cn->cn_tfm;
>  	struct xdr_netobj cksum;
>  	char *principal = NULL;
> @@ -1342,7 +1351,11 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
>  	if (crp)
>  		goto found;
>  
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	if (cn->cn_has_legacy) {
> +		struct xdr_netobj name;
> +		char dname[HEXDIR_LEN];
> +
>  		status = nfs4_make_rec_clidname(dname, &clp->cl_name);
>  		if (status)
>  			return -ENOENT;
> @@ -1360,6 +1373,7 @@ nfsd4_cld_check_v2(struct nfs4_client *clp)
>  			goto found;
>  
>  	}
> +#endif
>  	return -ENOENT;
>  found:
>  	if (crp->cr_princhash.len) {
> @@ -1663,6 +1677,7 @@ static const struct nfsd4_client_tracking_ops nfsd4_cld_tracking_ops_v2 = {
>  	.msglen		= sizeof(struct cld_msg_v2),
>  };
>  
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  /* upcall via usermodehelper */
>  static char cltrack_prog[PATH_MAX] = "/sbin/nfsdcltrack";
>  module_param_string(cltrack_prog, cltrack_prog, sizeof(cltrack_prog),
> @@ -2007,28 +2022,10 @@ static const struct nfsd4_client_tracking_ops nfsd4_umh_tracking_ops = {
>  	.msglen		= 0,
>  };
>  
> -int
> -nfsd4_client_tracking_init(struct net *net)
> +static inline int check_for_legacy_methods(int status, struct net *net)
>  {
> -	int status;
> -	struct path path;
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> -
> -	/* just run the init if it the method is already decided */
> -	if (nn->client_tracking_ops)
> -		goto do_init;
> -
> -	/* First, try to use nfsdcld */
> -	nn->client_tracking_ops = &nfsd4_cld_tracking_ops;
> -	status = nn->client_tracking_ops->init(net);
> -	if (!status)
> -		return status;
> -	if (status != -ETIMEDOUT) {
> -		nn->client_tracking_ops = &nfsd4_cld_tracking_ops_v0;
> -		status = nn->client_tracking_ops->init(net);
> -		if (!status)
> -			return status;
> -	}
> +	struct path path;
>  
>  	/*
>  	 * Next, try the UMH upcall.
> @@ -2045,14 +2042,46 @@ nfsd4_client_tracking_init(struct net *net)
>  	nn->client_tracking_ops = &nfsd4_legacy_tracking_ops;
>  	status = kern_path(nfs4_recoverydir(), LOOKUP_FOLLOW, &path);
>  	if (!status) {
> -		status = d_is_dir(path.dentry);
> +		status = !d_is_dir(path.dentry);
>  		path_put(&path);
> -		if (!status) {
> -			status = -EINVAL;
> -			goto out;
> -		}
> +		if (status)
> +			return -ENOTDIR;
> +		status = nn->client_tracking_ops->init(net);
> +	}
> +	return status;
> +}
> +#else
> +static inline int check_for_legacy_methods(int status, struct net *net)
> +{
> +	return status;
> +}
> +#endif /* CONFIG_LEGACY_NFSD_CLIENT_TRACKING */
> +
> +int
> +nfsd4_client_tracking_init(struct net *net)
> +{
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	int status;
> +
> +	/* just run the init if it the method is already decided */
> +	if (nn->client_tracking_ops)
> +		goto do_init;
> +
> +	/* First, try to use nfsdcld */
> +	nn->client_tracking_ops = &nfsd4_cld_tracking_ops;
> +	status = nn->client_tracking_ops->init(net);
> +	if (!status)
> +		return status;
> +	if (status != -ETIMEDOUT) {
> +		nn->client_tracking_ops = &nfsd4_cld_tracking_ops_v0;
> +		status = nn->client_tracking_ops->init(net);
> +		if (!status)
> +			return status;
>  	}
>  
> +	status = check_for_legacy_methods(status, net);
> +	if (status)
> +		goto out;
>  do_init:
>  	status = nn->client_tracking_ops->init(net);
>  out:
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7ed02fb88a36..48d1dc9cccfb 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -75,7 +75,9 @@ static ssize_t write_maxconn(struct file *file, char *buf, size_t size);
>  #ifdef CONFIG_NFSD_V4
>  static ssize_t write_leasetime(struct file *file, char *buf, size_t size);
>  static ssize_t write_gracetime(struct file *file, char *buf, size_t size);
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  static ssize_t write_recoverydir(struct file *file, char *buf, size_t size);
> +#endif
>  static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size);
>  #endif
>  
> @@ -92,7 +94,9 @@ static ssize_t (*const write_op[])(struct file *, char *, size_t) = {
>  #ifdef CONFIG_NFSD_V4
>  	[NFSD_Leasetime] = write_leasetime,
>  	[NFSD_Gracetime] = write_gracetime,
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	[NFSD_RecoveryDir] = write_recoverydir,
> +#endif
>  	[NFSD_V4EndGrace] = write_v4_end_grace,
>  #endif
>  };
> @@ -1012,6 +1016,7 @@ static ssize_t write_gracetime(struct file *file, char *buf, size_t size)
>  	return nfsd4_write_time(file, buf, size, &nn->nfsd4_grace, nn);
>  }
>  
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  static ssize_t __write_recoverydir(struct file *file, char *buf, size_t size,
>  				   struct nfsd_net *nn)
>  {
> @@ -1072,6 +1077,7 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
>  	mutex_unlock(&nfsd_mutex);
>  	return rv;
>  }
> +#endif
>  
>  /*
>   * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
> 
> ---
> base-commit: 401644852d0b2a278811de38081be23f74b5bb04
> change-id: 20231012-nfsd-cltrack-6198814aee58
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@...nel.org>
> 

-- 
Chuck Lever

Powered by blists - more mailing lists