Re: [PATCH rfc 1/2] nfsd: don't assume copy notify when preprocessing the stateid

[Chuck Lever <chuck.lever@oracle.com>]

On Thu, Jul 25, 2024 at 09:51:11AM -0400, Olga Kornievskaia wrote:
> On Wed, Jul 24, 2024 at 5:43 PM Sagi Grimberg <sagi@grimberg.me> wrote:
> >
> >
> >
> >
> > On 24/07/2024 22:12, Olga Kornievskaia wrote:
> > > On Wed, Jul 24, 2024 at 1:06 PM Chuck Lever <chuck.lever@oracle.com> wrote:
> > >> Adding Olga for her review and comments.
> > >>
> > >> On Wed, Jul 24, 2024 at 10:01:37AM -0700, Sagi Grimberg wrote:
> > >>> Move the stateid handling to nfsd4_copy_notify, if nfs4_preprocess_stateid_op
> > >>> did not produce an output stateid, error out.
> > >>>
> > >>> Copy notify specifically does not permit the use of special stateids,
> > >>> so enforce that outside generic stateid pre-processing.
> > > I dont see how this patch is accomplishing this. As far as I can tell
> > > (just by looking at the code without actually testing it) instead it
> > > does exactly the opposite.
> >
> > I haven't tested this either, does pynfs have a test for it?
> >
> > >
> > >>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> > >>> ---
> > >>>   fs/nfsd/nfs4proc.c  | 4 +++-
> > >>>   fs/nfsd/nfs4state.c | 6 +-----
> > >>>   2 files changed, 4 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > >>> index 46bd20fe5c0f..7b70309ad8fb 100644
> > >>> --- a/fs/nfsd/nfs4proc.c
> > >>> +++ b/fs/nfsd/nfs4proc.c
> > >>> @@ -1942,7 +1942,7 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >>>        struct nfsd4_copy_notify *cn = &u->copy_notify;
> > >>>        __be32 status;
> > >>>        struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > >>> -     struct nfs4_stid *stid;
> > >>> +     struct nfs4_stid *stid = NULL;
> > >>>        struct nfs4_cpntf_state *cps;
> > >>>        struct nfs4_client *clp = cstate->clp;
> > >>>
> > >>> @@ -1951,6 +1951,8 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >>>                                        &stid);
> > >>>        if (status)
> > >>>                return status;
> > >>> +     if (!stid)
> > >>> +             return nfserr_bad_stateid;
> > >>>
> > >>>        cn->cpn_lease_time.tv_sec = nn->nfsd4_lease;
> > >>>        cn->cpn_lease_time.tv_nsec = 0;
> > >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > >>> index 69d576b19eb6..dc61a8adfcd4 100644
> > >>> --- a/fs/nfsd/nfs4state.c
> > >>> +++ b/fs/nfsd/nfs4state.c
> > >>> @@ -7020,11 +7020,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
> > >>>                *nfp = NULL;
> > >>>
> > >>>        if (ZERO_STATEID(stateid) || ONE_STATEID(stateid)) {
> > >>> -             if (cstid)
> > >>> -                     status = nfserr_bad_stateid;
> > >>> -             else
> > >>> -                     status = check_special_stateids(net, fhp, stateid,
> > >>> -                                                                     flags);
> > > This code was put in by Bruce in commit ("nfsd: fix crash on
> > > COPY_NOTIFY with special stateid"). Its intentions were to make sure
> > > that IF COPY_NOTFY was sent with a special state it, then the server
> > > would produce an error (in this case bad_stateid). Only from
> > > copy_notify do we call nfs4_preproces_stateid_op() with a non-null
> > > cstid. This above logic is needed here as far as I can tell.
> >
> > So the purpose was now special stateids will not generate an output
> > stateid, which COPY_NOTIFY now checks, and fails locally in this case.
> 
> Ok I see it now I do agree with the nfsd4_copy_notify() changes it
> should be still guard against the use of special stateid.
> 
> > Maybe I'm missing something, but my assumption is that if a client
> > sends a COPY_NOTIFY with a special stateid, it will still error out with
> > bad stateid (due to the change in nfsd4_copy_notify...\
> 
> I agree now. Thank you for the explanation. Looks good.

Hi Olga, may I add a Reviewed-by: from you?

-- 
Chuck Lever