Re: [f2fs-dev] [PATCH] f2fs: fix to sanity check on nat_entry.nid in f2fs_get_node_info()

[Chao Yu]

On 12/18/2025 9:30 AM, Zhiguo Niu wrote:
> Chao Yu <chao@xxxxxxxxxx> 于2025年12月17日周三 16:03写道：
> > On 12/17/25 09:46, Zhiguo Niu wrote:
> > > Chao Yu <chao@xxxxxxxxxx> 于2025年12月16日周二 16:49写道：
> > > > On 12/16/25 09:36, Zhiguo Niu wrote:
> > > > > Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx>
> > > > > 于2025年12月15日周一 20:34写道：
> > > > > > In order to avoid loading corrupted nat entry from disk.
> > > > > >
> > > > > > Cc: stable@xxxxxxxxxx
> > > > > > Signed-off-by: Chao Yu <chao@xxxxxxxxxx>
> > > > > > ---
> > > > > >   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@xxxxxxxxxxxxxxxxxxxxx
> > > > > > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel