Date:   Wed, 2 Aug 2023 20:50:51 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Neil Brown <neilb@...e.de>
CC:     Jeff Layton <jlayton@...nel.org>,
        Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <dai.ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] nfsd: don't hand out write delegations on O_WRONLY
 opens



> On Aug 2, 2023, at 4:48 PM, NeilBrown <neilb@...e.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@...nel.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@...nel.org>


--
Chuck Lever

Powered by blists - more mailing lists