[PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()

[PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()

[NeilBrown]

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@suse.de>
---
 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

Re: [PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()

[Jeff Layton]

On Mon, 2024-02-05 at 13:22 +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@suse.de>
> ---
>  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);

Love the detailed explanation:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()

[Chuck Lever]

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@suse.de>
> ---
>  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.


-- 
Chuck Lever

Re: [PATCH] nfsd: don't take fi_lock in nfsd_break_deleg_cb()

[NeilBrown]

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@suse.de>
> > ---
> >  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