From mboxrd@z Thu Jan 1 00:00:00 1970 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 78AF12A1BF for ; Wed, 17 Dec 2025 08:03:32 +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=1765958612; cv=none; b=bcVsvPJGS3es/VjlGtHdUWfdFJkn77OcPmCRiWa6hJ6UGNd5DIX8SBWgltTTOlD7L5nVCAPp75WbvdpdBqNK8S7mY3/2fJ9VLaG+qWhKpdFuao/URwFzpVPYXWfQ44qxCOy7g3sk4c8pZ4bXVCztZWTV0qpMLy4A9CrgQ1r4OEQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765958612; c=relaxed/simple; bh=nwYN6mfsYPWZwbPbiMDPXqHErTa/3LHWzTi1bo0ce+o=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=nWPHmqqCwrfgO7+slLRYuZaesPRSUb1LpJoGwtfTQhVT5HOZndho8lH4nurJLFUI9Qmkzz1QVbU3+myBwktci/FibVKXu7VF9YRBfC0T1HxPJY7sEjGL4fOlmUrVXwfNCt76363L2kJpTp9SRSMmjWpMnOvxt+gdU+qvtojKInI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=nCY7kReU; 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="nCY7kReU" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D5DBEC4CEF5; Wed, 17 Dec 2025 08:03:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1765958612; bh=nwYN6mfsYPWZwbPbiMDPXqHErTa/3LHWzTi1bo0ce+o=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=nCY7kReUW/CenHdEeGnAUeTmWK0Lgsg9K+k7V7pOjEdPTZkDHHquHTfD98Yqz3YsG qYPyHJa56cbuYPgDshIMDTdFhe69IAJVxVvPpCWWFPPVxWrIorebkTj5f9nwWLWbvs nNmQZAPP/NxDTc4dg8BmAiy912p3gQwiJ/jw3nya6wLS06cZrpjfkQcY7lAoJ8Nm2G TJ6UkiaPIl7pPjuyar52XwMo2hV6CNZIotdkgJ9Ll92DYpGYW0ypXsYvwD1VfKzG/K y/M1NXp46dtUfSbGlcBNdEGdtxt7cDLF4UrV4DC601ZLCQ40UMNs6DBIn1jp16Fb9F V2Yp0FGLjxbdw== Message-ID: <6483577e-7455-4531-a33f-90b6541d8e42@kernel.org> Date: Wed, 17 Dec 2025 16:03:28 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, jaegeuk@kernel.org, stable@kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() To: Zhiguo Niu References: <20251215122830.3495475-1-chao@kernel.org> <83a27ae7-ecc2-4d0d-8fb6-5841ed78b5eb@kernel.org> Content-Language: en-US From: Chao Yu In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 12/17/25 09:46, Zhiguo Niu wrote: > Chao Yu 于2025年12月16日周二 16:49写道： >> >> On 12/16/25 09:36, Zhiguo Niu wrote: >>> Chao Yu via Linux-f2fs-devel >>> 于2025年12月15日周一 20:34写道： >>>> >>>> In order to avoid loading corrupted nat entry from disk. >>>> >>>> Cc: stable@kernel.org >>>> Signed-off-by: Chao Yu >>>> --- >>>> fs/f2fs/node.c | 9 +++++---- >>>> 1 file changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >>>> index ce471e033774..13c88dfd790d 100644 >>>> --- a/fs/f2fs/node.c >>>> +++ b/fs/f2fs/node.c >>>> @@ -630,14 +630,15 @@ int f2fs_get_node_info(struct f2fs_sb_info *sbi, nid_t nid, >>>> node_info_from_raw_nat(ni, &ne); >>>> f2fs_folio_put(folio, true); >>>> sanity_check: >>>> - if (__is_valid_data_blkaddr(ni->blk_addr) && >>>> + if (unlikely(ni->nid != nid || >>> Hi Chao, >>> (ni->nid==nid) should be always true? because the code: >>> >>> ni->flag = 0; >>> ni->nid = nid; >>> retry: >>> or am I missing something? >> >> Zhiguo, >> >> Oh, I may missed something, let's ignore this patch. >> >>> >>>> + (__is_valid_data_blkaddr(ni->blk_addr) && >>> btw, Is it possible to detect that some valid Nid entries contain >>> incorrect content? >>> such as ino/block_addr=NULL_ADDR in nid=4 entry? >> >> Something like this? >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 65ca1a5eaa88..c458df92bb0d 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -4928,16 +4928,16 @@ static inline bool is_journalled_quota(struct f2fs_sb_info *sbi) >> return false; >> } >> >> -static inline bool f2fs_quota_file(struct inode *inode) >> +static inline bool f2fs_quota_file(struct f2fs_sb_info *sbi, nid_t ino) >> { >> #ifdef CONFIG_QUOTA >> int i; >> >> - if (!f2fs_sb_has_quota_ino(F2FS_I_SB(inode))) >> + if (!f2fs_sb_has_quota_ino(sbi)) >> return false; >> >> for (i = 0; i < MAXQUOTAS; i++) { >> - if (f2fs_qf_ino(F2FS_I_SB(inode)->sb, i) == inode->i_ino) >> + if (f2fs_qf_ino(sbi->sb, i) == ino) >> return true; >> } >> #endif >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c >> index 921fb02c0f49..d1270b25ad7d 100644 >> --- a/fs/f2fs/inode.c >> +++ b/fs/f2fs/inode.c >> @@ -621,7 +621,7 @@ make_now: >> inode->i_fop = &f2fs_file_operations; >> inode->i_mapping->a_ops = &f2fs_dblock_aops; >> if (IS_IMMUTABLE(inode) && !f2fs_compressed_file(inode) && >> - !f2fs_quota_file(inode)) >> + !f2fs_quota_file(sbi, inode->i_ino)) >> mapping_set_folio_min_order(inode->i_mapping, 0); >> } else if (S_ISDIR(inode->i_mode)) { >> inode->i_op = &f2fs_dir_inode_operations; >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >> index 7feead595ba5..10448e115ea0 100644 >> --- a/fs/f2fs/node.c >> +++ b/fs/f2fs/node.c >> @@ -643,6 +643,17 @@ sanity_check: >> return -EFSCORRUPTED; >> } >> > Hi Chao >> + if (unlikely(f2fs_quota_file(sbi, ni->nid) && >> + __is_valid_data_blkaddr(ni->blk_addr))) { > __is_valid_data_blkaddr(ni->blk_addr) --> ! > __is_valid_data_blkaddr(ni->blk_addr)?? Zhiguo, Oh, yes. >> + set_sbi_flag(sbi, SBI_NEED_FSCK); >> + f2fs_err_ratelimited(sbi, >> + "f2fs_get_node_info of %pS: inconsistent nat entry from qf_ino, " >> + "ino:%u, nid:%u, blkaddr:%u, ver:%u, flag:%u", >> + __builtin_return_address(0), >> + ni->ino, ni->nid, ni->blk_addr, ni->version, ni->flag); >> + f2fs_handle_error(sbi, ERROR_INCONSISTENT_NAT); >> + } >> + > I think this is ok for quota file, > and This is not easy to apply to all common cases( nid entry not only > for quota), right? ^^ Yes, I guess partial of them may be common case, which may happen in race condition, e.g. truncate vs read. Thanks, > Thanks! >> /* cache nat entry */ >> if (need_cache) >> cache_nat_entry(sbi, nid, &ne); >> >> Thanks, >> >>> Thanks >>>> !f2fs_is_valid_blkaddr(sbi, ni->blk_addr, >>>> - DATA_GENERIC_ENHANCE)) { >>>> + DATA_GENERIC_ENHANCE)))) { >>>> set_sbi_flag(sbi, SBI_NEED_FSCK); >>>> f2fs_err_ratelimited(sbi, >>>> - "f2fs_get_node_info of %pS: inconsistent nat entry, " >>>> + "f2fs_get_node_info of %pS: nid:%u, inconsistent nat entry, " >>>> "ino:%u, nid:%u, blkaddr:%u, ver:%u, flag:%u", >>>> - __builtin_return_address(0), >>>> + __builtin_return_address(0), nid, >>>> ni->ino, ni->nid, ni->blk_addr, ni->version, ni->flag); >>>> f2fs_handle_error(sbi, ERROR_INCONSISTENT_NAT); >>>> return -EFSCORRUPTED; >>>> -- >>>> 2.49.0 >>>> >>>> >>>> >>>> _______________________________________________ >>>> Linux-f2fs-devel mailing list >>>> Linux-f2fs-devel@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >>