> On Aug 2, 2023, at 4:48 PM, NeilBrown <neilb@xxxxxxx> 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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx >> >> 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@xxxxxxxxxx> -- Chuck Lever
