On Tue, 06 Feb 2024, Chuck Lever wrote:
> On Mon, Feb 05, 2024 at 01:22:39PM +1100, NeilBrown wrote:
> >
> > A recent change to check_for_locks() changed it to take ->flc_lock while
> > holding ->fi_lock. This creates a lock inversion (reported by lockdep)
> > because there is a case where ->fi_lock is taken while holding
> > ->flc_lock.
> >
> > ->flc_lock is held across ->fl_lmops callbacks, and
> > nfsd_break_deleg_cb() is one of those and does take ->fi_lock. However
> > it doesn't need to.
> >
> > Prior to v4.17-rc1~110^2~22 ("nfsd: create a separate lease for each
> > delegation") nfsd_break_deleg_cb() would walk the ->fi_delegations list
> > and so needed the lock. Since then it doesn't walk the list and doesn't
> > need the lock.
> >
> > Two actions are performed under the lock. One is to call
> > nfsd_break_one_deleg which calls nfsd4_run_cb(). These doesn't act on
> > the nfs4_file at all, so don't need the lock.
> >
> > The other is to set ->fi_had_conflict which is in the nfs4_file.
> > This field is only ever set here (except when initialised to false)
> > so there is no possible problem will multiple threads racing when
> > setting it.
> >
> > The field is tested twice in nfs4_set_delegation(). The first test does
> > not hold a lock and is documented as an opportunistic optimisation, so
> > it doesn't impose any need to hold ->fi_lock while setting
> > ->fi_had_conflict.
> >
> > The second test in nfs4_set_delegation() *is* make under ->fi_lock, so
> > removing the locking when ->fi_had_conflict is set could make a change.
> > The change could only be interesting if ->fi_had_conflict tested as
> > false even though nfsd_break_one_deleg() ran before ->fi_lock was
> > unlocked. i.e. while hash_delegation_locked() was running.
> > As hash_delegation_lock() doesn't interact in any way with nfs4_run_cb()
> > there can be no importance to this interaction.
> >
> > So this patch removes the locking from nfsd_break_one_deleg() and moves
> > the final test on ->fi_had_conflict out of the locked region to make it
> > clear that locking isn't important to the test. It is still tested
> > *after* vfs_setlease() has succeeded. This might be significant and as
> > vfs_setlease() takes ->flc_lock, and nfsd_break_one_deleg() is called
> > under ->flc_lock this "after" is a true ordering provided by a spinlock.
> >
> > Fixes: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER")
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> > fs/nfsd/nfs4state.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 12534e12dbb3..8b112673d389 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5154,10 +5154,8 @@ nfsd_break_deleg_cb(struct file_lock *fl)
> > */
> > fl->fl_break_time = 0;
> >
> > - spin_lock(&fp->fi_lock);
> > fp->fi_had_conflict = true;
> > nfsd_break_one_deleg(dp);
> > - spin_unlock(&fp->fi_lock);
> > return false;
> > }
> >
> > @@ -5771,13 +5769,14 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > if (status)
> > goto out_unlock;
> >
> > + status = -EAGAIN;
> > + if (fp->fi_had_conflict)
> > + goto out_unlock;
> > +
> > spin_lock(&state_lock);
> > spin_lock(&clp->cl_lock);
> > spin_lock(&fp->fi_lock);
> > - if (fp->fi_had_conflict)
> > - status = -EAGAIN;
> > - else
> > - status = hash_delegation_locked(dp, fp);
> > + status = hash_delegation_locked(dp, fp);
> > spin_unlock(&fp->fi_lock);
> > spin_unlock(&clp->cl_lock);
> > spin_unlock(&state_lock);
> > --
> > 2.43.0
>
> Thanks for jumping on this issue.
>
> This version of the fix does not apply to nfsd-fixes since the
> ADMIN_REVOKED changes in nfsd-next also touch this part of
> nfs4_set_delegation().
>
> Because edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER") is now applied
> in v6.8-rc3, v6.7.y, v6.6.y, and probably v6.1.y, I've reworked this
> fix slightly to apply on nfsd-fixes and have started a round of
> testing there.
Thanks.
I see the conflict comes from the addition of ->cl_lock in
nfsd: hold ->cl_lock for hash_delegation_locked()
I guess that could go to -stable, but maybe not needed.
NeilBrown
