Re: [PATCH v2] nfsd: don't hand out write delegations on O_WRONLY opens

Chuck Lever III
2023-08-02T18:25:55Z

> On Aug 2, 2023, at 2:15 PM, Jeff Layton <jlayton@kernel.org> wrote:
>
> On Wed, 2023-08-02 at 09:29 -0700, dai.ngo@oracle.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@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 | 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@kernel.org>

--
Chuck Lever