Received-SPF: pass (google.com: domain of linux-nfs+bounces-1798-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Chuck Lever" <chuck.lever@oracle.com>
Cc: "Jeff Layton" <jlayton@kernel.org>, "Dai Ngo" <Dai.Ngo@oracle.com>,
 "Olga Kornievskaia" <kolga@netapp.com>, "Tom Talpey" <tom@talpey.com>,
 linux-nfs@vger.kernel.org
Subject: Re: [PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()
In-reply-to: <ZcD9tiK5FDcOH0P+@tissot.1015granger.net>
References: <170709975922.13976.3341850918979137018@noble.neil.brown.name>,
 <ZcD9tiK5FDcOH0P+@tissot.1015granger.net>
Date: Tue, 06 Feb 2024 06:50:17 +1100
Message-id: <170716261771.13976.14660106666976655215@noble.neil.brown.name>

On Tue, 06 Feb 2024, Chuck Lever wrote:
> On Mon, Feb 05, 2024 at 01:22:39PM +1100, NeilBrown wrote:
> >=20
> > 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.
> >=20
> > ->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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > 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.
> >=20
> > Fixes: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER")
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >=20
> > 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 =3D 0;
> > =20
> > -	spin_lock(&fp->fi_lock);
> >  	fp->fi_had_conflict =3D true;
> >  	nfsd_break_one_deleg(dp);
> > -	spin_unlock(&fp->fi_lock);
> >  	return false;
> >  }
> > =20
> > @@ -5771,13 +5769,14 @@ nfs4_set_delegation(struct nfsd4_open *open, stru=
ct nfs4_ol_stateid *stp,
> >  	if (status)
> >  		goto out_unlock;
> > =20
> > +	status =3D -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 =3D -EAGAIN;
> > -	else
> > -		status =3D hash_delegation_locked(dp, fp);
> > +	status =3D hash_delegation_locked(dp, fp);
> >  	spin_unlock(&fp->fi_lock);
> >  	spin_unlock(&clp->cl_lock);
> >  	spin_unlock(&state_lock);
> > --=20
> > 2.43.0
>=20
> Thanks for jumping on this issue.
>=20
> 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().
>=20
> 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=20

    nfsd: hold ->cl_lock for hash_delegation_locked()

I guess that could go to -stable, but maybe not needed.

NeilBrown