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