Re: [PATCH v2] nfsd: don't hand out write delegations on O_WRONLY opens
From: dai . ngo
Date: Wed Aug 02 2023 - 17:34:07 EST
On 8/2/23 2:22 PM, dai.ngo@xxxxxxxxxx wrote:
On 8/2/23 1:57 PM, Chuck Lever III wrote:
On Aug 2, 2023, at 4:48 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
On Wed, 2023-08-02 at 13:15 -0700, dai.ngo@xxxxxxxxxx wrote:
On 8/2/23 11:15 AM, Jeff Layton 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.
+ */
+ 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@xxxxxxxxxx>
--
Chuck Lever