Re: [PATCH RFC] NFSD: Fix possible sleep during nfsd4_release_lockowner()

From: Jeff Layton <jlayton@kernel.org>
To: Chuck Lever <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH RFC] NFSD: Fix possible sleep during nfsd4_release_lockowner()
Date: Mon, 23 May 2022 09:40:48 -0400	[thread overview]
Message-ID: <fe3f9ece807e1433631ee3e0bd6b78238305cb87.camel@kernel.org> (raw)
In-Reply-To: <165323344948.2381.7808135229977810927.stgit@bazille.1015granger.net>

On Sun, 2022-05-22 at 11:38 -0400, Chuck Lever wrote:
> nfsd4_release_lockowner() holds clp->cl_lock when it calls
> check_for_locks(). However, check_for_locks() calls nfsd_file_get()
> / nfsd_file_put() to access the backing inode's flc_posix list, and
> nfsd_file_put() can sleep if the inode was recently removed.
> 

It might be good to add a might_sleep() to nfsd_file_put?

> Let's instead rely on the stateowner's reference count to gate
> whether the release is permitted. This should be a reliable
> indication of locks-in-use since file lock operations and
> ->lm_get_owner take appropriate references, which are released
> appropriately when file locks are removed.
> 
> Reported-by: Dai Ngo <dai.ngo@oracle.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/nfsd/nfs4state.c |    9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> This might be a naive approach, but let's start with it.
> 
> This passes light testing, but it's not clear how much our existing
> fleet of tests exercises this area. I've locally built a couple of
> pynfs tests (one is based on the one Dai posted last week) and they
> pass too.
> 
> I don't believe that FREE_STATEID needs the same simplification.
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a280256cbb03..b77894e668a4 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7559,12 +7559,9 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>  
>  		/* see if there are still any locks associated with it */
>  		lo = lockowner(sop);
> -		list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
> -			if (check_for_locks(stp->st_stid.sc_file, lo)) {
> -				status = nfserr_locks_held;
> -				spin_unlock(&clp->cl_lock);
> -				return status;
> -			}
> +		if (atomic_read(&sop->so_count) > 1) {
> +			spin_unlock(&clp->cl_lock);
> +			return nfserr_locks_held;
>  		}
>  
>  		nfs4_get_stateowner(sop);
> 
> 

lm_get_owner is called from locks_copy_conflock, so if someone else
happens to be doing a LOCKT or F_GETLK call at the same time that
RELEASE_LOCKOWNER gets called, then this may end up returning an error
inappropriately. My guess is that that would be pretty hard to hit the
timing right, but not impossible.

What we may want to do is have the kernel do this check and only if it
comes back >1 do the actual check for locks. That won't fix the original
problem though.

In other places in nfsd, we've plumbed in a dispose_list head and
deferred the sleeping functions until the spinlock can be dropped. I
haven't looked closely at whether that's possible here, but it may be a
more reliable approach.
-- 
Jeff Layton <jlayton@kernel.org>