Re: [PATCH v4] nfsd: disallow file locking and delegations for NFSv4 reexport

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2025-02-10 at 11:25 -0500, cel@kernel.org wrote:
> From: Mike Snitzer <snitzer@kernel.org>
> 
> We do not and cannot support file locking with NFS reexport over
> NFSv4.x for the same reason we don't do it for NFSv3: NFS reexport
> server reboot cannot allow clients to recover locks because the source
> NFS server has not rebooted, and so it is not in grace. 
> 

Refresh my memory: how do we prevent this in v3? It seems like NLM
should use the same mechanism.

>  Since the
> source NFS server is not in grace, it cannot offer any guarantees that
> the file won't have been changed between the locks getting lost and
> any attempt to recover/reclaim them.  The same applies to delegations
> and any associated locks, so disallow them too.
> 
> Clients are no longer allowed to get file locks or delegations from a
> reexport server, any attempts will fail with operation not supported.
> 
> Update the "Reboot recovery" section accordingly in
> Documentation/filesystems/nfs/reexport.rst
> 
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  Documentation/filesystems/nfs/reexport.rst | 10 +++++++---
>  fs/nfs/export.c                            |  3 ++-
>  fs/nfsd/nfs4state.c                        | 18 ++++++++++++++++++
>  include/linux/exportfs.h                   | 14 +++++++++++++-
>  4 files changed, 40 insertions(+), 5 deletions(-)
> 
> 
> Jeff and I believe that it is better to prevent data corruption for
> now than to continue to enable risky use cases. Thus I've forward-
> ported this patch to v6.14-rc2 and intend to include it in the
> nfsd-6.15 pull request.
> 
> We remain open to considering solutions that enable locking through
> to the back-end NFS server with appropriate lock recovery.
> 
> 
> diff --git a/Documentation/filesystems/nfs/reexport.rst b/Documentation/filesystems/nfs/reexport.rst
> index ff9ae4a46530..044be965d75e 100644
> --- a/Documentation/filesystems/nfs/reexport.rst
> +++ b/Documentation/filesystems/nfs/reexport.rst
> @@ -26,9 +26,13 @@ Reboot recovery
>  ---------------
>  
>  The NFS protocol's normal reboot recovery mechanisms don't work for the
> -case when the reexport server reboots.  Clients will lose any locks
> -they held before the reboot, and further IO will result in errors.
> -Closing and reopening files should clear the errors.
> +case when the reexport server reboots because the source server has not
> +rebooted, and so it is not in grace.  Since the source server is not in
> +grace, it cannot offer any guarantees that the file won't have been
> +changed between the locks getting lost and any attempt to recover them.
> +The same applies to delegations and any associated locks.  Clients are
> +not allowed to get file locks or delegations from a reexport server, any
> +attempts will fail with operation not supported.
>  
>  Filehandle limits
>  -----------------
> diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> index be686b8e0c54..e9c233b6fd20 100644
> --- a/fs/nfs/export.c
> +++ b/fs/nfs/export.c
> @@ -154,5 +154,6 @@ const struct export_operations nfs_export_ops = {
>  		 EXPORT_OP_CLOSE_BEFORE_UNLINK	|
>  		 EXPORT_OP_REMOTE_FS		|
>  		 EXPORT_OP_NOATOMIC_ATTR	|
> -		 EXPORT_OP_FLUSH_ON_CLOSE,
> +		 EXPORT_OP_FLUSH_ON_CLOSE	|
> +		 EXPORT_OP_NOLOCKS,
>  };
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b42e2ab7a042..f0fb6ca4b70c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6000,6 +6000,15 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (!nf)
>  		return ERR_PTR(-EAGAIN);
>  
> +	/*
> +	 * File delegations and associated locks cannot be recovered if the
> +	 * export is from an NFS proxy server.
> +	 */
> +	if (exportfs_cannot_lock(nf->nf_file->f_path.mnt->mnt_sb->s_export_op)) {
> +		nfsd_file_put(nf);
> +		return ERR_PTR(-EOPNOTSUPP);
> +	}
> +
>  	spin_lock(&state_lock);
>  	spin_lock(&fp->fi_lock);
>  	if (nfs4_delegation_exists(clp, fp))
> @@ -8140,6 +8149,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0);
>  	if (status != nfs_ok)
>  		return status;
> +	if (exportfs_cannot_lock(cstate->current_fh.fh_dentry->d_sb->s_export_op)) {
> +		status = nfserr_notsupp;
> +		goto out;
> +	}
>  
>  	if (lock->lk_is_new) {
>  		if (nfsd4_has_session(cstate))
> @@ -8479,6 +8492,11 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_lock_range;
>  		goto put_stateid;
>  	}
> +	if (exportfs_cannot_lock(nf->nf_file->f_path.mnt->mnt_sb->s_export_op)) {
> +		status = nfserr_notsupp;
> +		goto put_file;
> +	}
> +
>  	file_lock = locks_alloc_lock();
>  	if (!file_lock) {
>  		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 4cc8801e50e3..ca6c605072c7 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -259,10 +259,22 @@ struct export_operations {
>  						  atomic attribute updates
>  						*/
>  #define EXPORT_OP_FLUSH_ON_CLOSE	(0x20) /* fs flushes file data on close */
> -#define EXPORT_OP_ASYNC_LOCK		(0x40) /* fs can do async lock request */
> +#define EXPORT_OP_NOLOCKS		(0x40) /* no file locking support */
>  	unsigned long	flags;
>  };
>  
> +/**
> + * exportfs_cannot_lock() - check if export implements file locking
> + * @export_ops:	the nfs export operations to check
> + *
> + * Returns true if the export does not support file locking.
> + */
> +static inline bool
> +exportfs_cannot_lock(const struct export_operations *export_ops)
> +{
> +	return export_ops->flags & EXPORT_OP_NOLOCKS;
> +}
> +
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  				    int *max_len, struct inode *parent,
>  				    int flags);


Aside from the question about NLM, this looks good:

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