Messages in this thread | | | Date | Wed, 2 Aug 2023 14:32:59 -0700 | Subject | Re: [PATCH v2] nfsd: don't hand out write delegations on O_WRONLY opens | From | dai.ngo@oracle ... |
| |
On 8/2/23 2:22 PM, dai.ngo@oracle.com wrote: > > On 8/2/23 1:57 PM, Chuck Lever III wrote: >> >>> On Aug 2, 2023, at 4:48 PM, Jeff Layton <jlayton@kernel.org> wrote: >>> >>> On Wed, 2023-08-02 at 13:15 -0700, dai.ngo@oracle.com wrote: >>>> On 8/2/23 11:15 AM, Jeff Layton 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. >>>>>>> + */ >>>>>>> + 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 server grants the write delegation on an OPEN with >>>> OPEN4_SHARE_ACCESS_WRITE on the file with WR-only access mode then >>>> why can't the server checks and denies the subsequent READ? >>>> >>>> Per RFC 8881, section 9.1.2: >>>> >>>> For delegation stateids, the access mode is based on the type of >>>> delegation. >>>> >>>> When a READ, WRITE, or SETATTR (that specifies the size >>>> attribute) >>>> operation is done, the operation is subject to checking >>>> against the >>>> access mode to verify that the operation is appropriate given the >>>> stateid with which the operation is associated. >>>> >>>> In the case of WRITE-type operations (i.e., WRITEs and >>>> SETATTRs that >>>> set size), the server MUST verify that the access mode allows >>>> writing >>>> and MUST return an NFS4ERR_OPENMODE error if it does not. In >>>> the case >>>> of READ, the server may perform the corresponding check on the >>>> access >>>> mode, or it may choose to allow READ on OPENs for >>>> OPEN4_SHARE_ACCESS_WRITE, >>>> to accommodate clients whose WRITE implementation may >>>> unavoidably do >>>> reads (e.g., due to buffer cache constraints). However, even >>>> if READs >>>> are allowed in these circumstances, the server MUST still >>>> check for >>>> locks that conflict with the READ (e.g., another OPEN specified >>>> OPEN4_SHARE_DENY_READ or OPEN4_SHARE_DENY_BOTH). Note that a >>>> server >>>> that does enforce the access mode check on READs need not >>>> explicitly >>>> check for conflicting share reservations since the existence >>>> of OPEN >>>> for OPEN4_SHARE_ACCESS_READ guarantees that no conflicting share >>>> reservation can exist. >>>> >>>> FWIW, The Solaris server grants write delegation on OPEN with >>>> OPEN4_SHARE_ACCESS_WRITE on file with access mode either RW or >>>> WR-only. Maybe this is a bug? or the spec is not clear? >>>> >>> I don't think that's necessarily a bug. >>> >>> It's not that the spec demands that we only hand out delegations on >>> BOTH >>> opens. This is more of a quirk of the Linux implementation. Linux' >>> write delegations require an open O_RDWR file descriptor because we may >>> be called upon to do a read on its behalf. >>> >>> Technically, we could probably just have it check for >>> OPEN4_SHARE_ACCESS_WRITE, but in the case where READ isn't also set, >>> then you're unlikely to get a delegation. Either the O_RDWR descriptor >>> will be NULL, or there are other, conflicting opens already present. >>> >>> Solaris may have a completely different design that doesn't require >>> this. I haven't looked at its code to know. >> I'm comfortable for now with not handing out write delegations for >> SHARE_ACCESS_WRITE opens. I prefer that to permission checking on >> every READ operation. > > I'm fine with just handling out write delegation for SHARE_ACCESS_BOTH > only. > > Just a concern about not checking for access at the time of READ > operation. or not checking file permission at the time WRITE. > If the file was opened with SHARE_ACCESS_WRITE (no write delegation > granted) > and the file access mode was changed to read-only, is it a correct > behavior > for the server to allow the READ to go through? I meant for the WRITE to go through. > > -Dai > >> >> If we find that it's a significant performance issue, we can revisit. >> >> >>>> It'd would be interesting to know how ONTAP server behaves in >>>> this scenario. >>>> >>> Indeed. Most likely it behaves more like Solaris does, but it'd nice to >>> know. >>> >>>>> >>>>>>> + } >>>>>>> + >>>>>>> + /* >>>>>>> + * 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 >> >>
| |