Re: [PATCH v4] f2fs: add check for deleted inode

[Chao Yu <chao@kernel.org>]

On 2/14/25 09:44, Chao Yu wrote:
> On 2/14/25 01:38, Jaegeuk Kim wrote:
>> On 02/13, Chao Yu wrote:
>>> On 2/13/25 00:47, Jaegeuk Kim wrote:
>>>> On 02/12, Chao Yu wrote:
>>>>> From: Leo Stone <leocstone@gmail.com>
>>>>>
>>>>> The syzbot reproducer mounts a f2fs image, then tries to unlink an
>>>>> existing file. However, the unlinked file already has a link count of 0
>>>>> when it is read for the first time in do_read_inode().
>>>>>
>>>>> Add a check to sanity_check_inode() for i_nlink == 0.
>>>>>
>>>>> [Chao Yu: rebase the code and fix orphan inode recovery issue]
>>>>> Reported-by: syzbot+b01a36acd7007e273a83@syzkaller.appspotmail.com
>>>>> Closes: https://syzkaller.appspot.com/bug?extid=b01a36acd7007e273a83
>>>>> Fixes: 39a53e0ce0df ("f2fs: add superblock and major in-memory structure")
>>>>> Signed-off-by: Leo Stone <leocstone@gmail.com>
>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>> ---
>>>>>  fs/f2fs/checkpoint.c | 4 ++++
>>>>>  fs/f2fs/f2fs.h       | 1 +
>>>>>  fs/f2fs/inode.c      | 6 ++++++
>>>>>  3 files changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>> index bd890738b94d..ada2c548645c 100644
>>>>> --- a/fs/f2fs/checkpoint.c
>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>> @@ -751,6 +751,8 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
>>>>>  	if (is_sbi_flag_set(sbi, SBI_IS_WRITABLE))
>>>>>  		f2fs_info(sbi, "orphan cleanup on readonly fs");
>>>>>  
>>>>> +	set_sbi_flag(sbi, SBI_ORPHAN_RECOVERY);
>>>>
>>>> What about using SBI_POR_DOING?
>>>
>>> SBI_POR_DOING will cover most flow of f2fs_fill_super(), I think we can add a
>>> separated flag just covering f2fs_recover_orphan_inodes(), so that we can allow
>>> iget() of root_inode and all inodes during roll-forward recovery to do sanity
>>> check nlink w/ zero. What do you think?
>>
>> Can we do this sanity check after f2fs_iget in the f2fs_unlink() only?
> 
> Sure, we need to cover f2fs_rename() as well, please check this:
> 
> https://lore.kernel.org/all/67450f9a.050a0220.21d33d.0003.GAE@google.com

Hi Jaegeuk,

I'm testing this, seems there is a problem, once we opened an inode that
has zeroed nlink, in f2fs_evict_inode(), the inode and all its data will be
deleted, then leaving its stale dir entry in parent directory.

What do you think using v4? so that we may has chance to repair it w/ fsck
rather than just deleting it?

---
 fs/f2fs/namei.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index a278c7da8177..949621bc0d07 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -547,6 +547,16 @@ static int f2fs_unlink(struct inode *dir, struct dentry *dentry)
 		goto fail;
 	}

+	if (unlikely(S_ISDIR(inode->i_mode) ?
+			inode->i_nlink <= 1 : inode->i_nlink == 0)) {
+		f2fs_err_ratelimited(sbi, "%s: inode (ino=%lx) has inconsistent nlink: %u, isdir: %d",
+				__func__, inode->i_ino, inode->i_nlink,
+				S_ISDIR(inode->i_mode));
+		err = -EFSCORRUPTED;
+		set_sbi_flag(sbi, SBI_NEED_FSCK);
+		goto fail;
+	}
+
 	err = f2fs_dquot_initialize(dir);
 	if (err)
 		goto fail;
@@ -968,6 +978,15 @@ static int f2fs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 	}

 	if (new_inode) {
+		if (unlikely(old_is_dir ?
+			new_inode->i_nlink <= 1 : new_inode->i_nlink == 0)) {
+			f2fs_err_ratelimited(sbi, "%s: inode (ino=%lx) has inconsistent nlink: %u, isdir: %d",
+				__func__, new_inode->i_ino, new_inode->i_nlink,
+				S_ISDIR(new_inode->i_mode));
+			err = -EFSCORRUPTED;
+			set_sbi_flag(sbi, SBI_NEED_FSCK);
+			goto out_dir;
+		}

 		err = -ENOTEMPTY;
 		if (old_is_dir && !f2fs_empty_dir(new_inode))
-- 
2.48.1.601.g30ceb7b040-goog

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>> +
>>>>>  	start_blk = __start_cp_addr(sbi) + 1 + __cp_payload(sbi);
>>>>>  	orphan_blocks = __start_sum_addr(sbi) - 1 - __cp_payload(sbi);
>>>>>  
>>>>> @@ -778,9 +780,11 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi)
>>>>>  		}
>>>>>  		f2fs_put_page(page, 1);
>>>>>  	}
>>>>> +
>>>>>  	/* clear Orphan Flag */
>>>>>  	clear_ckpt_flags(sbi, CP_ORPHAN_PRESENT_FLAG);
>>>>>  out:
>>>>> +	clear_sbi_flag(sbi, SBI_ORPHAN_RECOVERY);
>>>>>  	set_sbi_flag(sbi, SBI_IS_RECOVERED);
>>>>>  
>>>>>  	return err;
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index 05879c6dc4d6..1c75081c0c14 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -1322,6 +1322,7 @@ enum {
>>>>>  	SBI_IS_CLOSE,				/* specify unmounting */
>>>>>  	SBI_NEED_FSCK,				/* need fsck.f2fs to fix */
>>>>>  	SBI_POR_DOING,				/* recovery is doing or not */
>>>>> +	SBI_ORPHAN_RECOVERY,			/* orphan inodes recovery is doing */
>>>>>  	SBI_NEED_SB_WRITE,			/* need to recover superblock */
>>>>>  	SBI_NEED_CP,				/* need to checkpoint */
>>>>>  	SBI_IS_SHUTDOWN,			/* shutdown by ioctl */
>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>> index d6ad7810df69..02f1b69d03d8 100644
>>>>> --- a/fs/f2fs/inode.c
>>>>> +++ b/fs/f2fs/inode.c
>>>>> @@ -386,6 +386,12 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>>>>  		}
>>>>>  	}
>>>>>  
>>>>> +	if (inode->i_nlink == 0 && !is_sbi_flag_set(sbi, SBI_ORPHAN_RECOVERY)) {
>>>>> +		f2fs_warn(sbi, "%s: inode (ino=%lx) has a link count of 0",
>>>>> +			  __func__, inode->i_ino);
>>>>> +		return false;
>>>>> +	}
>>>>> +
>>>>>  	return true;
>>>>>  }
>>>>>  
>>>>> -- 
>>>>> 2.48.1.502.g6dc24dfdaf-goog
>