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

["Frank Filz" <ffilzlnx@mindspring.com>]

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