Re: [PATCH V2 2/2] NFSD: allow client to use write delegation stateid for READ

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-02-21 at 15:42 -0800, Dai Ngo wrote:
> Allow READ using write delegation stateid granted on OPENs with
> OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
> implementation may unavoidably do (e.g., due to buffer cache
> constraints).
> 
> When the server offers a write delegation for an OPEN with
> OPEN4_SHARE_ACCESS_WRITE, the file access mode, the nfs4_file
> and nfs4_ol_stateid are upgraded as if the OPEN was sent with
> OPEN4_SHARE_ACCESS_BOTH.
> 
> When this delegation is returned or revoked, the corresponding open
> stateid is looked up and if it's found then the file access mode,
> the nfs4_file and nfs4_ol_stateid are downgraded to remove the read
> access.
> 
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 62 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/nfsd/state.h     |  2 ++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b533225e57cf..0c14f902c54c 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6126,6 +6126,51 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
>  	return rc == 0;
>  }
>  
> +/*
> + * Upgrade file access mode to include FMODE_READ. This is called only when
> + * a write delegation is granted for an OPEN with OPEN4_SHARE_ACCESS_WRITE.
> + */
> +static void
> +nfs4_upgrade_rdwr_file_access(struct nfs4_ol_stateid *stp)
> +{
> +	struct nfs4_file *fp = stp->st_stid.sc_file;
> +	struct nfsd_file *nflp;
> +	struct file *file;
> +
> +	spin_lock(&fp->fi_lock);
> +	nflp = fp->fi_fds[O_WRONLY];
> +	file = nflp->nf_file;
> +	file->f_mode |= FMODE_READ;

You can't just do this. Open upgrade/downgrade doesn't exist at the VFS
layer currently. It might work with most local filesystems, but more
complex filesystems have significant context attached to a file. Just
because you've changed it here, doesn't mean that you will _actually_
be able to do reads using it.

This might even be actively unsafe, as you're bypassing permissions
checks here. You never checked whether the file is readable. What if
it's write only? Same clients will do an ACCESS check before allowing
it, but a hostile actor might be able to exploit this.

I think you need to acquire a R/W open from the get-go instead when you
intend to grant a delegation, and just fall back to doing a O_WRONLY
open if that fails (and don't grant one).

> +	swap(fp->fi_fds[O_RDWR], fp->fi_fds[O_WRONLY]);
> +	clear_access(NFS4_SHARE_ACCESS_WRITE, stp);
> +	set_access(NFS4_SHARE_ACCESS_BOTH, stp);
> +	__nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ);	/* incr fi_access[O_RDONLY] */
> +	spin_unlock(&fp->fi_lock);
> +}
> +
> +/*
> + * Downgrade file access mode to remove FMODE_READ. This is called when
> + * a write delegation, granted for an OPEN with OPEN4_SHARE_ACCESS_WRITE,
> + * is returned.
> + */
> +static void
> +nfs4_downgrade_wronly_file_access(struct nfs4_ol_stateid *stp)
> +{
> +	struct nfs4_file *fp = stp->st_stid.sc_file;
> +	struct nfsd_file *nflp;
> +	struct file *file;
> +
> +	spin_lock(&fp->fi_lock);
> +	nflp = fp->fi_fds[O_RDWR];
> +	file = nflp->nf_file;
> +	file->f_mode &= ~FMODE_READ;
> +	swap(fp->fi_fds[O_WRONLY], fp->fi_fds[O_RDWR]);
> +	clear_access(NFS4_SHARE_ACCESS_BOTH, stp);
> +	set_access(NFS4_SHARE_ACCESS_WRITE, stp);
> +	spin_unlock(&fp->fi_lock);
> +	nfs4_file_put_access(fp, NFS4_SHARE_ACCESS_READ);	/* decr. fi_access[O_RDONLY] */
> +}
> +
>  /*
>   * The Linux NFS server does not offer write delegations to NFSv4.0
>   * clients in order to avoid conflicts between write delegations and
> @@ -6207,6 +6252,11 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  		dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
>  		dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
>  		trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> +
> +		if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_WRITE) {
> +			dp->dl_stateid = stp->st_stid.sc_stateid;
> +			nfs4_upgrade_rdwr_file_access(stp);
> +		}
>  	} else {
>  		open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_READ_ATTRS_DELEG :
>  						    OPEN_DELEGATE_READ;
> @@ -7710,6 +7760,8 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfs4_stid *s;
>  	__be32 status;
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> +	struct nfs4_ol_stateid *stp;
> +	struct nfs4_stid *stid;
>  
>  	if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0)))
>  		return status;
> @@ -7724,6 +7776,16 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	trace_nfsd_deleg_return(stateid);
>  	destroy_delegation(dp);
> +
> +	if (dp->dl_stateid.si_generation && dp->dl_stateid.si_opaque.so_id) {
> +		if (!nfsd4_lookup_stateid(cstate, &dp->dl_stateid,
> +				SC_TYPE_OPEN, 0, &stid, nn)) {
> +			stp = openlockstateid(stid);
> +			nfs4_downgrade_wronly_file_access(stp);
> +			nfs4_put_stid(stid);
> +		}
> +	}
> +
>  	smp_mb__after_atomic();
>  	wake_up_var(d_inode(cstate->current_fh.fh_dentry));
>  put_stateid:
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 74d2d7b42676..3f2f1b92db66 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -207,6 +207,8 @@ struct nfs4_delegation {
>  
>  	/* for CB_GETATTR */
>  	struct nfs4_cb_fattr    dl_cb_fattr;
> +
> +	stateid_t		dl_stateid;  /* open stateid */
>  };
>  
>  static inline bool deleg_is_read(u32 dl_type)

-- 
Jeff Layton <jlayton@kernel.org>