From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 E694525B0B7 for ; Sat, 23 May 2026 15:37:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779550666; cv=none; b=G/u2I6/fNYVewOAjBgqD6XiGDHiXbmUAIUNwd7qYoDY61lGELUdrEeiJV1VWgAuHhzafv+CFkvxhhmFLe8WiCdfQlzMn+23SXeM4CLg3MRAnGb0fXWDQthC/QAU144dkucHGlrZDjLifpLRUa7UOpXLd8QNoLcXH5mH1PsZAl5c= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779550666; c=relaxed/simple; bh=nuMhY67j2kXpkCL6F1g8rN7Lw63fswI+AVXZSRraKXw=; h=MIME-Version:Date:From:To:Cc:Message-Id:In-Reply-To:References: Subject:Content-Type; b=p7MLdsrVwtJTFk6QiPJZy3jWLaQmz5gj+FzjzypGbZ3IqMB/my6zNQy4qU2aydqUumZ4Bt9y8t9GXINtLo4TicsXJ4D+gGt0LU6jp9RTnhRtDDyyLhWdrI0G3Qv2CaMECpbaiirErX2hpeGwFoQfIMZKxxZpuXhH5Q6cZV+IQCY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ADnozeJ0; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ADnozeJ0" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5ABEB1F000E9; Sat, 23 May 2026 15:37:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1779550664; bh=b59jaKuT2GEGtCUAJvouyB/kv9iV89Owv4NA+3ki6tI=; h=Date:From:To:Cc:In-Reply-To:References:Subject; b=ADnozeJ0cpvfkZuUx3kYhoPovs41zzcBK1mPuxV+AWPvvxWssmMQyrdI5oNPcSbrC fBPdnxtHQjis0oi8RnwI3kCCJGmehjNzsgyDTGVOwyzZ2zNXBKyVoJ4XlOb+xz6dr4 yRwndDP69d25jBm9VXqPqqBjp9Q5LtFk/N/HJo9BBmYSbdOT/d85NQESYMtCDQ11RS 8f5oQljuZPXjoTToPnkLz3l2w5vA0Eru2UQvysaiYn6Ltw+FowP3re9PSLyQG/g5kO PFxbqp6h6k23kQpy/YaCvL4PDCn1X9nxxTlg3NsdqMocTqOpaaZceUz0MwXOVB5vEb vT1HWziXX+7bw== Received: from phl-compute-10.internal (phl-compute-10.internal [10.202.2.50]) by mailfauth.phl.internal (Postfix) with ESMTP id 8B8CCF40068; Sat, 23 May 2026 11:37:43 -0400 (EDT) Received: from phl-imap-15 ([10.202.2.104]) by phl-compute-10.internal (MEProxy); Sat, 23 May 2026 11:37:43 -0400 X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeefhedrtddtgdduheefgeegucetufdoteggodetrf dotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu rghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf gurhepofggfffhvfevkfgjfhfutgfgsehtjeertdertddtnecuhfhrohhmpedfvehhuhgt khcunfgvvhgvrhdfuceotggvlheskhgvrhhnvghlrdhorhhgqeenucggtffrrghtthgvrh hnpedtleejheethfelgefhjefgteeiteehudfgieevffegkeejkeevvdelieegvdeujeen ucffohhmrghinhepshgrshhhihhkohdruggvvhenucevlhhushhtvghrufhiiigvpedtne curfgrrhgrmhepmhgrihhlfhhrohhmpegthhhutghklhgvvhgvrhdomhgvshhmthhprghu thhhphgvrhhsohhnrghlihhthidqudeifeegleelleehledqfedvleekgeegvdefqdgtvg hlpeepkhgvrhhnvghlrdhorhhgsehfrghsthhmrghilhdrtghomhdpnhgspghrtghpthht ohepuddvpdhmohguvgepshhmthhpohhuthdprhgtphhtthhopehnvghilhessghrohifnh drnhgrmhgvpdhrtghpthhtohepmhhitghhrggvlhdrsghomhhmrghrihhtohesghhmrghi lhdrtghomhdprhgtphhtthhopegrnhhnrgeskhgvrhhnvghlrdhorhhgpdhrtghpthhtoh epjhhlrgihthhonheskhgvrhhnvghlrdhorhhgpdhrtghpthhtohepthhrohhnughmhies khgvrhhnvghlrdhorhhgpdhrtghpthhtohepuggrihdrnhhgohesohhrrggtlhgvrdgtoh hmpdhrtghpthhtoheptghhuhgtkhdrlhgvvhgvrhesohhrrggtlhgvrdgtohhmpdhrtghp thhtohepohhkohhrnhhivghvsehrvgguhhgrthdrtghomhdprhgtphhtthhopehtohhmse htrghlphgvhidrtghomh X-ME-Proxy: Feedback-ID: ifa6e4810:Fastmail Received: by mailuser.phl.internal (Postfix, from userid 501) id 6251C780070; Sat, 23 May 2026 11:37:43 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-ThreadId: AtPzEyN-PPL- Date: Sat, 23 May 2026 11:37:22 -0400 From: "Chuck Lever" To: "Michael Bommarito" , "Trond Myklebust" , "Anna Schumaker" , "Chuck Lever" , "Jeff Layton" Cc: NeilBrown , "Olga Kornievskaia" , "Dai Ngo" , "Tom Talpey" , linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Message-Id: In-Reply-To: <20260523123053.3480369-1-michael.bommarito@gmail.com> References: <20260523123053.3480369-1-michael.bommarito@gmail.com> Subject: Re: [PATCH v2] lockd: pin next file across nlm_inspect_file lock-drop Content-Type: text/plain Content-Transfer-Encoding: 7bit On Sat, May 23, 2026, at 8:30 AM, Michael Bommarito wrote: > nlm_traverse_files() pins the current file with f_count++ across > a mutex_unlock for nlm_inspect_file(), but nothing pins the saved > next pointer. A concurrent nlm_release_file() can kfree the next > file during the unlock window, and the iterator dereferences freed > memory on the next loop step. > > Pin both current and next before the lock-drop. Advance by > swapping the pinned cursors at the end of each iteration so next > is always held alive across the unlock. > > Only call nlm_file_release() for files that matched the predicate > and were inspected. Skipped files just get f_count-- to undo the > iteration pin; their f_locks is stale and must not drive cleanup. > > Cc: stable@vger.kernel.org > Fixes: 01df9c5e918a ("LOCKD: Fix a deadlock in nlm_traverse_files()") > Assisted-by: Claude:claude-opus-4-7 > Signed-off-by: Michael Bommarito > --- > fs/lockd/svcsubs.c | 64 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 44 insertions(+), 20 deletions(-) > > > Changes since v1: > - Fixed premature kfree of non-matching files: nlm_file_release() > is now called only for files that matched the predicate and were > inspected. Non-matching files just get f_count-- to undo the > iteration pin. (Spotted by sashiko.dev automated review.) > > Reproduced under UML + KASAN with 768 concurrent POSIX holders and > parallel /proc/fs/nfsd/unlock_filesystem writes. > > Stock kernel: > > BUG: KASAN: slab-use-after-free in nlm_traverse_files+0x71d/0x9d0 > > Allocated by: nlm_lookup_file via nlm4svc_proc_lock > Freed by: another nlm_traverse_files instance > > Patched v2 UML kernel ran the same harness silently. > > diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c > index dd0214dcb6950..0b38125cf86ab 100644 > --- a/fs/lockd/svcsubs.c > +++ b/fs/lockd/svcsubs.c > @@ -295,36 +295,60 @@ static void nlm_close_files(struct nlm_file *file) > /* > * Loop over all files in the file table. > */ > +static void nlm_file_release(struct nlm_file *file) > +{ > + if (list_empty(&file->f_blocks) && !file->f_locks > + && !file->f_shares && !file->f_count) { > + hlist_del(&file->f_list); > + nlm_close_files(file); > + kfree(file); > + } > +} > + > static int > nlm_traverse_files(void *data, nlm_host_match_fn_t match, > int (*is_failover_file)(void *data, struct nlm_file *file)) > { > - struct hlist_node *next; > - struct nlm_file *file; > + struct nlm_file *file, *next; > int i, ret = 0; > > mutex_lock(&nlm_file_mutex); > for (i = 0; i < FILE_NRHASH; i++) { > - hlist_for_each_entry_safe(file, next, &nlm_files[i], f_list) { > - if (is_failover_file && !is_failover_file(data, file)) > - continue; > + file = hlist_entry_safe(nlm_files[i].first, > + struct nlm_file, f_list); > + if (file) > file->f_count++; > - mutex_unlock(&nlm_file_mutex); > - > - /* Traverse locks, blocks and shares of this file > - * and update file->f_locks count */ > - if (nlm_inspect_file(data, file, match)) > - ret = 1; > - > - mutex_lock(&nlm_file_mutex); > - file->f_count--; > - /* No more references to this file. Let go of it. */ > - if (list_empty(&file->f_blocks) && !file->f_locks > - && !file->f_shares && !file->f_count) { > - hlist_del(&file->f_list); > - nlm_close_files(file); > - kfree(file); > + while (file) { > + /* > + * Pin the next neighbour before we drop the mutex > + * for nlm_inspect_file(); a concurrent > + * nlm_release_file() under the same mutex would > + * otherwise be free to unlink and kfree it during > + * the unlock window, leaving us to dereference a > + * freed slab when we walked to next afterwards. > + */ > + next = hlist_entry_safe(file->f_list.next, > + struct nlm_file, f_list); > + if (next) > + next->f_count++; > + > + if (!is_failover_file || is_failover_file(data, file)) { > + mutex_unlock(&nlm_file_mutex); > + > + /* > + * Traverse locks, blocks and shares of this > + * file and update file->f_locks count. > + */ > + if (nlm_inspect_file(data, file, match)) > + ret = 1; > + > + mutex_lock(&nlm_file_mutex); > + file->f_count--; > + nlm_file_release(file); > + } else { > + file->f_count--; > } > + file = next; > } > } > mutex_unlock(&nlm_file_mutex); > -- > 2.53.0 Codex (gpt-5.5 xhigh) review reports: > When nlmsvc_unlock_all_by_sb() skips a file from another superblock, > this branch can be handling an entry that was already pinned as the > previous file's saved next. If that entry's last real nlm_release_file() > ran while the traversal pin was held, it saw f_count > 0 and skipped > nlm_file_inuse()/deletion; this decrement then takes f_count to zero > without any cleanup, leaving the nlm_file and its open f_file > references hashed indefinitely. It appears that sashiko identified the same issue: https://sashiko.dev/#/patchset/20260523123053.3480369-1-michael.bommarito@gmail.com?part=1 -- Chuck Lever