[PATCH] f2fs: Add check for deleted inode

[PATCH] f2fs: Add check for deleted inode

[Leo Stone]

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.

#syz test

Reported-by: syzbot+b01a36acd7007e273a83@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b01a36acd7007e273a83
Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Leo Stone <leocstone@gmail.com>
---
 fs/f2fs/inode.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 1ed86df343a5..65f1dc32f173 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -372,6 +372,12 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
 		return false;
 	}
 
+	if (inode->i_nlink == 0) {
+		f2fs_warn(sbi, "%s: inode (ino=%lx) has a link count of 0",
+			  __func__, inode->i_ino);
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.43.0

Re: [syzbot] [f2fs?] WARNING in f2fs_unlink

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-by: syzbot+b01a36acd7007e273a83@syzkaller.appspotmail.com
Tested-by: syzbot+b01a36acd7007e273a83@syzkaller.appspotmail.com

Tested on:

commit:         9f16d5e6 Merge tag 'for-linus' of git://git.kernel.org..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=14d40778580000
kernel config:  https://syzkaller.appspot.com/x/.config?x=e92fc420ca55fe33
dashboard link: https://syzkaller.appspot.com/bug?extid=b01a36acd7007e273a83
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=15dd69c0580000

Note: testing is done by a robot and is best-effort only.

Re: [PATCH] f2fs: Add check for deleted inode

[Chao Yu]

On 2024/11/24 9:04, Leo Stone wrote:
> 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.
> 
> #syz test
> 
> Reported-by: syzbot+b01a36acd7007e273a83@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=b01a36acd7007e273a83
> Fixes: 4c8ff7095bef ("f2fs: support data compression")

Hello Leo,

I'm fine w/ this change, but I didn't get it, how did above commit introduce
this bug?

Thanks,

> Signed-off-by: Leo Stone <leocstone@gmail.com>
> ---
>   fs/f2fs/inode.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 1ed86df343a5..65f1dc32f173 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -372,6 +372,12 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>   		return false;
>   	}
>   
> +	if (inode->i_nlink == 0) {
> +		f2fs_warn(sbi, "%s: inode (ino=%lx) has a link count of 0",
> +			  __func__, inode->i_ino);
> +		return false;
> +	}
> +
>   	return true;
>   }
>   

Re: [PATCH] f2fs: Add check for deleted inode

[Leo Stone]

On Mon, Nov 25, 2024 at 07:16:41PM +0800, Chao Yu wrote:
> 
> I'm fine w/ this change, but I didn't get it, how did above commit introduce
> this bug?

Hello Chao,

The commit from the bisect didn't exactly introduce this bug, since it
would still be possible to make a different image that does the exact
same thing in the older code.

This commit was blamed in the bisect because it changes the layout of
struct f2fs_inode:

> @@ -271,6 +272,10 @@ struct f2fs_inode {
> 			__le32 i_inode_checksum;/* inode meta checksum */
> 			__le64 i_crtime;	/* creation time */
> 			__le32 i_crtime_nsec;	/* creation time in nano scale */
>+			__le64 i_compr_blocks;	/* # of compressed blocks */
>+			__u8 i_compress_algorithm;	/* compress algorithm */
>+			__u8 i_log_cluster_size;	/* log of cluster size */
>+			__le16 i_padding;		/* padding */
> 			__le32 i_extra_end[0];	/* for attribute size calculation */
> 		} __packed;
> 		__le32 i_addr[DEF_ADDRS_PER_INODE];	/* Pointers to data blocks */

This changes F2FS_TOTAL_EXTRA_ATTR_SIZE, which allows the syzbot
reproducer's inode to pass the sanity check for corrupted i_extra_size.
Before this change, the inode gets rejected as corrupt:

>[   62.794566][ T9662] F2FS-fs (loop0): sanity_check_inode: inode (ino=7) has corrupted i_extra_isize: 36, max: 24

Thanks,
Leo

Re: [PATCH] f2fs: Add check for deleted inode

[Chao Yu]

On 2024/11/26 1:56, Leo Stone wrote:
> On Mon, Nov 25, 2024 at 07:16:41PM +0800, Chao Yu wrote:
>>
>> I'm fine w/ this change, but I didn't get it, how did above commit introduce
>> this bug?
> 
> Hello Chao,
> 
> The commit from the bisect didn't exactly introduce this bug, since it
> would still be possible to make a different image that does the exact
> same thing in the older code.
> 
> This commit was blamed in the bisect because it changes the layout of
> struct f2fs_inode:
> 
>> @@ -271,6 +272,10 @@ struct f2fs_inode {
>> 			__le32 i_inode_checksum;/* inode meta checksum */
>> 			__le64 i_crtime;	/* creation time */
>> 			__le32 i_crtime_nsec;	/* creation time in nano scale */
>> +			__le64 i_compr_blocks;	/* # of compressed blocks */
>> +			__u8 i_compress_algorithm;	/* compress algorithm */
>> +			__u8 i_log_cluster_size;	/* log of cluster size */
>> +			__le16 i_padding;		/* padding */
>> 			__le32 i_extra_end[0];	/* for attribute size calculation */
>> 		} __packed;
>> 		__le32 i_addr[DEF_ADDRS_PER_INODE];	/* Pointers to data blocks */
> 
> This changes F2FS_TOTAL_EXTRA_ATTR_SIZE, which allows the syzbot
> reproducer's inode to pass the sanity check for corrupted i_extra_size.
> Before this change, the inode gets rejected as corrupt:
> 
>> [   62.794566][ T9662] F2FS-fs (loop0): sanity_check_inode: inode (ino=7) has corrupted i_extra_isize: 36, max: 24

Leo,

Ah, right, thanks for the explanation, anyway, can you please correct this
"Fixes line"?

Thanks,

> 
> Thanks,
> Leo