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

[Chuck Lever <chuck.lever@oracle.com>]

On Wed, Oct 23, 2024 at 10:54:36AM -0400, Mike Snitzer wrote:
> We do not and cannot support NFS proxy server file locking over
> NFSv4.x for the same reason we don't do it for NFSv3: NFS proxy server
> reboot cannot allow clients to recover locks because the source NFS
> server has not rebooted, and so it is not in grace.  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.
> 
> Add EXPORT_OP_NOLOCKSUPPORT and exportfs_lock_op_is_unsupported(), set
> EXPORT_OP_NOLOCKSUPPORT in nfs_export_ops and check for it in
> nfsd4_lock(), nfsd4_locku() and nfs4_set_delegation().
> 
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfs/export.c          |  3 ++-
>  fs/nfsd/nfs4state.c      | 20 ++++++++++++++++++++
>  include/linux/exportfs.h | 14 ++++++++++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/export.c b/fs/nfs/export.c
> index be686b8e0c54..2f001a0273bc 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_NOLOCKSUPPORT,
>  };
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ac1859c7cc9d..63297ea82e4e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5813,6 +5813,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
> +	 * export is from NFS proxy server.
> +	 */
> +	if (exportfs_lock_op_is_unsupported(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))
> @@ -7917,6 +7926,11 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	}
>  	sb = cstate->current_fh.fh_dentry->d_sb;
>  
> +	if (exportfs_lock_op_is_unsupported(sb->s_export_op)) {
> +		status = nfserr_notsupp;
> +		goto out;
> +	}
> +
>  	if (lock->lk_is_new) {
>  		if (nfsd4_has_session(cstate))
>  			/* See rfc 5661 18.10.3: given clientid is ignored: */
> @@ -8266,6 +8280,12 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserr_lock_range;
>  		goto put_stateid;
>  	}
> +
> +	if (exportfs_lock_op_is_unsupported(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 893a1d21dc1c..106fd590d323 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -247,6 +247,7 @@ struct export_operations {
>  						*/
>  #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_NOLOCKSUPPORT		(0x80) /* no file locking support */
>  	unsigned long	flags;
>  };
>  
> @@ -263,6 +264,19 @@ exportfs_lock_op_is_async(const struct export_operations *export_ops)
>  	return export_ops->flags & EXPORT_OP_ASYNC_LOCK;
>  }
>  
> +/**
> + * exportfs_lock_op_is_unsupported() - export does not support file locking
> + * @export_ops:	the nfs export operations to check
> + *
> + * Returns true if the nfs export_operations structure has
> + * EXPORT_OP_NOLOCKSUPPORT in their flags set
> + */
> +static inline bool
> +exportfs_lock_op_is_unsupported(const struct export_operations *export_ops)
> +{
> +	return export_ops->flags & EXPORT_OP_NOLOCKSUPPORT;
> +}
> +
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  				    int *max_len, struct inode *parent,
>  				    int flags);
> -- 
> 2.44.0
> 

Re: this patch:

I expect that this behavior will be surprising to users and
administrators. It would be great to have some documentation for NFS
re-export / proxy somewhere prominent that explains this (and other)
limitations for NFS proxy. Any thoughts, please share.

General comment:

We have talked about adding NFS re-export to our kdevops CI for
upstream NFSD, but that hasn't happened yet. I'm wondering if there
is any other regular testing of re-export.

-- 
Chuck Lever