From patchwork Thu Feb 1 19:06:41 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chuck Lever X-Patchwork-Id: 13541671 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3535984FC7; Thu, 1 Feb 2024 19:06:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706814403; cv=none; b=vB8YGGJ00z+rs1xb5czf24GBD9NIiFBs5JHKHfqPesD8Hg4R+VeVAojViigfwf18GwQ9bFORm8cNkc4r2vxpwrvh3DSLOb8B91/m75JvfSiqkdYoZgFkNl2A6LN1a5r4kYm1QIfrO9D2ZBjJjj4jCTS6R2LOw2Sis2yPU0Sx2Sg= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706814403; c=relaxed/simple; bh=dCK+VAU2AQKDgMEEoSter8OFWkWUAXH351Qawc8AMYw=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=bUTCHzz5+yIl+UfEg9+YQeM67zx4W0WVOBuSZHi99GPKkYR3JLSywxG5/+MBiOgzvIM+PCoe64xJrj0H2iCNsgm8qIFi7etnDbOEx3SEE7p6K5Iu8oZRnYbmOSVTeUPO3u1ILlaFhiOlJ13TedZj33MmCysY5J05Rb/9FpJe1VU= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=HTT7Pnr2; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="HTT7Pnr2" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6B47FC433C7; Thu, 1 Feb 2024 19:06:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1706814402; bh=dCK+VAU2AQKDgMEEoSter8OFWkWUAXH351Qawc8AMYw=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=HTT7Pnr2hkN6QJyCxo/o24GbDxy+G8Ratkh4lip4ZWB3Dz+mpqjfCHCn8df5q5vTX 9vfZMFPuTgNqDMqCqi4MQ5lX54KEcu8hYcBjfC/GVB2WvvwpmzFQCrK4jT+/WsQMGY onUBO1sNmMbGMqpBaLO6ypSOO2MrSeZA7Hu70E5ivFCCm9vDADdkfsQo9UFhvAA/kb NasspqLtT+d80PwXfpGiCz4aUssr2jTigLJuQ8qMrI5Dh1ksryxW5xS7NrpYAz45oR Vt2xl1eEeyKvAf6yPsz2cOinnZ37+V/ker3bWQSaHDRXEGOzQSzW2CSnaUHnI1R3oz /FXHhR6cW0vxw== Subject: [PATCH 1/3] NFSD: Modernize nfsd4_release_lockowner() From: Chuck Lever To: stable@vger.kernel.org Cc: linux-nfs@vger.kernel.org Date: Thu, 01 Feb 2024 14:06:41 -0500 Message-ID: <170681440143.15250.6029809473642562886.stgit@klimt.1015granger.net> In-Reply-To: <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net> References: <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net> User-Agent: StGit/1.5 Precedence: bulk X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: Chuck Lever [ Upstream commit bd8fdb6e545f950f4654a9a10d7e819ad48146e5 ] Refactor: Use existing helpers that other lock operations use. This change removes several automatic variables, so re-organize the variable declarations for readability. Stable-dep-of: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER") Signed-off-by: Chuck Lever --- fs/nfsd/nfs4state.c | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index a0aa7e63739d..9a77a3eac4ac 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6873,16 +6873,13 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, union nfsd4_op_u *u) { struct nfsd4_release_lockowner *rlockowner = &u->release_lockowner; + struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); clientid_t *clid = &rlockowner->rl_clientid; - struct nfs4_stateowner *sop; - struct nfs4_lockowner *lo = NULL; struct nfs4_ol_stateid *stp; - struct xdr_netobj *owner = &rlockowner->rl_owner; - unsigned int hashval = ownerstr_hashval(owner); - __be32 status; - struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); + struct nfs4_lockowner *lo; struct nfs4_client *clp; - LIST_HEAD (reaplist); + LIST_HEAD(reaplist); + __be32 status; dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n", clid->cl_boot, clid->cl_id); @@ -6890,30 +6887,19 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, status = lookup_clientid(clid, cstate, nn); if (status) return status; - clp = cstate->clp; - /* Find the matching lock stateowner */ - spin_lock(&clp->cl_lock); - list_for_each_entry(sop, &clp->cl_ownerstr_hashtbl[hashval], - so_strhash) { - if (sop->so_is_open_owner || !same_owner_str(sop, owner)) - continue; - - if (atomic_read(&sop->so_count) != 1) { - spin_unlock(&clp->cl_lock); - return nfserr_locks_held; - } - - lo = lockowner(sop); - nfs4_get_stateowner(sop); - break; - } + spin_lock(&clp->cl_lock); + lo = find_lockowner_str_locked(clp, &rlockowner->rl_owner); if (!lo) { spin_unlock(&clp->cl_lock); return status; } - + if (atomic_read(&lo->lo_owner.so_count) != 2) { + spin_unlock(&clp->cl_lock); + nfs4_put_stateowner(&lo->lo_owner); + return nfserr_locks_held; + } unhash_lockowner_locked(lo); while (!list_empty(&lo->lo_owner.so_stateids)) { stp = list_first_entry(&lo->lo_owner.so_stateids, From patchwork Thu Feb 1 19:06:47 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chuck Lever X-Patchwork-Id: 13541672 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6081E85274; Thu, 1 Feb 2024 19:06:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706814409; cv=none; b=dmFTy7LmMEjFkJzOkYtco4oKojk4N0TYAQJXAfHsJCFyoUI43Oa9drjjB1+e/rOXFt1G51MLGaDrPmwcqWEAsNVf7O66DCnDOH+z3h8VibH/+Fi1mEfyZAxLgYrVFOoN2wzAwCVevA1kB0mX+XbvNAQBMOF0lLnUaCuZURgufmw= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706814409; c=relaxed/simple; bh=iH85Pz0com4u+I8MDjLgoTQAwlaAU32vDG21kw8Y+hc=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=fTGHnEzoGo25Tx83/nwAvsZRspU523v9MaVyxOr+1eLB8MkzcQ5TEPlr3//+9v0o3AKI1d05e7JOqJbqE7E1vndsm8aK2jiKnxxeVFNzVrZ0Elpk96pQ0geb9dA6oS3qYNKbTCZA/WQK8cQEmXu7R8Jz9NgLppfmu4sKqq+VxIM= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=nQgT0ASm; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="nQgT0ASm" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C1355C433C7; Thu, 1 Feb 2024 19:06:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1706814409; bh=iH85Pz0com4u+I8MDjLgoTQAwlaAU32vDG21kw8Y+hc=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=nQgT0ASmfO4xnEVE/DNGC97Fuoz+VIA1gMLW/SmPnEYUg2CwQ4LbRGsJ+FU3uzrYy 2oHPgLGeLIBMZmlhu1OZMsOpKIiFiNzWe5TdxHthpVqWy2Kbksz0FxqRWLUO/+Jlxm +GlFm9FIVvc+nvNYzdwf5u0ZK+WsX093kymBg8aAhsXQwN2vmKUFCeUIlmaOkPC+6f 2DBaIjbvaZBDnsj5Tx8/KcZoTe5g9VlNs5bWKxGhgmKmsDdowlO3crSyVOmB80jIYa 4UCX58RDxs7TX55cyK0Y4AzGqHWS4GY+IetmQ21qqz6iC01nohKR303tt5QgMRsUCq 3nRjRvqMbd20w== Subject: [PATCH 2/3] NFSD: Add documenting comment for nfsd4_release_lockowner() From: Chuck Lever To: stable@vger.kernel.org Cc: linux-nfs@vger.kernel.org Date: Thu, 01 Feb 2024 14:06:47 -0500 Message-ID: <170681440781.15250.14604698752530527458.stgit@klimt.1015granger.net> In-Reply-To: <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net> References: <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net> User-Agent: StGit/1.5 Precedence: bulk X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: Chuck Lever [ Upstream commit 043862b09cc00273e35e6c3a6389957953a34207 ] And return explicit nfserr values that match what is documented in the new comment / API contract. Stable-dep-of: edcf9725150e ("nfsd: fix RELEASE_LOCKOWNER") Signed-off-by: Chuck Lever --- fs/nfsd/nfs4state.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 9a77a3eac4ac..0dfc45d37658 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6867,6 +6867,23 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) return status; } +/** + * nfsd4_release_lockowner - process NFSv4.0 RELEASE_LOCKOWNER operations + * @rqstp: RPC transaction + * @cstate: NFSv4 COMPOUND state + * @u: RELEASE_LOCKOWNER arguments + * + * The lockowner's so_count is bumped when a lock record is added + * or when copying a conflicting lock. The latter case is brief, + * but can lead to fleeting false positives when looking for + * locks-in-use. + * + * Return values: + * %nfs_ok: lockowner released or not found + * %nfserr_locks_held: lockowner still in use + * %nfserr_stale_clientid: clientid no longer active + * %nfserr_expired: clientid not recognized + */ __be32 nfsd4_release_lockowner(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, @@ -6893,7 +6910,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, lo = find_lockowner_str_locked(clp, &rlockowner->rl_owner); if (!lo) { spin_unlock(&clp->cl_lock); - return status; + return nfs_ok; } if (atomic_read(&lo->lo_owner.so_count) != 2) { spin_unlock(&clp->cl_lock); @@ -6909,11 +6926,11 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, put_ol_stateid_locked(stp, &reaplist); } spin_unlock(&clp->cl_lock); + free_ol_stateid_reaplist(&reaplist); remove_blocked_locks(lo); nfs4_put_stateowner(&lo->lo_owner); - - return status; + return nfs_ok; } static inline struct nfs4_client_reclaim * From patchwork Thu Feb 1 19:06:54 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chuck Lever X-Patchwork-Id: 13541673 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B0ADE84FC7; Thu, 1 Feb 2024 19:06:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706814415; cv=none; b=HvXvi/luyk0surFSfDa1ENR/MSjg/9KcrsIZSh8Yg05yQxORIwi8BObq+IFF0Rc6456IbzJsLLVHz3iel/w/9vemqYrlUf6JKIeeVWY88H5h3z5GgBeKghIgfgzQV/4naMiHoDVlDxcadnV+jo8g3dPon8PWQ3Y6d8xf3vGfu3U= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1706814415; c=relaxed/simple; bh=BYOKs9NKPpuBG9IPS6whvOJvJ4k+wFljOI69fH2yZ58=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=ZgSb6FI0uDOfhBlDrSEVb6J0srCpgeQUptPkon/kEkBHV4Un/EmKDJGEuwfCoeqMEM7eOcy4ZrO9KMV5DvbirUbxguyU7HFDc3Gn4WUMgdKBRpwf7NqnbpRn9r+SX8pQ8lafusNSmvzunKRdXryKjaZGYSFYF6Zhc1dm7voiey4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=t6EcjmQ5; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="t6EcjmQ5" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2BEB0C433F1; Thu, 1 Feb 2024 19:06:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1706814415; bh=BYOKs9NKPpuBG9IPS6whvOJvJ4k+wFljOI69fH2yZ58=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=t6EcjmQ50xZQS2bpM24uRKIT0lFqCFeEnLaCjvlSxxzI5G8OIzKK1SZypF5KfHsnk SY3ZvI5GydAw72IcW9Cto7QPiV8Qrb90AWdcO5zMvEWzpDP88YQMdyLvVYMpcWHT6L PtHKz+PhduNp/DN487MsElXdYf+ejNmFtqZYUorwIC0YVWbg4sfKezVkjYKeOZvT/5 Ot5lCqJ0H3cUGi6sY7LnfFioVuXNmj8nYDBuJOxpvhHP+T+JctskoZBzk+7y3myPdw GCqdS7b/fj0osuvlHLnxwxHvT8TGMy0pWFBuUM4srTFNlmntSjgdreqiX0EzqLk0ZV EQJBC6kqzEp6w== Subject: [PATCH 3/3] nfsd: fix RELEASE_LOCKOWNER From: Chuck Lever To: stable@vger.kernel.org Cc: linux-nfs@vger.kernel.org Date: Thu, 01 Feb 2024 14:06:54 -0500 Message-ID: <170681441416.15250.7334579169354555886.stgit@klimt.1015granger.net> In-Reply-To: <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net> References: <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net> User-Agent: StGit/1.5 Precedence: bulk X-Mailing-List: linux-nfs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 From: NeilBrown [ Upstream commit edcf9725150e42beeca42d085149f4c88fa97afd ] The test on so_count in nfsd4_release_lockowner() is nonsense and harmful. Revert to using check_for_locks(), changing that to not sleep. First: harmful. As is documented in the kdoc comment for nfsd4_release_lockowner(), the test on so_count can transiently return a false positive resulting in a return of NFS4ERR_LOCKS_HELD when in fact no locks are held. This is clearly a protocol violation and with the Linux NFS client it can cause incorrect behaviour. If RELEASE_LOCKOWNER is sent while some other thread is still processing a LOCK request which failed because, at the time that request was received, the given owner held a conflicting lock, then the nfsd thread processing that LOCK request can hold a reference (conflock) to the lock owner that causes nfsd4_release_lockowner() to return an incorrect error. The Linux NFS client ignores that NFS4ERR_LOCKS_HELD error because it never sends NFS4_RELEASE_LOCKOWNER without first releasing any locks, so it knows that the error is impossible. It assumes the lock owner was in fact released so it feels free to use the same lock owner identifier in some later locking request. When it does reuse a lock owner identifier for which a previous RELEASE failed, it will naturally use a lock_seqid of zero. However the server, which didn't release the lock owner, will expect a larger lock_seqid and so will respond with NFS4ERR_BAD_SEQID. So clearly it is harmful to allow a false positive, which testing so_count allows. The test is nonsense because ... well... it doesn't mean anything. so_count is the sum of three different counts. 1/ the set of states listed on so_stateids 2/ the set of active vfs locks owned by any of those states 3/ various transient counts such as for conflicting locks. When it is tested against '2' it is clear that one of these is the transient reference obtained by find_lockowner_str_locked(). It is not clear what the other one is expected to be. In practice, the count is often 2 because there is precisely one state on so_stateids. If there were more, this would fail. In my testing I see two circumstances when RELEASE_LOCKOWNER is called. In one case, CLOSE is called before RELEASE_LOCKOWNER. That results in all the lock states being removed, and so the lockowner being discarded (it is removed when there are no more references which usually happens when the lock state is discarded). When nfsd4_release_lockowner() finds that the lock owner doesn't exist, it returns success. The other case shows an so_count of '2' and precisely one state listed in so_stateid. It appears that the Linux client uses a separate lock owner for each file resulting in one lock state per lock owner, so this test on '2' is safe. For another client it might not be safe. So this patch changes check_for_locks() to use the (newish) find_any_file_locked() so that it doesn't take a reference on the nfs4_file and so never calls nfsd_file_put(), and so never sleeps. With this check is it safe to restore the use of check_for_locks() rather than testing so_count against the mysterious '2'. Fixes: ce3c4ad7f4ce ("NFSD: Fix possible sleep during nfsd4_release_lockowner()") Signed-off-by: NeilBrown Reviewed-by: Jeff Layton Cc: stable@vger.kernel.org # v6.2+ Signed-off-by: Chuck Lever --- fs/nfsd/nfs4state.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 0dfc45d37658..bca22325083c 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -6840,14 +6840,16 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) { struct file_lock *fl; int status = false; - struct nfsd_file *nf = find_any_file(fp); + struct nfsd_file *nf; struct inode *inode; struct file_lock_context *flctx; + spin_lock(&fp->fi_lock); + nf = find_any_file_locked(fp); if (!nf) { /* Any valid lock stateid should have some sort of access */ WARN_ON_ONCE(1); - return status; + goto out; } inode = locks_inode(nf->nf_file); @@ -6863,7 +6865,8 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) } spin_unlock(&flctx->flc_lock); } - nfsd_file_put(nf); +out: + spin_unlock(&fp->fi_lock); return status; } @@ -6873,10 +6876,8 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) * @cstate: NFSv4 COMPOUND state * @u: RELEASE_LOCKOWNER arguments * - * The lockowner's so_count is bumped when a lock record is added - * or when copying a conflicting lock. The latter case is brief, - * but can lead to fleeting false positives when looking for - * locks-in-use. + * Check if theree are any locks still held and if not - free the lockowner + * and any lock state that is owned. * * Return values: * %nfs_ok: lockowner released or not found @@ -6912,10 +6913,13 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp, spin_unlock(&clp->cl_lock); return nfs_ok; } - if (atomic_read(&lo->lo_owner.so_count) != 2) { - spin_unlock(&clp->cl_lock); - nfs4_put_stateowner(&lo->lo_owner); - return nfserr_locks_held; + + list_for_each_entry(stp, &lo->lo_owner.so_stateids, st_perstateowner) { + if (check_for_locks(stp->st_stid.sc_file, lo)) { + spin_unlock(&clp->cl_lock); + nfs4_put_stateowner(&lo->lo_owner); + return nfserr_locks_held; + } } unhash_lockowner_locked(lo); while (!list_empty(&lo->lo_owner.so_stateids)) {