Date:   Wed, 2 Aug 2023 18:25:32 +0000
From:   Chuck Lever III <chuck.lever@...cle.com>
To:     Jeff Layton <jlayton@...nel.org>
CC:     Dai Ngo <dai.ngo@...cle.com>, Neil Brown <neilb@...e.de>,
        Olga Kornievskaia <kolga@...app.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 v2] nfsd: don't hand out write delegations on O_WRONLY
 opens



> On Aug 2, 2023, at 2:15 PM, Jeff Layton <jlayton@...nel.org> wrote:
> 
> On Wed, 2023-08-02 at 09:29 -0700, dai.ngo@...cle.com wrote:
>> On 8/1/23 6:33 AM, 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
>> 
>> not sure why the client opens the source file of a COPY operation with
>> OPEN4_SHARE_ACCESS_WRITE?
>> 
> 
> It doesn't. The original open is to write the data for the file being
> copied. It then opens the file again for READ, but since it has a write
> delegation, it doesn't need to talk to the server at all -- it can just
> use that stateid for later operations.
> 
>>>  or CLONE operation, and
>>> the server would respond with NFS4ERR_STALE.
>> 
>> If the server does not allow client to use write delegation for the
>> READ, should the correct error return be NFS4ERR_OPENMODE?
>> 
> 
> The server must allow the client to use a write delegation for read
> operations. It's required by the spec, AFAIU.
> 
> The error in this case was just bogus. The vfs copy routine would return
> -EBADF since the file didn't have FMODE_READ, and the nfs server would
> translate that into NFS4ERR_STALE.
> 
> Probably there is a better v4 error code that we could translate EBADF
> to, but with this patch it shouldn't be a problem any longer. 
> 
>>> 
>>> 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 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 | 29 ++++++++++++++++++-----------
>>>  1 file changed, 18 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index ef7118ebee00..e79d82fd05e7 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -5449,7 +5449,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 +5461,28 @@ 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. We need an O_RDWR file
>>> +  * since a write delegation allows the client to perform any open
>>> +  * from its cache.

Since you're sending a v3... can you cite the section of RFC 8881
that specifies this "all open" case in this commment? I'm sure we're
all going to forget this requirement.


>>> +  */
>>> + if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
>>> + nf = nfsd_file_get(fp->fi_fds[O_RDWR]);
>>>   dl_type = NFS4_OPEN_DELEGATE_WRITE;
>>> - } else {
>> 
>> Does this mean OPEN4_SHARE_ACCESS_WRITE do not get a write delegation?
>> It does not seem right.
>> 
>> -Dai
>> 
> 
> Why? Per RFC 8881:
> 
> "An OPEN_DELEGATE_WRITE delegation allows the client to handle, on its
> own, all opens."
> 
> All opens. That includes read opens.
> 
> An OPEN4_SHARE_ACCESS_WRITE open will succeed on a file to which the
> user has no read permissions. Therefore, we can't grant a write
> delegation since can't guarantee that the user is allowed to do that.
> 
> 
>>> + }
>>> +
>>> + /*
>>> +  * 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