| Message ID | 20240823-nfsd-fixes-v1-1-fc99aa16f6a0@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: CB_GETATTR fixes | expand |
On Fri, Aug 23, 2024 at 06:27:38PM -0400, Jeff Layton wrote: > Once we've dropped the flc_lock, there is nothing that ensures that the > delegation that was found will still be around later. Take a reference > to it while holding the lock and then drop it when we've finished with > the delegation. > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index dafff707e23a..19d39872be32 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); > struct file_lock_context *ctx; > struct file_lease *fl; > - struct nfs4_delegation *dp; > struct iattr attrs; > struct nfs4_cb_fattr *ncf; > > @@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > goto break_lease; > } > if (type == F_WRLCK) { > - dp = fl->c.flc_owner; > + struct nfs4_delegation *dp = fl->c.flc_owner; Setting @dp here seems redundant; just below, after the break_lease label it is set again to the same value. May I change this line to: struct nfs4_delegation *dp; > + > if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) { > spin_unlock(&ctx->flc_lock); > return 0; > @@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > break_lease: > nfsd_stats_wdeleg_getattr_inc(nn); > dp = fl->c.flc_owner; > + refcount_inc(&dp->dl_stid.sc_count); > ncf = &dp->dl_cb_fattr; > nfs4_cb_getattr(&dp->dl_cb_fattr); > spin_unlock(&ctx->flc_lock); > @@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > /* Recall delegation only if client didn't respond */ > status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ)); > if (status != nfserr_jukebox || > - !nfsd_wait_for_delegreturn(rqstp, inode)) > + !nfsd_wait_for_delegreturn(rqstp, inode)) { > + nfs4_put_stid(&dp->dl_stid); > return status; > + } > } > if (!ncf->ncf_file_modified && > (ncf->ncf_initial_cinfo != ncf->ncf_cb_change || > @@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > *size = ncf->ncf_cur_fsize; > *modified = true; > } > + nfs4_put_stid(&dp->dl_stid); > return 0; > } > break; > > -- > 2.46.0 >
On Sat, 2024-08-24 at 11:03 -0400, Chuck Lever wrote: > On Fri, Aug 23, 2024 at 06:27:38PM -0400, Jeff Layton wrote: > > Once we've dropped the flc_lock, there is nothing that ensures that the > > delegation that was found will still be around later. Take a reference > > to it while holding the lock and then drop it when we've finished with > > the delegation. > > > > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4state.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index dafff707e23a..19d39872be32 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); > > struct file_lock_context *ctx; > > struct file_lease *fl; > > - struct nfs4_delegation *dp; > > struct iattr attrs; > > struct nfs4_cb_fattr *ncf; > > > > @@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > goto break_lease; > > } > > if (type == F_WRLCK) { > > - dp = fl->c.flc_owner; > > + struct nfs4_delegation *dp = fl->c.flc_owner; > > Setting @dp here seems redundant; just below, after the break_lease > label it is set again to the same value. May I change this line to: > > struct nfs4_delegation *dp; > I don't think you can just remove that one since it's dereferenced just after that. The problem is the goto break_lease case needs to have that assigned too so you also can't just remove the later one. The way the code flows here is weird, unfortunately, but I don't see an easy way to improve it right offhand. Maybe assign "dp" just before the "goto break_lease" ? > > + > > if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) { > > spin_unlock(&ctx->flc_lock); > > return 0; > > @@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > break_lease: > > nfsd_stats_wdeleg_getattr_inc(nn); > > dp = fl->c.flc_owner; > > + refcount_inc(&dp->dl_stid.sc_count); > > ncf = &dp->dl_cb_fattr; > > nfs4_cb_getattr(&dp->dl_cb_fattr); > > spin_unlock(&ctx->flc_lock); > > @@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > /* Recall delegation only if client didn't respond */ > > status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ)); > > if (status != nfserr_jukebox || > > - !nfsd_wait_for_delegreturn(rqstp, inode)) > > + !nfsd_wait_for_delegreturn(rqstp, inode)) { > > + nfs4_put_stid(&dp->dl_stid); > > return status; > > + } > > } > > if (!ncf->ncf_file_modified && > > (ncf->ncf_initial_cinfo != ncf->ncf_cb_change || > > @@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, > > *size = ncf->ncf_cur_fsize; > > *modified = true; > > } > > + nfs4_put_stid(&dp->dl_stid); > > return 0; > > } > > break; > > > > -- > > 2.46.0 > > >
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index dafff707e23a..19d39872be32 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); struct file_lock_context *ctx; struct file_lease *fl; - struct nfs4_delegation *dp; struct iattr attrs; struct nfs4_cb_fattr *ncf; @@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, goto break_lease; } if (type == F_WRLCK) { - dp = fl->c.flc_owner; + struct nfs4_delegation *dp = fl->c.flc_owner; + if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) { spin_unlock(&ctx->flc_lock); return 0; @@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, break_lease: nfsd_stats_wdeleg_getattr_inc(nn); dp = fl->c.flc_owner; + refcount_inc(&dp->dl_stid.sc_count); ncf = &dp->dl_cb_fattr; nfs4_cb_getattr(&dp->dl_cb_fattr); spin_unlock(&ctx->flc_lock); @@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, /* Recall delegation only if client didn't respond */ status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ)); if (status != nfserr_jukebox || - !nfsd_wait_for_delegreturn(rqstp, inode)) + !nfsd_wait_for_delegreturn(rqstp, inode)) { + nfs4_put_stid(&dp->dl_stid); return status; + } } if (!ncf->ncf_file_modified && (ncf->ncf_initial_cinfo != ncf->ncf_cb_change || @@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode, *size = ncf->ncf_cur_fsize; *modified = true; } + nfs4_put_stid(&dp->dl_stid); return 0; } break;
Once we've dropped the flc_lock, there is nothing that ensures that the delegation that was found will still be around later. Take a reference to it while holding the lock and then drop it when we've finished with the delegation. Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation") Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)