[v3] nfsd: don't hand out write delegations on O_WRONLY opens

  I noticed that xfstests generic/001 was failing against linux-next nfsd.

The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
would hand out a write delegation. The client would then try to use that
write delegation as the source stateid in a COPY or CLONE operation, and
the server would respond with NFS4ERR_STALE.

The problem is that the struct file associated with the delegation does
not necessarily have read permissions. It's handing out a write
delegation on what is effectively an O_WRONLY open. RFC 8881 states:

 "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
  own, all opens."

Given that the client didn't request any read permissions, and that nfsd
didn't check for any, it seems wrong to give out a write delegation.

Only hand out a write delegation if we have a O_RDWR descriptor
available. If it fails to find an appropriate write descriptor, go
ahead and try for a read delegation if NFS4_SHARE_ACCESS_READ was
requested.

This fixes xfstest generic/001.

Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v3:
- add find_rw_file helper to ensure spinlock is taken appropriately
- refine comments over conditionals
- Link to v2: https://lore.kernel.org/r/20230801-wdeleg-v2-1-20c14252bab4@kernel.org

Changes in v2:
- Rework the logic when finding struct file for the delegation. The
  earlier patch might still have attached a O_WRONLY file to the deleg
  in some cases, and could still have handed out a write delegation on
  an O_WRONLY OPEN request in some cases.
---
 fs/nfsd/nfs4state.c | 48 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 11 deletions(-)


---
base-commit: a734662572708cf062e974f659ae50c24fc1ad17
change-id: 20230731-wdeleg-bbdb6b25a3c6

Best regards,
  

On Thu, 03 Aug 2023, Jeff Layton wrote:
> I noticed that xfstests generic/001 was failing against linux-next nfsd.
> 
> The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
> would hand out a write delegation. The client would then try to use that
> write delegation as the source stateid in a COPY or CLONE operation, and
> the server would respond with NFS4ERR_STALE.
> 
> The problem is that the struct file associated with the delegation does
> not necessarily have read permissions. It's handing out a write
> delegation on what is effectively an O_WRONLY open. RFC 8881 states:
> 
>  "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
>   own, all opens."
> 
> Given that the client didn't request any read permissions, and that nfsd
> didn't check for any, it seems wrong to give out a write delegation.
> 
> Only hand out a write delegation if we have a O_RDWR descriptor
> available. If it fails to find an appropriate write descriptor, go
> ahead and try for a read delegation if NFS4_SHARE_ACCESS_READ was
> requested.
> 
> This fixes xfstest generic/001.
> 
> Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Changes in v3:
> - add find_rw_file helper to ensure spinlock is taken appropriately
> - refine comments over conditionals
> - Link to v2: https://lore.kernel.org/r/20230801-wdeleg-v2-1-20c14252bab4@kernel.org
> 
> Changes in v2:
> - Rework the logic when finding struct file for the delegation. The
>   earlier patch might still have attached a O_WRONLY file to the deleg
>   in some cases, and could still have handed out a write delegation on
>   an O_WRONLY OPEN request in some cases.
> ---
>  fs/nfsd/nfs4state.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ef7118ebee00..c551784d108a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -649,6 +649,18 @@ find_readable_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> +static struct nfsd_file *
> +find_rw_file(struct nfs4_file *f)
> +{
> +	struct nfsd_file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
> +	spin_unlock(&f->fi_lock);
> +
> +	return ret;
> +}
> +
>  struct nfsd_file *
>  find_any_file(struct nfs4_file *f)
>  {
> @@ -5449,7 +5461,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	struct nfs4_file *fp = stp->st_stid.sc_file;
>  	struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate;
>  	struct nfs4_delegation *dp;
> -	struct nfsd_file *nf;
> +	struct nfsd_file *nf = NULL;
>  	struct file_lock *fl;
>  	u32 dl_type;
>  
> @@ -5461,21 +5473,35 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (fp->fi_had_conflict)
>  		return ERR_PTR(-EAGAIN);
>  
> -	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> -		nf = find_writeable_file(fp);
> +	/*
> +	 * Try for a write delegation first. RFC8881 section 10.4 says:
> +	 *
> +	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
> +	 *   on its own, all opens."
> +	 *
> +	 * Furthermore the client can use a write delegationf or most read
> +	 * operations as well, so we require a O_RDWR file here.
> +	 *
> +	 * Only a write delegation in the case of a BOTH open, and ensure
> +	 * we get the O_RDWR descriptor.
> +	 */

This comment isn't working for me, and it isn't just the need for
    s/f / f/
Neither the "Furthermore" or the "Only a" seem to make sense.

I think the key take away from the RFC quote is "all opens" and that
implies "opens for read".  i.e. all delegations imply read access.
So I would then start the code with

    if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ)) 
         return ERR_PTR(-EACCES);

then choose between readable and rw.
So the comment would say:

	 * RFC8881 section 10.4 says:
	 *
	 *  "An OPEN_DELEGATE_READ delegation allows a client to handle, 
	 *   on its own, requests to open a file for reading ...."
	 * and
	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
	 *   on its own, all opens."
	 * and as "all" includes "for reading", any delegation must
	 * allow reading.  So if the original access is write-only we
	 * do not return a delegation, otherwise we require at least
	 * "readable", to return a DELGATE_READ and "rw" to return
	 * DELEGATE_WRITE which we only try if the original open
	 * requested write access.

Code looks good, though I find the growth of find_foo_file APIs
aesthetically unpleasant. 
NeilBrown


> +	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
> +		nf = find_rw_file(fp);
>  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
> -	} else {
> +	}
> +
> +	/*
> +	 * If the file is being opened O_RDONLY or we couldn't get a O_RDWR
> +	 * file for some reason, then try for a read deleg instead.
> +	 */
> +	if (!nf && (open->op_share_access & NFS4_SHARE_ACCESS_READ)) {
>  		nf = find_readable_file(fp);
>  		dl_type = NFS4_OPEN_DELEGATE_READ;
>  	}
> -	if (!nf) {
> -		/*
> -		 * We probably could attempt another open and get a read
> -		 * delegation, but for now, don't bother until the
> -		 * client actually sends us one.
> -		 */
> +
> +	if (!nf)
>  		return ERR_PTR(-EAGAIN);
> -	}
> +
>  	spin_lock(&state_lock);
>  	spin_lock(&fp->fi_lock);
>  	if (nfs4_delegation_exists(clp, fp))
> 
> ---
> base-commit: a734662572708cf062e974f659ae50c24fc1ad17
> change-id: 20230731-wdeleg-bbdb6b25a3c6
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
>
  

> On Aug 2, 2023, at 4:48 PM, NeilBrown <neilb@suse.de> wrote:
> 
> On Thu, 03 Aug 2023, Jeff Layton wrote:
>> I noticed that xfstests generic/001 was failing against linux-next nfsd.
>> 
>> The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
>> would hand out a write delegation. The client would then try to use that
>> write delegation as the source stateid in a COPY or CLONE operation, and
>> the server would respond with NFS4ERR_STALE.
>> 
>> The problem is that the struct file associated with the delegation does
>> not necessarily have read permissions. It's handing out a write
>> delegation on what is effectively an O_WRONLY open. RFC 8881 states:
>> 
>> "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
>>  own, all opens."
>> 
>> Given that the client didn't request any read permissions, and that nfsd
>> didn't check for any, it seems wrong to give out a write delegation.
>> 
>> Only hand out a write delegation if we have a O_RDWR descriptor
>> available. If it fails to find an appropriate write descriptor, go
>> ahead and try for a read delegation if NFS4_SHARE_ACCESS_READ was
>> requested.
>> 
>> This fixes xfstest generic/001.
>> 
>> Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>> Changes in v3:
>> - add find_rw_file helper to ensure spinlock is taken appropriately
>> - refine comments over conditionals
>> - Link to v2: https://lore.kernel.org/r/20230801-wdeleg-v2-1-20c14252bab4@kernel.org
>> 
>> Changes in v2:
>> - Rework the logic when finding struct file for the delegation. The
>>  earlier patch might still have attached a O_WRONLY file to the deleg
>>  in some cases, and could still have handed out a write delegation on
>>  an O_WRONLY OPEN request in some cases.
>> ---
>> fs/nfsd/nfs4state.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 37 insertions(+), 11 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index ef7118ebee00..c551784d108a 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -649,6 +649,18 @@ find_readable_file(struct nfs4_file *f)
>> return ret;
>> }
>> 
>> +static struct nfsd_file *
>> +find_rw_file(struct nfs4_file *f)
>> +{
>> + struct nfsd_file *ret;
>> +
>> + spin_lock(&f->fi_lock);
>> + ret = nfsd_file_get(f->fi_fds[O_RDWR]);
>> + spin_unlock(&f->fi_lock);
>> +
>> + return ret;
>> +}
>> +
>> struct nfsd_file *
>> find_any_file(struct nfs4_file *f)
>> {
>> @@ -5449,7 +5461,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> struct nfs4_file *fp = stp->st_stid.sc_file;
>> struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate;
>> struct nfs4_delegation *dp;
>> - struct nfsd_file *nf;
>> + struct nfsd_file *nf = NULL;
>> struct file_lock *fl;
>> u32 dl_type;
>> 
>> @@ -5461,21 +5473,35 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> if (fp->fi_had_conflict)
>> return ERR_PTR(-EAGAIN);
>> 
>> - if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>> - nf = find_writeable_file(fp);
>> + /*
>> +  * Try for a write delegation first. RFC8881 section 10.4 says:
>> +  *
>> +  *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
>> +  *   on its own, all opens."
>> +  *
>> +  * Furthermore the client can use a write delegationf or most read
>> +  * operations as well, so we require a O_RDWR file here.
>> +  *
>> +  * Only a write delegation in the case of a BOTH open, and ensure
>> +  * we get the O_RDWR descriptor.
>> +  */
> 
> This comment isn't working for me, and it isn't just the need for
>    s/f / f/
> Neither the "Furthermore" or the "Only a" seem to make sense.

I changed this to "Offer a write delegation in the case ..."
when I applied it.


> I think the key take away from the RFC quote is "all opens" and that
> implies "opens for read".  i.e. all delegations imply read access.
> So I would then start the code with
> 
>    if (!(open->op_share_access & NFS4_SHARE_ACCESS_READ)) 
>         return ERR_PTR(-EACCES);
> 
> then choose between readable and rw.
> So the comment would say:
> 
>  * RFC8881 section 10.4 says:
>  *
>  *  "An OPEN_DELEGATE_READ delegation allows a client to handle, 
>  *   on its own, requests to open a file for reading ...."
>  * and
>  *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
>  *   on its own, all opens."
>  * and as "all" includes "for reading", any delegation must
>  * allow reading.  So if the original access is write-only we
>  * do not return a delegation, otherwise we require at least
>  * "readable", to return a DELGATE_READ and "rw" to return
>  * DELEGATE_WRITE which we only try if the original open
>  * requested write access.
> 
> Code looks good, though I find the growth of find_foo_file APIs
> aesthetically unpleasant. 
> NeilBrown
> 
> 
>> + if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
>> + nf = find_rw_file(fp);
>> dl_type = NFS4_OPEN_DELEGATE_WRITE;
>> - } else {
>> + }
>> +
>> + /*
>> +  * If the file is being opened O_RDONLY or we couldn't get a O_RDWR
>> +  * file for some reason, then try for a read deleg instead.
>> +  */
>> + if (!nf && (open->op_share_access & NFS4_SHARE_ACCESS_READ)) {
>> nf = find_readable_file(fp);
>> dl_type = NFS4_OPEN_DELEGATE_READ;
>> }
>> - if (!nf) {
>> - /*
>> -  * We probably could attempt another open and get a read
>> -  * delegation, but for now, don't bother until the
>> -  * client actually sends us one.
>> -  */
>> +
>> + if (!nf)
>> return ERR_PTR(-EAGAIN);
>> - }
>> +
>> spin_lock(&state_lock);
>> spin_lock(&fp->fi_lock);
>> if (nfs4_delegation_exists(clp, fp))
>> 
>> ---
>> base-commit: a734662572708cf062e974f659ae50c24fc1ad17
>> change-id: 20230731-wdeleg-bbdb6b25a3c6
>> 
>> Best regards,
>> -- 
>> Jeff Layton <jlayton@kernel.org>


--
Chuck Lever
  

On Wed, Aug 02, 2023 at 02:53:00PM -0400, Jeff Layton wrote:
> I noticed that xfstests generic/001 was failing against linux-next nfsd.
> 
> The client would request a OPEN4_SHARE_ACCESS_WRITE open, and the server
> would hand out a write delegation. The client would then try to use that
> write delegation as the source stateid in a COPY or CLONE operation, and
> the server would respond with NFS4ERR_STALE.
> 
> The problem is that the struct file associated with the delegation does
> not necessarily have read permissions. It's handing out a write
> delegation on what is effectively an O_WRONLY open. RFC 8881 states:
> 
>  "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
>   own, all opens."
> 
> Given that the client didn't request any read permissions, and that nfsd
> didn't check for any, it seems wrong to give out a write delegation.
> 
> Only hand out a write delegation if we have a O_RDWR descriptor
> available. If it fails to find an appropriate write descriptor, go
> ahead and try for a read delegation if NFS4_SHARE_ACCESS_READ was
> requested.
> 
> This fixes xfstest generic/001.
> 
> Closes: https://bugzilla.linux-nfs.org/show_bug.cgi?id=412
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Changes in v3:
> - add find_rw_file helper to ensure spinlock is taken appropriately
> - refine comments over conditionals
> - Link to v2: https://lore.kernel.org/r/20230801-wdeleg-v2-1-20c14252bab4@kernel.org
> 
> Changes in v2:
> - Rework the logic when finding struct file for the delegation. The
>   earlier patch might still have attached a O_WRONLY file to the deleg
>   in some cases, and could still have handed out a write delegation on
>   an O_WRONLY OPEN request in some cases.
> ---
>  fs/nfsd/nfs4state.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ef7118ebee00..c551784d108a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -649,6 +649,18 @@ find_readable_file(struct nfs4_file *f)
>  	return ret;
>  }
>  
> +static struct nfsd_file *
> +find_rw_file(struct nfs4_file *f)
> +{
> +	struct nfsd_file *ret;
> +
> +	spin_lock(&f->fi_lock);
> +	ret = nfsd_file_get(f->fi_fds[O_RDWR]);
> +	spin_unlock(&f->fi_lock);
> +
> +	return ret;
> +}
> +
>  struct nfsd_file *
>  find_any_file(struct nfs4_file *f)
>  {
> @@ -5449,7 +5461,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	struct nfs4_file *fp = stp->st_stid.sc_file;
>  	struct nfs4_clnt_odstate *odstate = stp->st_clnt_odstate;
>  	struct nfs4_delegation *dp;
> -	struct nfsd_file *nf;
> +	struct nfsd_file *nf = NULL;
>  	struct file_lock *fl;
>  	u32 dl_type;
>  
> @@ -5461,21 +5473,35 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>  	if (fp->fi_had_conflict)
>  		return ERR_PTR(-EAGAIN);
>  
> -	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> -		nf = find_writeable_file(fp);
> +	/*
> +	 * Try for a write delegation first. RFC8881 section 10.4 says:
> +	 *
> +	 *  "An OPEN_DELEGATE_WRITE delegation allows the client to handle,
> +	 *   on its own, all opens."
> +	 *
> +	 * Furthermore the client can use a write delegationf or most read
> +	 * operations as well, so we require a O_RDWR file here.
> +	 *
> +	 * Only a write delegation in the case of a BOTH open, and ensure
> +	 * we get the O_RDWR descriptor.
> +	 */
> +	if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
> +		nf = find_rw_file(fp);
>  		dl_type = NFS4_OPEN_DELEGATE_WRITE;
> -	} else {
> +	}
> +
> +	/*
> +	 * If the file is being opened O_RDONLY or we couldn't get a O_RDWR
> +	 * file for some reason, then try for a read deleg instead.
> +	 */
> +	if (!nf && (open->op_share_access & NFS4_SHARE_ACCESS_READ)) {
>  		nf = find_readable_file(fp);
>  		dl_type = NFS4_OPEN_DELEGATE_READ;
>  	}
> -	if (!nf) {
> -		/*
> -		 * We probably could attempt another open and get a read
> -		 * delegation, but for now, don't bother until the
> -		 * client actually sends us one.
> -		 */
> +
> +	if (!nf)
>  		return ERR_PTR(-EAGAIN);
> -	}
> +
>  	spin_lock(&state_lock);
>  	spin_lock(&fp->fi_lock);
>  	if (nfs4_delegation_exists(clp, fp))
> 
> ---
> base-commit: a734662572708cf062e974f659ae50c24fc1ad17
> change-id: 20230731-wdeleg-bbdb6b25a3c6
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Tested and applied to nfsd-next as a separate patch. I intend
to squash it into commit 68a593f24a35 ("NFSD: Enable write
delegation support") in a few days.