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. + */ + 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>
