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

From: dai . ngo
Date: Wed Aug 02 2023 - 12:30:07 EST

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

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 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

This fixes xfstest generic/001.

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.
+ */
+ if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_BOTH) {
+ nf = nfsd_file_get(fp->fi_fds[O_RDWR]);
- } else {

Does this mean OPEN4_SHARE_ACCESS_WRITE do not get a write delegation?
It does not seem right.


+ }
+ /*
+ * 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);
- 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);
- }
if (nfs4_delegation_exists(clp, fp))

base-commit: a734662572708cf062e974f659ae50c24fc1ad17
change-id: 20230731-wdeleg-bbdb6b25a3c6

Best regards,