Re: [PATCH 1/3] nfsd: provide proper locking for all write_ function

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-06-19 at 07:31 +1000, NeilBrown wrote:
> write_foo functions are called to handle IO to files in /proc/fs/nfsd/.
> The can be called at any time and so generally need locking to ensure
> they don't happen at an awkward time.
> 
> Many already take nfsd_mutex and check if nfsd_serv has been set.  This
> ensures they only run when the server is fully configured.
> 
> write_filehandle() does *not* need locking.  It interacts with the
> export table which is set up when the netns is set up, so it is always
> valid and it has its own locking.  write_filehandle() is needed before
> the nfs server is started so checking nfsd_serv would be wrong.
> 
> The remaining files which do not have any locking are
> write_v4_end_grace(), write_unlock_ip(), and write_unlock_fs().
> None of these make sense when the nfs server is not running and there is
> evidence that write_v4_end_grace() can race with ->client_tracking_op
> setup/shutdown and cause problems.
> 
> This patch adds locking to these three and ensures the "unlock"
> functions abort if ->nfsd_serv is not set.
> 
> Reported-by: Li Lingfeng <lilingfeng3@huawei.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfsctl.c | 115 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 77 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 3f3e9f6c4250..3710a1992d17 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -200,27 +200,18 @@ static inline struct net *netns(struct file *file)
>  	return file_inode(file)->i_sb->s_fs_info;
>  }
>  
> -/*
> - * write_unlock_ip - Release all locks used by a client
> - *
> - * Experimental.
> - *
> - * Input:
> - *			buf:	'\n'-terminated C string containing a
> - *				presentation format IP address
> - *			size:	length of C string in @buf
> - * Output:
> - *	On success:	returns zero if all specified locks were released;
> - *			returns one if one or more locks were not released
> - *	On error:	return code is negative errno value
> - */
> -static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
> +static ssize_t __write_unlock_ip(struct file *file, char *buf, size_t size)
>  {
>  	struct sockaddr_storage address;
>  	struct sockaddr *sap = (struct sockaddr *)&address;
>  	size_t salen = sizeof(address);
>  	char *fo_path;
>  	struct net *net = netns(file);
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	if (!nn->nfsd_serv)
> +		/* There cannot be any files to unlock */
> +		return -EINVAL;
>  
>  	/* sanity check */
>  	if (size == 0)
> @@ -241,24 +232,39 @@ static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
>  }
>  
>  /*
> - * write_unlock_fs - Release all locks on a local file system
> + * write_unlock_ip - Release all locks used by a client
>   *
>   * Experimental.
>   *
>   * Input:
> - *			buf:	'\n'-terminated C string containing the
> - *				absolute pathname of a local file system
> + *			buf:	'\n'-terminated C string containing a
> + *				presentation format IP address
>   *			size:	length of C string in @buf
>   * Output:
>   *	On success:	returns zero if all specified locks were released;
>   *			returns one if one or more locks were not released
>   *	On error:	return code is negative errno value
>   */
> -static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
> +static ssize_t write_unlock_ip(struct file *file, char *buf, size_t size)
> +{
> +	ssize_t rv;
> +
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_unlock_ip(file, buf, size);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
> +}
> +
> +static ssize_t __write_unlock_fs(struct file *file, char *buf, size_t size)
>  {
>  	struct path path;
>  	char *fo_path;
>  	int error;
> +	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> +
> +	if (!nn->nfsd_serv)
> +		/* There cannot be any files to unlock */
> +		return -EINVAL;
>  
>  	/* sanity check */
>  	if (size == 0)
> @@ -291,6 +297,30 @@ static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
>  	return error;
>  }
>  
> +/*
> + * write_unlock_fs - Release all locks on a local file system
> + *
> + * Experimental.
> + *
> + * Input:
> + *			buf:	'\n'-terminated C string containing the
> + *				absolute pathname of a local file system
> + *			size:	length of C string in @buf
> + * Output:
> + *	On success:	returns zero if all specified locks were released;
> + *			returns one if one or more locks were not released
> + *	On error:	return code is negative errno value
> + */
> +static ssize_t write_unlock_fs(struct file *file, char *buf, size_t size)
> +{
> +	ssize_t rv;
> +
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_unlock_fs(file, buf, size);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
> +}
> +
>  /*
>   * write_filehandle - Get a variable-length NFS file handle by path
>   *
> @@ -1053,6 +1083,29 @@ 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)
> +{
> +	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> +
> +	if (size > 0) {
> +		switch(buf[0]) {
> +		case 'Y':
> +		case 'y':
> +		case '1':
> +			if (!nn->nfsd_serv)
> +				return -EBUSY;
> +			trace_nfsd_end_grace(netns(file));
> +			nfsd4_end_grace(nn);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
> +			 nn->grace_ended ? 'Y' : 'N');
> +}
> +
>  /*
>   * write_v4_end_grace - release grace period for nfsd's v4.x lock manager
>   *
> @@ -1075,27 +1128,13 @@ static ssize_t write_recoverydir(struct file *file, char *buf, size_t size)
>   */
>  static ssize_t write_v4_end_grace(struct file *file, char *buf, size_t size)
>  {
> -	struct nfsd_net *nn = net_generic(netns(file), nfsd_net_id);
> -
> -	if (size > 0) {
> -		switch(buf[0]) {
> -		case 'Y':
> -		case 'y':
> -		case '1':
> -			if (!nn->nfsd_serv)
> -				return -EBUSY;
> -			trace_nfsd_end_grace(netns(file));
> -			nfsd4_end_grace(nn);
> -			break;
> -		default:
> -			return -EINVAL;
> -		}
> -	}
> +	ssize_t rv;
>  
> -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%c\n",
> -			 nn->grace_ended ? 'Y' : 'N');
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_v4_end_grace(file, buf, size);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
>  }
> -
>  #endif
>  
>  /*----------------------------------------------------------------------------*/

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