Re: [PATCH 1/1] NFSD: detect mismatch of file handle and delegation stateid in OPEN op
From: Dai Ngo <dai.ngo@oracle.com>
Date: 2025-06-10 15:05:32
On 6/10/25 7:53 AM, Frank Filz wrote:
From: Chuck Lever [mailto:chuck.lever@oracle.com]quoted
On 6/10/25 10:12 AM, Dai Ngo wrote:quoted
On 6/10/25 7:01 AM, Chuck Lever wrote:quoted
On 6/10/25 9:59 AM, Jeff Layton wrote:quoted
On Tue, 2025-06-10 at 09:52 -0400, Chuck Lever wrote:quoted
On 6/10/25 9:50 AM, Jeff Layton wrote:quoted
On Tue, 2025-06-10 at 06:41 -0700, Dai Ngo wrote:quoted
When the client sends an OPEN with claim type CLAIM_DELEG_CUR_FH or CLAIM_DELEGATION_CUR, the delegation stateid and the file handle must belongs to the same file, otherwise returnNFS4ERR_BAD_STATEID.quoted
quoted
quoted
quoted
quoted
quoted
Signed-off-by: Dai Ngo <dai.ngo@oracle.com> --- fs/nfsd/nfs4state.c | 5 +++++ 1 file changed, 5 insertions(+)diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index59a693f22452..be2ee641a22d 100644--- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c@@ -6318,6 +6318,11 @@ nfsd4_process_open2(struct svc_rqst*rqstp, struct svc_fh *current_fh, struct nf status = nfs4_check_deleg(cl, open, &dp); if (status) goto out; + if (dp && nfsd4_is_deleg_cur(open) && + (dp->dl_stid.sc_file != fp)) { + status = nfserr_bad_stateid; + goto out; + } stp = nfsd4_find_and_lock_existing_open(fp, open); } else { open->op_file = NULL;This seems like a good idea. I wonder if BAD_STATEID is the right error here. It is a valid stateid, after all, it just doesn't match the current_fh. Maybe this should be nfserr_inval ?I agree, NFS4ERR_BAD_STATEID /might/ cause a loop, so that needs to be tested. BAD_STATEID is mandated by the spec, so if we choose to return a different status code here, it needs a comment explaining why.Oh, I didn't realize that error was mandated, but you're right. RFC8881, section 8.2.4: - If the selected table entry does not match the current filehandle, return NFS4ERR_BAD_STATEID. I guess we're stuck with reporting that unless we want to amend the spec.It is spec-mandated behavior, but we are always free to ignore the spec. I'm OK with NFS4ERR_INVAL if it results in better behavior (as long as there is a comment explaining why we deviate from the mandate).Since the Linux client does not behave this way I can not test if this error get us into a loop.Good point!quoted
I used pynfs to force this behavior. However, here is the comment in nfs4_do_open: /* * BAD_STATEID on OPEN means that the server cancelled our * state before it received the OPEN_CONFIRM. * Recover by retrying the request as per the discussion * on Page 181 of RFC3530. */ So it guess BAD_STATEID will get the client and server into a loop. I'll change error to NFS4ERR_INVAL and add a comment in the code.Thanks, we'll start there. If that's problematic, it can always be changed later. Maybe someone should file an errata against RFC 8881. <whistles tunelessly>An interesting case. Ganesha doesn't handle this. It would definitely be good to see an errata for it. Also a pynfs test case.
Here is the pynfs test I used to test CLAIM_DELEG_CUR_FH:
def testClaimDeleg_CurFh(t, env):
"""Test OPEN with CLAIM_DELEG_CUR_FH with mismatch file file handle
and delegation stateid.
FLAGS: writedelegations deleg
CODE: DELEG32
"""
sess = env.c1.new_client_session(env.testname(t))
# create file-1 with read-only access (0444) no delegatation wanted,
# and leave the file opened
filename1 = b"file-1"
res = open_create_file(sess, filename1, open_create = OPEN4_CREATE, attrs={FATTR4_MODE: 0o444},
access = OPEN4_SHARE_ACCESS_BOTH, want_deleg = False)
check(res)
deleg = res.resarray[-2].delegation
if (_got_deleg(deleg)):
fail("Not expect to get delegation")
fh = res.resarray[-1].object
stateid = res.resarray[-2].stateid
print("----- CREATED ", filename1)
# create file-2 with access RW and delegation wanted
filename2 = b"file-2"
res = open_create_file(sess, filename2, open_create = OPEN4_CREATE,
access = OPEN4_SHARE_ACCESS_BOTH, want_deleg = True)
check(res)
print("----- CREATED ", filename2)
wfh = res.resarray[-1].object
wdeleg = res.resarray[-2].delegation
if (not _got_deleg(wdeleg)):
fail("Could not get WRITE delegation")
wdelegstateid = wdeleg.write.stateid
# OPEN for WRITE with CLAIM_DELEG_CUR_FH using the file handle
# of filename1 and the delegation stateid granted for filename2.
# Since the file handle and the delegation stateid do not belong
# to the same file, expect server to return NFS4ERR_BAD_STATEID.
claim = open_claim4(CLAIM_DELEG_CUR_FH, oc_delegate_stateid=wdelegstateid)
owner = open_owner4(0, b"My Open Owner 2")
how = openflag4(OPEN4_NOCREATE)
open_op = op.open(0, OPEN4_SHARE_ACCESS_WRITE, OPEN4_SHARE_DENY_NONE,
owner, how, claim)
res = sess.compound([op.putfh(fh), open_op])
check(res, NFS4ERR_BAD_STATEID)
# close file-1
res = close_file(sess, fh, stateid)
check(res)
# return the write delegation
res = sess.compound([op.putfh(wfh), op.delegreturn(wdelegstateid)])
check(res)
Thanks Frank