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

[Chao Yu <chao@kernel.org>]

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
>>>>
>>