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

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

[Dai Ngo]

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

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

[Chuck Lever]

On 6/10/25 9:41 AM, 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;

How does the client react to NFS4ERR_BAD_STATEID ? Does it retry the
same request or does it reflect the failure to the application?


> +			goto out;
> +		}
>  		stp = nfsd4_find_and_lock_existing_open(fp, open);
>  	} else {
>  		open->op_file = NULL;


-- 
Chuck Lever

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

[Jeff Layton]

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 ?

In any case, whatever we decide:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

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

[Chuck Lever]

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.


> 
> In any case, whatever we decide:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>


-- 
Chuck Lever

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

[Jeff Layton]

On Tue, 2025-06-10 at 09:50 -0400, Chuck Lever wrote:
> On 6/10/25 9:41 AM, 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;
> 
> How does the client react to NFS4ERR_BAD_STATEID ? Does it retry the
> same request or does it reflect the failure to the application?
> 

I imagine this triggers the client to go into state recovery, which
won't be terribly helpful. Another reason to consider a different error
code. I suggest:

    15.1.1.4. NFS4ERR_INVAL (Error Code 22)

    The arguments for this operation are not valid for some reason, even though they do match those specified in the XDR definition for the request.

This might bubble up as an application error and get noticed that way.

> 
> > +			goto out;
> > +		}
> >  		stp = nfsd4_find_and_lock_existing_open(fp, open);
> >  	} else {
> >  		open->op_file = NULL;
> 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Jeff Layton]

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.

> 
> > 
> > In any case, whatever we decide:
> > 
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Chuck Lever]

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


>>> In any case, whatever we decide:
>>>
>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>
> 


-- 
Chuck Lever

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

[Dai Ngo]

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

-Dai

>
>
>>>> In any case, whatever we decide:
>>>>
>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>

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

[Chuck Lever]

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>


>>>>> In any case, whatever we decide:
>>>>>
>>>>> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>>


-- 
Chuck Lever

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

[Frank Filz]

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.

Thanks

Frank

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

[Dai Ngo]

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:

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
>

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

[Calum Mackay]

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?

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

-- 
Calum Mackay
Linux Kernel Engineering
Oracle Linux and Virtualisation

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

[Dai Ngo]

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