Re: [PATCH AUTOSEL for 4.9 26/55] nfsd: Ensure we don't recognise lock stateids after freeing them

From: "J. Bruce Fields" <bfields@redhat.com>
To: Sasha Levin <Alexander.Levin@microsoft.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Trond Myklebust <trond.myklebust@primarydata.com>
Subject: Re: [PATCH AUTOSEL for 4.9 26/55] nfsd: Ensure we don't recognise lock stateids after freeing them
Date: Wed, 24 Jan 2018 10:47:48 -0500	[thread overview]
Message-ID: <20180124154748.GC31619@parsley.fieldses.org> (raw)
In-Reply-To: <20180124041521.32223-26-alexander.levin@microsoft.com>

As reported on other instance of this patch; maybe hold off on this for
now.

--b.

On Wed, Jan 24, 2018 at 04:15:42AM +0000, Sasha Levin wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> [ Upstream commit 659aefb68eca28ba9aa482a9fc64de107332e256 ]
> 
> In order to deal with lookup races, nfsd4_free_lock_stateid() needs
> to be able to signal to other stateful functions that the lock stateid
> is no longer valid. Right now, nfsd_lock() will check whether or not an
> existing stateid is still hashed, but only in the "new lock" path.
> 
> To ensure the stateid invalidation is also recognised by the "existing lock"
> path, and also by a second call to nfsd4_free_lock_stateid() itself, we can
> change the type to NFS4_CLOSED_STID under the stp->st_mutex.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
> ---
>  fs/nfsd/nfs4state.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 7f3a11ab4191..fed5902099f5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5098,7 +5098,9 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
>  	struct nfs4_ol_stateid *stp = openlockstateid(s);
>  	__be32 ret;
>  
> -	mutex_lock(&stp->st_mutex);
> +	ret = nfsd4_lock_ol_stateid(stp);
> +	if (ret)
> +		goto out_put_stid;
>  
>  	ret = check_stateid_generation(stateid, &s->sc_stateid, 1);
>  	if (ret)
> @@ -5109,11 +5111,13 @@ nfsd4_free_lock_stateid(stateid_t *stateid, struct nfs4_stid *s)
>  			    lockowner(stp->st_stateowner)))
>  		goto out;
>  
> +	stp->st_stid.sc_type = NFS4_CLOSED_STID;
>  	release_lock_stateid(stp);
>  	ret = nfs_ok;
>  
>  out:
>  	mutex_unlock(&stp->st_mutex);
> +out_put_stid:
>  	nfs4_put_stid(s);
>  	return ret;
>  }
> @@ -5678,6 +5682,8 @@ find_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp)
>  	lockdep_assert_held(&clp->cl_lock);
>  
>  	list_for_each_entry(lst, &lo->lo_owner.so_stateids, st_perstateowner) {
> +		if (lst->st_stid.sc_type != NFS4_LOCK_STID)
> +			continue;
>  		if (lst->st_stid.sc_file == fp) {
>  			atomic_inc(&lst->st_stid.sc_count);
>  			return lst;
> @@ -5752,7 +5758,6 @@ lookup_or_create_lock_state(struct nfsd4_compound_state *cstate,
>  	struct nfs4_lockowner *lo;
>  	struct nfs4_ol_stateid *lst;
>  	unsigned int strhashval;
> -	bool hashed;
>  
>  	lo = find_lockowner_str(cl, &lock->lk_new_owner);
>  	if (!lo) {
> @@ -5775,15 +5780,7 @@ retry:
>  		goto out;
>  	}
>  
> -	mutex_lock(&lst->st_mutex);
> -
> -	/* See if it's still hashed to avoid race with FREE_STATEID */
> -	spin_lock(&cl->cl_lock);
> -	hashed = !list_empty(&lst->st_perfile);
> -	spin_unlock(&cl->cl_lock);
> -
> -	if (!hashed) {
> -		mutex_unlock(&lst->st_mutex);
> +	if (nfsd4_lock_ol_stateid(lst) != nfs_ok) {
>  		nfs4_put_stid(&lst->st_stid);
>  		goto retry;
>  	}
> -- 
> 2.11.0