| Message ID | 20240724170138.1942307-1-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [rfc,1/2] nfsd: don't assume copy notify when preprocessing the stateid | expand |
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. > > 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); > + status = check_special_stateids(net, fhp, stateid, flags); > goto done; > } > > -- > 2.43.0 > >
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. > > > > 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. > > + status = check_special_stateids(net, fhp, stateid, flags); > > goto done; > > } > > > > -- > > 2.43.0 > > > > > > -- > Chuck Lever
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. 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...
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.
On Wed, 2024-07-24 at 10:01 -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. > > 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); > + status = check_special_stateids(net, fhp, stateid, > flags); > goto done; > } > Nice cleanup. Reviewed-by: Jeff Layton <jlayton@kernel.org>
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?
On Fri, Jul 26, 2024 at 9:47 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > 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? Of course. > > -- > Chuck Lever
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); + status = check_special_stateids(net, fhp, stateid, flags); goto done; }
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. 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(-)