Re: [PATCH 1/1] NFSD: detect mismatch of file handle and delegation stateid in OPEN op

[Dai Ngo <dai.ngo@oracle.com>]

On 6/10/25 3:03 PM, Calum Mackay wrote:
> On 10/06/2025 4:05 pm, Dai Ngo wrote:
>>
>> On 6/10/25 7:53 AM, Frank Filz wrote:
>>> From: Chuck Lever [mailto:chuck.lever@oracle.com]
>>>> On 6/10/25 10:12 AM, Dai Ngo wrote:
>>>>> On 6/10/25 7:01 AM, Chuck Lever wrote:
>>>>>> On 6/10/25 9:59 AM, Jeff Layton wrote:
>>>>>>> On Tue, 2025-06-10 at 09:52 -0400, Chuck Lever wrote:
>>>>>>>> On 6/10/25 9:50 AM, Jeff Layton wrote:
>>>>>>>>> On Tue, 2025-06-10 at 06:41 -0700, Dai Ngo wrote:
>>>>>>>>>> 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 return
>>>> NFS4ERR_BAD_STATEID.
>>>>>>>>>> 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 index
>>>>>>>>>> 59a693f22452..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!
>>>>
>>>>
>>>>> 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:
>
> Thanks Dai; would you like to submit that as a pynfs patch, please?

Yes, I will change the expected error code to NFS4ERR_INVAL, as we 
discussed, and submit the patch.

-Dai

>
> ta,
> c.
>
>
>>
>> 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
>>>
>>
>