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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <zomib7dzvmnggqqy6aqlwij3zihbvzkzrnfvzhk7tcp2mdgh34@tjjugevo4q4a>
Date: Tue, 14 Oct 2025 15:17:04 +0300
From: "Nikola Z. Ivanov" <zlatistiv@...il.com>
To: Chao Yu <chao@...nel.org>
Cc: jaegeuk@...nel.org, linux-f2fs-devel@...ts.sourceforge.net, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, david.hunter.linux@...il.com, 
	linux-kernel-mentees@...ts.linuxfoundation.org, khalid@...nel.org, 
	syzbot+c07d47c7bc68f47b9083@...kaller.appspotmail.com
Subject: Re: [PATCH] f2fs: Perform sanity check before unlinking directory
 inode

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@...kaller.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@...il.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.

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?

Thank you,

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