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@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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>
>>>>> In any case, whatever we decide:
>>>>>
>>>>> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>
--
Chuck Lever
