Re: [f2fs-dev] [PATCH] f2fs: Perform sanity check before unlinking directory inode

[Chao Yu via Linux-f2fs-devel <linux-f2fs-devel@lists.sourceforge.net>]

On 10/14/25 20:17, Nikola Z. Ivanov wrote:
> On Mon, Oct 13, 2025 at 08:53:04PM +0800, Chao Yu wrote:
>> On 10/13/25 05:19, Nikola Z. Ivanov wrote:
>>> On Thu, Oct 09, 2025 at 10:54:40AM +0800, Chao Yu wrote:
>>>> On 10/3/2025 9:47 PM, Nikola Z. Ivanov wrote:
>>>>> Current i_nlink corruption check does not take into account
>>>>> directory inodes which have one additional i_nlink for their "." entry.
>>>>>
>>>>> Add additional check and a common corruption path.
>>>>>
>>>>> Reported-by: syzbot+c07d47c7bc68f47b9083@syzkaller.appspotmail.com
>>>>> Closes: https://syzkaller.appspot.com/bug?extid=c07d47c7bc68f47b9083
>>>>> Fixes: 81edb983b3f5 ("f2fs: add check for deleted inode")
>>>>> Signed-off-by: Nikola Z. Ivanov <zlatistiv@gmail.com>
>>>>> ---
>>>>>   fs/f2fs/namei.c | 28 ++++++++++++++++++++--------
>>>>>   1 file changed, 20 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>>>>> index b882771e4699..68b33e8089b0 100644
>>>>> --- a/fs/f2fs/namei.c
>>>>> +++ b/fs/f2fs/namei.c
>>>>> @@ -502,12 +502,14 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
>>>>>   		goto out;
>>>>>   	}
>>>>> -	if (inode->i_nlink == 0) {
>>>>> +	if (unlikely(inode->i_nlink == 0)) {
>>>>>   		f2fs_warn(F2FS_I_SB(inode), "%s: inode (ino=%lx) has zero i_nlink",
>>>>>   			  __func__, inode->i_ino);
>>>>> -		err = -EFSCORRUPTED;
>>>>> -		set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
>>>>> -		goto out_iput;
>>>>> +		goto corrupted;
>>>>> +	} else if (unlikely(S_ISDIR(inode->i_mode) && inode->i_nlink == 1)) {
>>>>> +		f2fs_warn(F2FS_I_SB(inode), "%s: directory inode (ino=%lx) has a single i_nlink",
>>>>> +			  __func__, inode->i_ino);
>>>>> +		goto corrupted;
>>>>
>>>> Can we detect such corruption in sanity_check_inode() as well? So that if
>>>> f2fs internal flow calls f2fs_iget() on corrupted inode, we can set SBI_NEED_FSCK
>>>> flag and then triggering fsck repairment later.
>>>>
>>>> Thanks,
>>>>
>>>>>   	}
>>>>>   	if (IS_ENCRYPTED(dir) &&
>>>>> @@ -533,6 +535,9 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
>>>>>   	trace_f2fs_lookup_end(dir, !IS_ERR_OR_NULL(new) ? new : dentry,
>>>>>   				ino, IS_ERR(new) ? PTR_ERR(new) : err);
>>>>>   	return new;
>>>>> +corrupted:
>>>>> +	err = -EFSCORRUPTED;
>>>>> +	set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
>>>>>   out_iput:
>>>>>   	iput(inode);
>>>>>   out:
>>>>> @@ -572,10 +577,11 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
>>>>>   	if (unlikely(inode->i_nlink == 0)) {
>>>>>   		f2fs_warn(F2FS_I_SB(inode), "%s: inode (ino=%lx) has zero i_nlink",
>>>>>   			  __func__, inode->i_ino);
>>>>> -		err = -EFSCORRUPTED;
>>>>> -		set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
>>>>> -		f2fs_folio_put(folio, false);
>>>>> -		goto fail;
>>>>> +		goto corrupted;
>>>>> +	} else if (unlikely(S_ISDIR(inode->i_mode) && inode->i_nlink == 1)) {
>>>>> +		f2fs_warn(F2FS_I_SB(inode), "%s: directory inode (ino=%lx) has a single i_nlink",
>>>>> +			  __func__, inode->i_ino);
>>>>> +		goto corrupted;
>>>>>   	}
>>>>>   	f2fs_balance_fs(sbi, true);
>>>>> @@ -601,6 +607,12 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
>>>>>   	if (IS_DIRSYNC(dir))
>>>>>   		f2fs_sync_fs(sbi->sb, 1);
>>>>> +
>>>>> +	goto fail;
>>>>> +corrupted:
>>>>> +	err = -EFSCORRUPTED;
>>>>> +	set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
>>>>> +	f2fs_folio_put(folio, false);
>>>>>   fail:
>>>>>   	trace_f2fs_unlink_exit(inode, err);
>>>>>   	return err;
>>>>
>>>
>>> Hi Chao,
>>>
>>> Thank you for the suggestion.
>>> I will add this to sanity_check_inode and remove it
>>> from f2fs_lookup as it becomes redundant since f2fs_lookup
>>> obtains the inode through f2fs_iget. For f2fs_unlink I will
>>> move the i_nlink == 1 check to f2fs_rmdir.
>>
>> Hi Nikola,
>>
>> I meant we can move the i_nlink == 1 check from both f2fs_lookup() and
>> f2fs_unlink() to sanity_check_inode(), because before we create in-memory
>> inode, we will always call sanity_check_inode().
>>
>> Let me know if you have other concerns.
>>
>> Thanks,
>>
> 
> The issue here is that sanity_check_inode will be called only when 
> we initially read the inode off disk, not when it's already in the cache
> 
> The syzkaller repro does something like this:
> Creates a directory structure /dir1/dir2 where dir1 has
> i_nlink == 2, which is one less than it should. It then does
> rmdir(/dir1/dir2) followed by rmdir(/dir1) which leads to the warning.

Oh, I missed this case.

> 
> In such case what would you say should happen, should the second rmdir
> fail and report the corruption, or do we close our eyes and just drop
> i_nlink to 0 and possibly log a message that something isn't quite right?

I agreed that we should keep i_nlink == 1 check in f2fs_unlink().

Thanks,

> 
> Thank you,
> 
>>>
>>> I will send v2 as soon as I do some more testing.
>>



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel