f2fs: fix to do sanity check for inline inode

PATCH Superseded Chao Yu 2 participants 9 messages Apr 28, 2022 – May 13, 2022

Subsystems (2)

Stable branch F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2022-04-28 02:49 UTC #0 lore raw

PATCH 1/1 f2fs: fix to do sanity check for inline inode

As Yanming reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=215895

I have encountered a bug in F2FS file system in kernel v5.17.

The kernel message is shown below:

kernel BUG at fs/inode.c:611!

Call Trace:
 evict+0x282/0x4e0
 __dentry_kill+0x2b2/0x4d0
 dput+0x2dd/0x720
 do_renameat2+0x596/0x970
 __x64_sys_rename+0x78/0x90
 do_syscall_64+0x3b/0x90

The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().

This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().

Cc [email protected]

Reported-by Ming Yan <[email protected]>

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h  | 7 +++++++
 fs/f2fs/inode.c | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)

fs/f2fs/f2fs.h +7 -0

index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
  */
 static inline bool f2fs_post_read_required(struct inode *inode)
 {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
 	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
 		f2fs_compressed_file(inode);
 }

fs/f2fs/inode.c +1 -2

index 83639238a1fe..234b8ed02644 100644
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
 		}
 	}
 
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
 		set_sbi_flag(sbi, SBI_NEED_FSCK);
 		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
 			  __func__, inode->i_ino, inode->i_mode);

Signature · 1 line

2.25.1

Jaegeuk Kim

maintainer

2022-05-04 21:28 UTC #1 lore raw

Quoted 56 lines · Chao Yu

As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
 evict+0x282/0x4e0
 __dentry_kill+0x2b2/0x4d0
 dput+0x2dd/0x720
 do_renameat2+0x596/0x970
 __x64_sys_rename+0x78/0x90
 do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/f2fs.h  | 7 +++++++
 fs/f2fs/inode.c | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
  */
 static inline bool f2fs_post_read_required(struct inode *inode)
 {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
 	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
 		f2fs_compressed_file(inode);
 }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644

fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 279

Quoted 6 lines

fs/f2fs/inode.c +1 -2

@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
 		}
 	}
 
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {

It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.

Quoted 5 lines

fs/f2fs/inode.c +0 -0

 		set_sbi_flag(sbi, SBI_NEED_FSCK);
 		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
 			  __func__, inode->i_ino, inode->i_mode);

Signature · 1 line

2.25.1

Chao Yu

author maintainer

2022-05-05 02:49 UTC #2 lore raw

Quoted 60 lines · Jaegeuk Kim

Quoted 59 lines · Chao Yu
As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
  evict+0x282/0x4e0
  __dentry_kill+0x2b2/0x4d0
  dput+0x2dd/0x720
  do_renameat2+0x596/0x970
  __x64_sys_rename+0x78/0x90
  do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
  fs/f2fs/f2fs.h  | 7 +++++++
  fs/f2fs/inode.c | 3 +--
  2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
   */
  static inline bool f2fs_post_read_required(struct inode *inode)
  {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
  	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
  		f2fs_compressed_file(inode);
  }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
  		}
  	}
 

Quoted 5 lines

fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 279
Quoted 3 lines
fs/f2fs/inode.c +1 -2
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.

sanity_check_inode() change only affect f2fs_iget(), during inode initialization, file should not be set as atomic one, right?

I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you please provide me detail of the testcase?

Thanks,

Quoted 6 lines

fs/f2fs/inode.c +0 -0

  		set_sbi_flag(sbi, SBI_NEED_FSCK);
  		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
  			  __func__, inode->i_ino, inode->i_mode);

Signature · 1 line

2.25.1

Jaegeuk Kim

maintainer

2022-05-05 03:31 UTC #3 lore raw

Quoted 66 lines · Chao Yu

Quoted 56 lines · Jaegeuk Kim
Quoted 55 lines · Chao Yu
As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
  evict+0x282/0x4e0
  __dentry_kill+0x2b2/0x4d0
  dput+0x2dd/0x720
  do_renameat2+0x596/0x970
  __x64_sys_rename+0x78/0x90
  do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
  fs/f2fs/f2fs.h  | 7 +++++++
  fs/f2fs/inode.c | 3 +--
  2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
   */
  static inline bool f2fs_post_read_required(struct inode *inode)
  {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
  	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
  		f2fs_compressed_file(inode);
  }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644
Quoted 8 lines
fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 278
Quoted 6 lines
fs/f2fs/inode.c +1 -2
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
  		}
  	}
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.

Quoted 5 lines

sanity_check_inode() change only affect f2fs_iget(), during inode initialization, file should not be set as atomic one, right?
I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you please provide me detail of the testcase?

I just applied this into my device and was getting lots of the below error messages resulting in open failures of database files.

Quoted 9 lines

Thanks,

Quoted 6 lines

fs/f2fs/inode.c +0 -0

  		set_sbi_flag(sbi, SBI_NEED_FSCK);
  		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
  			  __func__, inode->i_ino, inode->i_mode);

Signature · 1 line

2.25.1

Chao Yu

author maintainer

2022-05-05 14:34 UTC #4 lore raw

Quoted 67 lines · Jaegeuk Kim

Quoted 66 lines · Chao Yu
Quoted 64 lines · Jaegeuk Kim
Quoted 56 lines · Chao Yu
As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
   evict+0x282/0x4e0
   __dentry_kill+0x2b2/0x4d0
   dput+0x2dd/0x720
   do_renameat2+0x596/0x970
   __x64_sys_rename+0x78/0x90
   do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
   fs/f2fs/f2fs.h  | 7 +++++++
   fs/f2fs/inode.c | 3 +--
   2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
    */
   static inline bool f2fs_post_read_required(struct inode *inode)
   {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
   	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
   		f2fs_compressed_file(inode);
   }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644
fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 278
Quoted 5 lines
fs/f2fs/inode.c +1 -2
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
   		}
   	}
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.

Quoted 8 lines

Quoted 5 lines
sanity_check_inode() change only affect f2fs_iget(), during inode initialization, file should not be set as atomic one, right?
I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you please provide me detail of the testcase?
I just applied this into my device and was getting lots of the below error messages resulting in open failures of database files.

Could you please help to apply below patch and dump the log?

From: Chao Yu <[email protected]>
Subject: [PATCH] f2fs: fix to do sanity check for inline inode

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/f2fs.h  |  7 +++++++
 fs/f2fs/inode.c | 11 +++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

fs/f2fs/f2fs.h +7 -0

index 0f8c426aed50..13a9212d6cb6 100644
@@ -4159,6 +4159,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
  */
 static inline bool f2fs_post_read_required(struct inode *inode)
 {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
 	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
 		f2fs_compressed_file(inode);
 }

fs/f2fs/inode.c +7 -4

index 02630c17da93..a98614a24ad0 100644
@@ -276,11 +276,14 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
 		}
 	}
 
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
 		set_sbi_flag(sbi, SBI_NEED_FSCK);
-		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
-			  __func__, inode->i_ino, inode->i_mode);
+		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) reason(%d, %llu, %ld, %d, %d, %lu) should not have inline_data, run fsck to fix",
+			  __func__, inode->i_ino, inode->i_mode,
+			  f2fs_is_atomic_file(inode),
+			  i_size_read(inode), MAX_INLINE_DATA(inode),
+			  file_is_encrypt(inode), file_is_verity(inode),
+			  F2FS_I(inode)->i_flags & F2FS_COMPR_FL);
 		return false;
 	}

Signature · 1 line

2.32.0

Quoted 10 lines

Thanks,

Quoted 6 lines

fs/f2fs/inode.c +0 -0

   		set_sbi_flag(sbi, SBI_NEED_FSCK);
   		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
   			  __func__, inode->i_ino, inode->i_mode);

Signature · 1 line

2.25.1

Chao Yu

author maintainer

2022-05-08 13:52 UTC #5 lore raw

Ping,

Quoted 129 lines · Chao Yu

Quoted 67 lines · Jaegeuk Kim
Quoted 66 lines · Chao Yu
Quoted 64 lines · Jaegeuk Kim
Quoted 56 lines · Chao Yu
As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
    evict+0x282/0x4e0
    __dentry_kill+0x2b2/0x4d0
    dput+0x2dd/0x720
    do_renameat2+0x596/0x970
    __x64_sys_rename+0x78/0x90
    do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
    fs/f2fs/f2fs.h  | 7 +++++++
    fs/f2fs/inode.c | 3 +--
    2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
     */
    static inline bool f2fs_post_read_required(struct inode *inode)
    {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
    	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
    		f2fs_compressed_file(inode);
    }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644
fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 278
Quoted 5 lines
fs/f2fs/inode.c +1 -2
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
    		}
    	}
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.
Quoted 8 lines
Quoted 5 lines
sanity_check_inode() change only affect f2fs_iget(), during inode initialization, file should not be set as atomic one, right?
I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you please provide me detail of the testcase?
I just applied this into my device and was getting lots of the below error messages resulting in open failures of database files.
Could you please help to apply below patch and dump the log?
From: Chao Yu <[email protected]>
Subject: [PATCH] f2fs: fix to do sanity check for inline inode
Signed-off-by Chao Yu <[email protected]>
---
   fs/f2fs/f2fs.h  |  7 +++++++
   fs/f2fs/inode.c | 11 +++++++----
   2 files changed, 14 insertions(+), 4 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 0f8c426aed50..13a9212d6cb6 100644
@@ -4159,6 +4159,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
    */
   static inline bool f2fs_post_read_required(struct inode *inode)
   {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
   	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
   		f2fs_compressed_file(inode);
   }
fs/f2fs/inode.c +7 -4
index 02630c17da93..a98614a24ad0 100644
@@ -276,11 +276,14 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
   		}
   	}
 
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
   		set_sbi_flag(sbi, SBI_NEED_FSCK);
-		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
-			  __func__, inode->i_ino, inode->i_mode);
+		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) reason(%d, %llu, %ld, %d, %d, %lu) should not have inline_data, run fsck to fix",
+			  __func__, inode->i_ino, inode->i_mode,
+			  f2fs_is_atomic_file(inode),
+			  i_size_read(inode), MAX_INLINE_DATA(inode),
+			  file_is_encrypt(inode), file_is_verity(inode),
+			  F2FS_I(inode)->i_flags & F2FS_COMPR_FL);
   		return false;
   	}
 

Jaegeuk Kim

maintainer

2022-05-09 16:57 UTC #6 lore raw

Quoted 1 line · Chao Yu

Ping,

This is in my TODO list, but will take some time. Sorry.

Quoted 131 lines · Chao Yu

Quoted 129 lines · Jaegeuk Kim
Quoted 67 lines · Chao Yu
Quoted 66 lines · Jaegeuk Kim
Quoted 64 lines · Chao Yu
Quoted 56 lines
As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
    evict+0x282/0x4e0
    __dentry_kill+0x2b2/0x4d0
    dput+0x2dd/0x720
    do_renameat2+0x596/0x970
    __x64_sys_rename+0x78/0x90
    do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
    fs/f2fs/f2fs.h  | 7 +++++++
    fs/f2fs/inode.c | 3 +--
    2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
     */
    static inline bool f2fs_post_read_required(struct inode *inode)
    {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
    	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
    		f2fs_compressed_file(inode);
    }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644
fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 278
Quoted 5 lines
fs/f2fs/inode.c +1 -2
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
    		}
    	}
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.
Quoted 8 lines
Quoted 5 lines
sanity_check_inode() change only affect f2fs_iget(), during inode initialization, file should not be set as atomic one, right?
I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you please provide me detail of the testcase?
I just applied this into my device and was getting lots of the below error messages resulting in open failures of database files.
Could you please help to apply below patch and dump the log?
From: Chao Yu <[email protected]>
Subject: [PATCH] f2fs: fix to do sanity check for inline inode
Signed-off-by Chao Yu <[email protected]>
---
   fs/f2fs/f2fs.h  |  7 +++++++
   fs/f2fs/inode.c | 11 +++++++----
   2 files changed, 14 insertions(+), 4 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 0f8c426aed50..13a9212d6cb6 100644
@@ -4159,6 +4159,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
    */
   static inline bool f2fs_post_read_required(struct inode *inode)
   {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
   	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
   		f2fs_compressed_file(inode);
   }
fs/f2fs/inode.c +7 -4
index 02630c17da93..a98614a24ad0 100644
@@ -276,11 +276,14 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
   		}
   	}
 
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
   		set_sbi_flag(sbi, SBI_NEED_FSCK);
-		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
-			  __func__, inode->i_ino, inode->i_mode);
+		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) reason(%d, %llu, %ld, %d, %d, %lu) should not have inline_data, run fsck to fix",
+			  __func__, inode->i_ino, inode->i_mode,
+			  f2fs_is_atomic_file(inode),
+			  i_size_read(inode), MAX_INLINE_DATA(inode),
+			  file_is_encrypt(inode), file_is_verity(inode),
+			  F2FS_I(inode)->i_flags & F2FS_COMPR_FL);
   		return false;
   	}
 

Jaegeuk Kim

maintainer

2022-05-13 21:35 UTC #7 lore raw

Quoted 4 lines · Jaegeuk Kim

Quoted 1 line · Chao Yu
Ping,
This is in my TODO list, but will take some time. Sorry.

Got this.

sanity_check_inode: inode (ino=1125d, mode=41471) reason(0, 98, 3452, 4, 0, 0) should not have inline_data, run fsck to fix

Ok, this is a symlink, which was encrypted having inline_data.

Quoted 138 lines

Quoted 117 lines · Chao Yu
Quoted 114 lines · Jaegeuk Kim
Quoted 67 lines · Chao Yu
Quoted 66 lines · Jaegeuk Kim
Quoted 64 lines · Chao Yu
Quoted 56 lines
As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
    evict+0x282/0x4e0
    __dentry_kill+0x2b2/0x4d0
    dput+0x2dd/0x720
    do_renameat2+0x596/0x970
    __x64_sys_rename+0x78/0x90
    do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
    fs/f2fs/f2fs.h  | 7 +++++++
    fs/f2fs/inode.c | 3 +--
    2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
     */
    static inline bool f2fs_post_read_required(struct inode *inode)
    {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
    	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
    		f2fs_compressed_file(inode);
    }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644
fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 278
Quoted 5 lines
fs/f2fs/inode.c +1 -2
@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
    		}
    	}
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
It seems f2fs_may_inline_data() is breaking the atomic write case. Please fix.
Quoted 8 lines
Quoted 5 lines
sanity_check_inode() change only affect f2fs_iget(), during inode initialization, file should not be set as atomic one, right?
I didn't see any failure during 'f2fs_io write atomic_write' testcase... could you please provide me detail of the testcase?
I just applied this into my device and was getting lots of the below error messages resulting in open failures of database files.
Could you please help to apply below patch and dump the log?
From: Chao Yu <[email protected]>
Subject: [PATCH] f2fs: fix to do sanity check for inline inode
Signed-off-by Chao Yu <[email protected]>
---
   fs/f2fs/f2fs.h  |  7 +++++++
   fs/f2fs/inode.c | 11 +++++++----
   2 files changed, 14 insertions(+), 4 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 0f8c426aed50..13a9212d6cb6 100644
@@ -4159,6 +4159,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
    */
   static inline bool f2fs_post_read_required(struct inode *inode)
   {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
   	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
   		f2fs_compressed_file(inode);
   }
fs/f2fs/inode.c +0 -0
index 02630c17da93..a98614a24ad0 100644
@@ -276,11 +276,14 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
   		}
   	}
 
Quoted 15 lines
Quoted 5 lines
fs/f2fs/inode.c +1 -3
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {
   		set_sbi_flag(sbi, SBI_NEED_FSCK);
-		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 289
Quoted 10 lines
fs/f2fs/inode.c +6 -1
-			  __func__, inode->i_ino, inode->i_mode);
+		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) reason(%d, %llu, %ld, %d, %d, %lu) should not have inline_data, run fsck to fix",
+			  __func__, inode->i_ino, inode->i_mode,
+			  f2fs_is_atomic_file(inode),
+			  i_size_read(inode), MAX_INLINE_DATA(inode),
+			  file_is_encrypt(inode), file_is_verity(inode),
+			  F2FS_I(inode)->i_flags & F2FS_COMPR_FL);
   		return false;
   	}
 
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

maintainer

2022-05-13 21:07 UTC #8 lore raw

In reply to Chao Yu

Quoted 56 lines · Chao Yu

As Yanming reported in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=215895
I have encountered a bug in F2FS file system in kernel v5.17.
The kernel message is shown below:
kernel BUG at fs/inode.c:611!
Call Trace:
 evict+0x282/0x4e0
 __dentry_kill+0x2b2/0x4d0
 dput+0x2dd/0x720
 do_renameat2+0x596/0x970
 __x64_sys_rename+0x78/0x90
 do_syscall_64+0x3b/0x90
The root cause is: fuzzed inode has both inline_data flag and encrypted flag, so after it was deleted by rename(), during f2fs_evict_inode(), it will cause inline data conversion due to flags confilction, then page cache will be polluted and trigger panic in clear_inode().
This patch tries to fix the issue by do more sanity checks for inline data inode in sanity_check_inode().
Cc [email protected]
Reported-by Ming Yan <[email protected]>
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/f2fs.h  | 7 +++++++
 fs/f2fs/inode.c | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)
fs/f2fs/f2fs.h +7 -0
index 27aa93caec06..64c511b498cc 100644
@@ -4173,6 +4173,13 @@ static inline void f2fs_set_encrypted_inode(struct inode *inode)
  */
 static inline bool f2fs_post_read_required(struct inode *inode)
 {
+	/*
+	 * used by sanity_check_inode(), when disk layout fields has not
+	 * been synchronized to inmem fields.
+	 */
+	if (file_is_encrypt(inode) || file_is_verity(inode) ||
+			F2FS_I(inode)->i_flags & F2FS_COMPR_FL)
+		return true;
 	return f2fs_encrypted_file(inode) || fsverity_active(inode) ||
 		f2fs_compressed_file(inode);
 }
fs/f2fs/inode.c +0 -0
index 83639238a1fe..234b8ed02644 100644

fs/f2fs/inode.c static bool sanity_check_inode(struct inode *inode, struct page *node_page) · line 279

Quoted 6 lines

fs/f2fs/inode.c +1 -2

@@ -276,8 +276,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
 		}
 	}
 
-	if (f2fs_has_inline_data(inode) &&
-			(!S_ISREG(inode->i_mode) && !S_ISLNK(inode->i_mode))) {
+	if (f2fs_has_inline_data(inode) && !f2fs_may_inline_data(inode)) {

BTW, why can't we just check the above on-disk fields here only?

Quoted 5 lines

fs/f2fs/inode.c +0 -0

 		set_sbi_flag(sbi, SBI_NEED_FSCK);
 		f2fs_warn(sbi, "%s: inode (ino=%lx, mode=%u) should not have inline_data, run fsck to fix",
 			  __func__, inode->i_ino, inode->i_mode);

Signature · 1 line

2.25.1