[f2fs-dev] On FI_INLINE_XATTR vs. FI_EXTRA_ATTR

Commit Message

Dmitry Antipov Dec. 20, 2024, 1:30 p.m. UTC

Does FI_INLINE_XATTR always assume FI_EXTRA_ATTR? Looking through the code, it's
not quite clear (at a first glance at least). If this is not so, I have a strong
suspicion that FI_INLINE_XATTR-related checks in sanity_check_inode() should be
moved to outer scope (outside of FI_EXTRA_ATTR check), i.e.:


(Looking around https://syzkaller.appspot.com/bug?extid=e4876215632c2d23b481).

Dmitry

Comments

Chao Yu Dec. 20, 2024, 1:34 p.m. UTC | #1

Hi, Dmitry,

I agree w/ you, I've figured out a patch previously, please check it?

https://lore.kernel.org/linux-f2fs-devel/[e-mail]

On 2024/12/20 21:30, Dmitry Antipov wrote:
> Does FI_INLINE_XATTR always assume FI_EXTRA_ATTR? Looking through the code, it's
> not quite clear (at a first glance at least). If this is not so, I have a strong
> suspicion that FI_INLINE_XATTR-related checks in sanity_check_inode() should be
> moved to outer scope (outside of FI_EXTRA_ATTR check), i.e.:
> 
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 282fd320bdb3..3061cf69a7fb 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -302,15 +302,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>    				  F2FS_TOTAL_EXTRA_ATTR_SIZE);
>    			return false;
>    		}
> -		if (f2fs_sb_has_flexible_inline_xattr(sbi) &&
> -			f2fs_has_inline_xattr(inode) &&
> -			(!fi->i_inline_xattr_size ||
> -			fi->i_inline_xattr_size > MAX_INLINE_XATTR_SIZE)) {
> -			f2fs_warn(sbi, "%s: inode (ino=%lx) has corrupted i_inline_xattr_size: %d, max: %lu",
> -				  __func__, inode->i_ino, fi->i_inline_xattr_size,
> -				  MAX_INLINE_XATTR_SIZE);
> -			return false;
> -		}
> +
>    		if (f2fs_sb_has_compression(sbi) &&
>    			fi->i_flags & F2FS_COMPR_FL &&
>    			F2FS_FITS_IN_INODE(ri, fi->i_extra_isize,
> @@ -320,6 +312,16 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>    		}
>    	}
> 
> +	if (f2fs_sb_has_flexible_inline_xattr(sbi) &&
> +	    f2fs_has_inline_xattr(inode) &&
> +	    (!fi->i_inline_xattr_size ||
> +	     fi->i_inline_xattr_size > MAX_INLINE_XATTR_SIZE)) {
> +		f2fs_warn(sbi, "%s: inode (ino=%lx) has corrupted i_inline_xattr_size: %d, max: %lu",
> +			  __func__, inode->i_ino, fi->i_inline_xattr_size,
> +			  MAX_INLINE_XATTR_SIZE);
> +		return false;
> +	}
> +
>    	if (!f2fs_sb_has_extra_attr(sbi)) {
>    		if (f2fs_sb_has_project_quota(sbi)) {
>    			f2fs_warn(sbi, "%s: corrupted inode ino=%lx, wrong feature flag: %u, run fsck to fix.",
> 
> (Looking around https://syzkaller.appspot.com/bug?extid=e4876215632c2d23b481).
> 
> Dmitry
>

Dmitry Antipov Dec. 20, 2024, 1:51 p.m. UTC | #2

On 12/20/24 4:34 PM, Chao Yu wrote:

> I agree w/ you, I've figured out a patch previously, please check it?

Hm... https://syzkaller.appspot.com/bug?extid=e4876215632c2d23b481 and
https://syzkaller.appspot.com/bug?extid=69f5379a1717a0b982a1 looks
identical (but the second one doesn't provide the reproducer). I've
tested this fix locally and submitted to syzbot under the first report
as well (still pending).

Dmitry

Chao Yu Dec. 20, 2024, 2:03 p.m. UTC | #3

On 2024/12/20 21:51, Dmitry Antipov wrote:
> On 12/20/24 4:34 PM, Chao Yu wrote:
> 
>> I agree w/ you, I've figured out a patch previously, please check it?
> 
> Hm... https://syzkaller.appspot.com/bug?extid=e4876215632c2d23b481 and
> https://syzkaller.appspot.com/bug?extid=69f5379a1717a0b982a1 looks
> identical (but the second one doesn't provide the reproducer). I've

Yes, I think so.

> tested this fix locally and submitted to syzbot under the first report
> as well (still pending).

I think our solutions are almost the same, I tested my patch w/ reproducer
downloaded from https://syzkaller.appspot.com/x/repro.c?x=13511730580000,
and also w/ below testcase, we can reproduce this bug more directly:

- mkfs.f2fs -f -O extra_attr,flexible_inline_xattr /dev/sdb
- mount -o inline_xattr_size=512 /dev/sdb /mnt/f2fs
- touch /mnt/f2fs/file
- umount /mnt/f2fs
- inject.f2fs --node --mb i_inline --nid 4 --val 0x1 /dev/sdb
- inject.f2fs --node --mb i_inline_xattr_size --nid 4 --val 2048 /dev/sdb
- mount /dev/sdb /mnt/f2fs
- getfattr /mnt/f2fs/file

Thanks,

> 
> Dmitry
>

Patch

diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 282fd320bdb3..3061cf69a7fb 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -302,15 +302,7 @@  static bool sanity_check_inode(struct inode *inode, struct page *node_page)
  				  F2FS_TOTAL_EXTRA_ATTR_SIZE);
  			return false;
  		}
-		if (f2fs_sb_has_flexible_inline_xattr(sbi) &&
-			f2fs_has_inline_xattr(inode) &&
-			(!fi->i_inline_xattr_size ||
-			fi->i_inline_xattr_size > MAX_INLINE_XATTR_SIZE)) {
-			f2fs_warn(sbi, "%s: inode (ino=%lx) has corrupted i_inline_xattr_size: %d, max: %lu",
-				  __func__, inode->i_ino, fi->i_inline_xattr_size,
-				  MAX_INLINE_XATTR_SIZE);
-			return false;
-		}
+
  		if (f2fs_sb_has_compression(sbi) &&
  			fi->i_flags & F2FS_COMPR_FL &&
  			F2FS_FITS_IN_INODE(ri, fi->i_extra_isize,
@@ -320,6 +312,16 @@  static bool sanity_check_inode(struct inode *inode, struct page *node_page)
  		}
  	}

+	if (f2fs_sb_has_flexible_inline_xattr(sbi) &&
+	    f2fs_has_inline_xattr(inode) &&
+	    (!fi->i_inline_xattr_size ||
+	     fi->i_inline_xattr_size > MAX_INLINE_XATTR_SIZE)) {
+		f2fs_warn(sbi, "%s: inode (ino=%lx) has corrupted i_inline_xattr_size: %d, max: %lu",
+			  __func__, inode->i_ino, fi->i_inline_xattr_size,
+			  MAX_INLINE_XATTR_SIZE);
+		return false;
+	}
+
  	if (!f2fs_sb_has_extra_attr(sbi)) {
  		if (f2fs_sb_has_project_quota(sbi)) {
  			f2fs_warn(sbi, "%s: corrupted inode ino=%lx, wrong feature flag: %u, run fsck to fix.",