On Wed, Jul 24, 2024 at 1:06 PM Chuck Lever <chuck.lever@xxxxxxxxxx> 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.
> >
> > Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> > ---
> > 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.
> > + status = check_special_stateids(net, fhp, stateid, flags);
> > goto done;
> > }
> >
> > --
> > 2.43.0
> >
> >
>
> --
> Chuck Lever
