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@xxxxxxxxxx>
> > ---
> > 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
> >
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
