* [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() @ 2025-12-15 12:28 Chao Yu 2025-12-16 1:36 ` [f2fs-dev] " Zhiguo Niu 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2025-12-15 12:28 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu, stable In order to avoid loading corrupted nat entry from disk. Cc: stable@kernel.org Signed-off-by: Chao Yu <chao@kernel.org> --- 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 || + (__is_valid_data_blkaddr(ni->blk_addr) && !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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() 2025-12-15 12:28 [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() Chao Yu @ 2025-12-16 1:36 ` Zhiguo Niu 2025-12-16 8:49 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Zhiguo Niu @ 2025-12-16 1:36 UTC (permalink / raw) To: Chao Yu; +Cc: jaegeuk, stable, linux-kernel, linux-f2fs-devel Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> 于2025年12月15日周一 20:34写道： > > In order to avoid loading corrupted nat entry from disk. > > Cc: stable@kernel.org > Signed-off-by: Chao Yu <chao@kernel.org> > --- > 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? > + (__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? 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() 2025-12-16 1:36 ` [f2fs-dev] " Zhiguo Niu @ 2025-12-16 8:49 ` Chao Yu 2025-12-17 1:46 ` Zhiguo Niu 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2025-12-16 8:49 UTC (permalink / raw) To: Zhiguo Niu; +Cc: chao, jaegeuk, stable, linux-kernel, linux-f2fs-devel On 12/16/25 09:36, Zhiguo Niu wrote: > Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> > 于2025年12月15日周一 20:34写道： >> >> In order to avoid loading corrupted nat entry from disk. >> >> Cc: stable@kernel.org >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> 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; } + if (unlikely(f2fs_quota_file(sbi, ni->nid) && + __is_valid_data_blkaddr(ni->blk_addr))) { + 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); + } + /* 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() 2025-12-16 8:49 ` Chao Yu @ 2025-12-17 1:46 ` Zhiguo Niu 2025-12-17 8:03 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Zhiguo Niu @ 2025-12-17 1:46 UTC (permalink / raw) To: Chao Yu; +Cc: jaegeuk, stable, linux-kernel, linux-f2fs-devel Chao Yu <chao@kernel.org> 于2025年12月16日周二 16:49写道： > > On 12/16/25 09:36, Zhiguo Niu wrote: > > Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> > > 于2025年12月15日周一 20:34写道： > >> > >> In order to avoid loading corrupted nat entry from disk. > >> > >> Cc: stable@kernel.org > >> Signed-off-by: Chao Yu <chao@kernel.org> > >> --- > >> 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)?? > + 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? ^^ 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() 2025-12-17 1:46 ` Zhiguo Niu @ 2025-12-17 8:03 ` Chao Yu 2025-12-18 1:30 ` Zhiguo Niu 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2025-12-17 8:03 UTC (permalink / raw) To: Zhiguo Niu; +Cc: chao, jaegeuk, stable, linux-kernel, linux-f2fs-devel On 12/17/25 09:46, Zhiguo Niu wrote: > Chao Yu <chao@kernel.org> 于2025年12月16日周二 16:49写道： >> >> On 12/16/25 09:36, Zhiguo Niu wrote: >>> Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> >>> 于2025年12月15日周一 20:34写道： >>>> >>>> In order to avoid loading corrupted nat entry from disk. >>>> >>>> Cc: stable@kernel.org >>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>> --- >>>> 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() 2025-12-17 8:03 ` Chao Yu @ 2025-12-18 1:30 ` Zhiguo Niu 2025-12-19 2:52 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Zhiguo Niu @ 2025-12-18 1:30 UTC (permalink / raw) To: Chao Yu; +Cc: jaegeuk, stable, linux-kernel, linux-f2fs-devel Chao Yu <chao@kernel.org> 于2025年12月17日周三 16:03写道： > > On 12/17/25 09:46, Zhiguo Niu wrote: > > Chao Yu <chao@kernel.org> 于2025年12月16日周二 16:49写道： > >> > >> On 12/16/25 09:36, Zhiguo Niu wrote: > >>> Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> > >>> 于2025年12月15日周一 20:34写道： > >>>> > >>>> In order to avoid loading corrupted nat entry from disk. > >>>> > >>>> Cc: stable@kernel.org > >>>> Signed-off-by: Chao Yu <chao@kernel.org> > >>>> --- > >>>> 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. Hi Chao, Thanks for this explaination, so Could you resend this official patch? Thanks! > 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 > >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() 2025-12-18 1:30 ` Zhiguo Niu @ 2025-12-19 2:52 ` Chao Yu 0 siblings, 0 replies; 7+ messages in thread From: Chao Yu @ 2025-12-19 2:52 UTC (permalink / raw) To: Zhiguo Niu; +Cc: chao, jaegeuk, stable, linux-kernel, linux-f2fs-devel On 12/18/2025 9:30 AM, Zhiguo Niu wrote: > Chao Yu <chao@kernel.org> 于2025年12月17日周三 16:03写道： >> >> On 12/17/25 09:46, Zhiguo Niu wrote: >>> Chao Yu <chao@kernel.org> 于2025年12月16日周二 16:49写道： >>>> >>>> On 12/16/25 09:36, Zhiguo Niu wrote: >>>>> Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net> >>>>> 于2025年12月15日周一 20:34写道： >>>>>> >>>>>> In order to avoid loading corrupted nat entry from disk. >>>>>> >>>>>> Cc: stable@kernel.org >>>>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>>>> --- >>>>>> 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. > Hi Chao, > Thanks for this explaination, so > Could you resend this official patch? Done. :) Thanks, > Thanks! >> 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 >>>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-12-19 2:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-12-15 12:28 [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info() Chao Yu 2025-12-16 1:36 ` [f2fs-dev] " Zhiguo Niu 2025-12-16 8:49 ` Chao Yu 2025-12-17 1:46 ` Zhiguo Niu 2025-12-17 8:03 ` Chao Yu 2025-12-18 1:30 ` Zhiguo Niu 2025-12-19 2:52 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®