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

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

[Nikola Z. Ivanov]

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;
 	}
 
 	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;
-- 
2.51.0



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

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

[Shuah Khan]

On 10/3/25 07:47, 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.

How does this change fix the problem? What is common corruption
path in this context?

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

How did you test this patch? Can add details?

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



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

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

[Chao Yu via Linux-f2fs-devel]

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;



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

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

[Nikola Z. Ivanov]

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.

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

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

[Chao Yu via Linux-f2fs-devel]

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,

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

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

[Nikola Z. Ivanov]

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.

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


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

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

[Chao Yu via Linux-f2fs-devel]

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

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

[Nikola Z. Ivanov]

On Wed, Oct 15, 2025 at 02:41:53PM +0800, Chao Yu wrote:
> 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,
> 

Hi Chao,

Just to make sure we're on the same page, do you mean to keep the check 
in f2fs_unlink as well as sanity_check_inode, or only do it in f2fs_unlink?

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

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

[Nikola Z. Ivanov]

On Wed, Oct 15, 2025 at 03:55:40PM +0300, Nikola Z. Ivanov wrote:
> On Wed, Oct 15, 2025 at 02:41:53PM +0800, Chao Yu wrote:
> > 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,
> > 
> 
> Hi Chao,
> 
> Just to make sure we're on the same page, do you mean to keep the check 
> in f2fs_unlink as well as sanity_check_inode, or only do it in f2fs_unlink?
> 

Hello,

Kindly sending a reminder here, could you please
provide input when you have the chance?

Thank you!

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

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

[Chao Yu via Linux-f2fs-devel]

On 10/24/25 03:15, Nikola Z. Ivanov wrote:
> On Wed, Oct 15, 2025 at 03:55:40PM +0300, Nikola Z. Ivanov wrote:
>> On Wed, Oct 15, 2025 at 02:41:53PM +0800, Chao Yu wrote:
>>> 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,
>>>
>>
>> Hi Chao,
>>
>> Just to make sure we're on the same page, do you mean to keep the check 
>> in f2fs_unlink as well as sanity_check_inode, or only do it in f2fs_unlink?
>>
> 
> Hello,
> 
> Kindly sending a reminder here, could you please
> provide input when you have the chance?

Hi Nikola,

Sorry for the delay, I missed to reply.

I meant that we can add sanity check (i_nlink == 1) for directory in:
- f2fs_unlink(), so that, we can detect runtime i_nlink inconsistency.
- sanity_check_inode(), then, we can detect on-disk i_nlink inconsistency.

Thanks,

> 
> Thank you!
> 
>>>>
>>>> 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