[PATCH] f2fs: fix to do sanity check for inline inode

[PATCH] f2fs: fix to do sanity check for inline inode

[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: stable@vger.kernel.org
Reported-by: Ming Yan <yanming@tju.edu.cn>
Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
 fs/f2fs/f2fs.h  | 7 +++++++
 fs/f2fs/inode.c | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 27aa93caec06..64c511b498cc 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -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);
 }
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 83639238a1fe..234b8ed02644 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -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);
-- 
2.25.1

[f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[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: stable@vger.kernel.org
Reported-by: Ming Yan <yanming@tju.edu.cn>
Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
 fs/f2fs/f2fs.h  | 7 +++++++
 fs/f2fs/inode.c | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 27aa93caec06..64c511b498cc 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -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);
 }
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 83639238a1fe..234b8ed02644 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -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);
-- 
2.25.1



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

Re: [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 04/28, Chao Yu wrote:
> 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: stable@vger.kernel.org
> Reported-by: Ming Yan <yanming@tju.edu.cn>
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>  fs/f2fs/f2fs.h  | 7 +++++++
>  fs/f2fs/inode.c | 3 +--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 27aa93caec06..64c511b498cc 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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);
>  }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 83639238a1fe..234b8ed02644 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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.

>  		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);
> -- 
> 2.25.1

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 04/28, Chao Yu wrote:
> 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: stable@vger.kernel.org
> Reported-by: Ming Yan <yanming@tju.edu.cn>
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>  fs/f2fs/f2fs.h  | 7 +++++++
>  fs/f2fs/inode.c | 3 +--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 27aa93caec06..64c511b498cc 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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);
>  }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 83639238a1fe..234b8ed02644 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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.

>  		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);
> -- 
> 2.25.1


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

Re: [PATCH] f2fs: fix to do sanity check for inline inode

[Chao Yu]

On 2022/5/5 5:28, Jaegeuk Kim wrote:
> On 04/28, Chao Yu wrote:
>> 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: stable@vger.kernel.org
>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>>   fs/f2fs/f2fs.h  | 7 +++++++
>>   fs/f2fs/inode.c | 3 +--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 27aa93caec06..64c511b498cc 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -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);
>>   }
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index 83639238a1fe..234b8ed02644 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -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.

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,

> 
>>   		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);
>> -- 
>> 2.25.1

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Chao Yu]

On 2022/5/5 5:28, Jaegeuk Kim wrote:
> On 04/28, Chao Yu wrote:
>> 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: stable@vger.kernel.org
>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>>   fs/f2fs/f2fs.h  | 7 +++++++
>>   fs/f2fs/inode.c | 3 +--
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 27aa93caec06..64c511b498cc 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -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);
>>   }
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index 83639238a1fe..234b8ed02644 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -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.

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,

> 
>>   		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);
>> -- 
>> 2.25.1


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

Re: [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 05/05, Chao Yu wrote:
> On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > On 04/28, Chao Yu wrote:
> > > 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: stable@vger.kernel.org
> > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > >   fs/f2fs/f2fs.h  | 7 +++++++
> > >   fs/f2fs/inode.c | 3 +--
> > >   2 files changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 27aa93caec06..64c511b498cc 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -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);
> > >   }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 83639238a1fe..234b8ed02644 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -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.
> 
> 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.

> 
> Thanks,
> 
> > 
> > >   		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);
> > > -- 
> > > 2.25.1

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 05/05, Chao Yu wrote:
> On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > On 04/28, Chao Yu wrote:
> > > 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: stable@vger.kernel.org
> > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > >   fs/f2fs/f2fs.h  | 7 +++++++
> > >   fs/f2fs/inode.c | 3 +--
> > >   2 files changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 27aa93caec06..64c511b498cc 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -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);
> > >   }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 83639238a1fe..234b8ed02644 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -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.
> 
> 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.

> 
> Thanks,
> 
> > 
> > >   		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);
> > > -- 
> > > 2.25.1


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

Re: [PATCH] f2fs: fix to do sanity check for inline inode

[Chao Yu]

On 2022/5/5 11:31, Jaegeuk Kim wrote:
> On 05/05, Chao Yu wrote:
>> On 2022/5/5 5:28, Jaegeuk Kim wrote:
>>> On 04/28, Chao Yu wrote:
>>>> 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: stable@vger.kernel.org
>>>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>> ---
>>>>    fs/f2fs/f2fs.h  | 7 +++++++
>>>>    fs/f2fs/inode.c | 3 +--
>>>>    2 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 27aa93caec06..64c511b498cc 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -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);
>>>>    }
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index 83639238a1fe..234b8ed02644 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -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.
>>
>> 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 <chao@kernel.org>
Subject: [PATCH] f2fs: fix to do sanity check for inline inode

Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
  fs/f2fs/f2fs.h  |  7 +++++++
  fs/f2fs/inode.c | 11 +++++++----
  2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 0f8c426aed50..13a9212d6cb6 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -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);
  }
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 02630c17da93..a98614a24ad0 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -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;
  	}

-- 
2.32.0

> 
>>
>> Thanks,
>>
>>>
>>>>    		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);
>>>> -- 
>>>> 2.25.1

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Chao Yu]

On 2022/5/5 11:31, Jaegeuk Kim wrote:
> On 05/05, Chao Yu wrote:
>> On 2022/5/5 5:28, Jaegeuk Kim wrote:
>>> On 04/28, Chao Yu wrote:
>>>> 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: stable@vger.kernel.org
>>>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>> ---
>>>>    fs/f2fs/f2fs.h  | 7 +++++++
>>>>    fs/f2fs/inode.c | 3 +--
>>>>    2 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index 27aa93caec06..64c511b498cc 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -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);
>>>>    }
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index 83639238a1fe..234b8ed02644 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -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.
>>
>> 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 <chao@kernel.org>
Subject: [PATCH] f2fs: fix to do sanity check for inline inode

Signed-off-by: Chao Yu <chao.yu@oppo.com>
---
  fs/f2fs/f2fs.h  |  7 +++++++
  fs/f2fs/inode.c | 11 +++++++----
  2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 0f8c426aed50..13a9212d6cb6 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -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);
  }
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 02630c17da93..a98614a24ad0 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -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;
  	}

-- 
2.32.0

> 
>>
>> Thanks,
>>
>>>
>>>>    		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);
>>>> -- 
>>>> 2.25.1


_______________________________________________
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: fix to do sanity check for inline inode

[Chao Yu]

Ping,

On 2022/5/5 22:33, Chao Yu wrote:
> On 2022/5/5 11:31, Jaegeuk Kim wrote:
>> On 05/05, Chao Yu wrote:
>>> On 2022/5/5 5:28, Jaegeuk Kim wrote:
>>>> On 04/28, Chao Yu wrote:
>>>>> 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: stable@vger.kernel.org
>>>>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>>> ---
>>>>>     fs/f2fs/f2fs.h  | 7 +++++++
>>>>>     fs/f2fs/inode.c | 3 +--
>>>>>     2 files changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index 27aa93caec06..64c511b498cc 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -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);
>>>>>     }
>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>> index 83639238a1fe..234b8ed02644 100644
>>>>> --- a/fs/f2fs/inode.c
>>>>> +++ b/fs/f2fs/inode.c
>>>>> @@ -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.
>>>
>>> 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 <chao@kernel.org>
> Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> 
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>    fs/f2fs/f2fs.h  |  7 +++++++
>    fs/f2fs/inode.c | 11 +++++++----
>    2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 0f8c426aed50..13a9212d6cb6 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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);
>    }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 02630c17da93..a98614a24ad0 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
>    	}
> 

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Chao Yu]

Ping,

On 2022/5/5 22:33, Chao Yu wrote:
> On 2022/5/5 11:31, Jaegeuk Kim wrote:
>> On 05/05, Chao Yu wrote:
>>> On 2022/5/5 5:28, Jaegeuk Kim wrote:
>>>> On 04/28, Chao Yu wrote:
>>>>> 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: stable@vger.kernel.org
>>>>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>>>>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>>>>> ---
>>>>>     fs/f2fs/f2fs.h  | 7 +++++++
>>>>>     fs/f2fs/inode.c | 3 +--
>>>>>     2 files changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index 27aa93caec06..64c511b498cc 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -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);
>>>>>     }
>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>> index 83639238a1fe..234b8ed02644 100644
>>>>> --- a/fs/f2fs/inode.c
>>>>> +++ b/fs/f2fs/inode.c
>>>>> @@ -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.
>>>
>>> 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 <chao@kernel.org>
> Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> 
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>    fs/f2fs/f2fs.h  |  7 +++++++
>    fs/f2fs/inode.c | 11 +++++++----
>    2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 0f8c426aed50..13a9212d6cb6 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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);
>    }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 02630c17da93..a98614a24ad0 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
>    	}
> 


_______________________________________________
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: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 05/08, Chao Yu wrote:
> Ping,

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

> 
> On 2022/5/5 22:33, Chao Yu wrote:
> > On 2022/5/5 11:31, Jaegeuk Kim wrote:
> > > On 05/05, Chao Yu wrote:
> > > > On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > > > > On 04/28, Chao Yu wrote:
> > > > > > 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: stable@vger.kernel.org
> > > > > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > > > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > > > > ---
> > > > > >     fs/f2fs/f2fs.h  | 7 +++++++
> > > > > >     fs/f2fs/inode.c | 3 +--
> > > > > >     2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > index 27aa93caec06..64c511b498cc 100644
> > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > @@ -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);
> > > > > >     }
> > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > index 83639238a1fe..234b8ed02644 100644
> > > > > > --- a/fs/f2fs/inode.c
> > > > > > +++ b/fs/f2fs/inode.c
> > > > > > @@ -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.
> > > > 
> > > > 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 <chao@kernel.org>
> > Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> > 
> > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > ---
> >    fs/f2fs/f2fs.h  |  7 +++++++
> >    fs/f2fs/inode.c | 11 +++++++----
> >    2 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 0f8c426aed50..13a9212d6cb6 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -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);
> >    }
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 02630c17da93..a98614a24ad0 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -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;
> >    	}
> > 

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 05/08, Chao Yu wrote:
> Ping,

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

> 
> On 2022/5/5 22:33, Chao Yu wrote:
> > On 2022/5/5 11:31, Jaegeuk Kim wrote:
> > > On 05/05, Chao Yu wrote:
> > > > On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > > > > On 04/28, Chao Yu wrote:
> > > > > > 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: stable@vger.kernel.org
> > > > > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > > > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > > > > ---
> > > > > >     fs/f2fs/f2fs.h  | 7 +++++++
> > > > > >     fs/f2fs/inode.c | 3 +--
> > > > > >     2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > index 27aa93caec06..64c511b498cc 100644
> > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > @@ -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);
> > > > > >     }
> > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > index 83639238a1fe..234b8ed02644 100644
> > > > > > --- a/fs/f2fs/inode.c
> > > > > > +++ b/fs/f2fs/inode.c
> > > > > > @@ -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.
> > > > 
> > > > 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 <chao@kernel.org>
> > Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> > 
> > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > ---
> >    fs/f2fs/f2fs.h  |  7 +++++++
> >    fs/f2fs/inode.c | 11 +++++++----
> >    2 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 0f8c426aed50..13a9212d6cb6 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -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);
> >    }
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 02630c17da93..a98614a24ad0 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -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;
> >    	}
> > 


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

Re: [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 04/28, Chao Yu wrote:
> 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: stable@vger.kernel.org
> Reported-by: Ming Yan <yanming@tju.edu.cn>
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>  fs/f2fs/f2fs.h  | 7 +++++++
>  fs/f2fs/inode.c | 3 +--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 27aa93caec06..64c511b498cc 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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);
>  }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 83639238a1fe..234b8ed02644 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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?

>  		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);
> -- 
> 2.25.1

Re: [f2fs-dev] [PATCH] f2fs: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 04/28, Chao Yu wrote:
> 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: stable@vger.kernel.org
> Reported-by: Ming Yan <yanming@tju.edu.cn>
> Signed-off-by: Chao Yu <chao.yu@oppo.com>
> ---
>  fs/f2fs/f2fs.h  | 7 +++++++
>  fs/f2fs/inode.c | 3 +--
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 27aa93caec06..64c511b498cc 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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);
>  }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 83639238a1fe..234b8ed02644 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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?

>  		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);
> -- 
> 2.25.1


_______________________________________________
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: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 05/09, Jaegeuk Kim wrote:
> On 05/08, Chao Yu wrote:
> > 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.

> 
> > 
> > On 2022/5/5 22:33, Chao Yu wrote:
> > > On 2022/5/5 11:31, Jaegeuk Kim wrote:
> > > > On 05/05, Chao Yu wrote:
> > > > > On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > > > > > On 04/28, Chao Yu wrote:
> > > > > > > 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: stable@vger.kernel.org
> > > > > > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > > > > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > > > > > ---
> > > > > > >     fs/f2fs/f2fs.h  | 7 +++++++
> > > > > > >     fs/f2fs/inode.c | 3 +--
> > > > > > >     2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > index 27aa93caec06..64c511b498cc 100644
> > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > @@ -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);
> > > > > > >     }
> > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > index 83639238a1fe..234b8ed02644 100644
> > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > @@ -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.
> > > > > 
> > > > > 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 <chao@kernel.org>
> > > Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> > > 
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > >    fs/f2fs/f2fs.h  |  7 +++++++
> > >    fs/f2fs/inode.c | 11 +++++++----
> > >    2 files changed, 14 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 0f8c426aed50..13a9212d6cb6 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -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);
> > >    }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 02630c17da93..a98614a24ad0 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -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;
> > >    	}
> > > 
> 
> 
> _______________________________________________
> 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: fix to do sanity check for inline inode

[Jaegeuk Kim]

On 05/09, Jaegeuk Kim wrote:
> On 05/08, Chao Yu wrote:
> > 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.

> 
> > 
> > On 2022/5/5 22:33, Chao Yu wrote:
> > > On 2022/5/5 11:31, Jaegeuk Kim wrote:
> > > > On 05/05, Chao Yu wrote:
> > > > > On 2022/5/5 5:28, Jaegeuk Kim wrote:
> > > > > > On 04/28, Chao Yu wrote:
> > > > > > > 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: stable@vger.kernel.org
> > > > > > > Reported-by: Ming Yan <yanming@tju.edu.cn>
> > > > > > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > > > > > ---
> > > > > > >     fs/f2fs/f2fs.h  | 7 +++++++
> > > > > > >     fs/f2fs/inode.c | 3 +--
> > > > > > >     2 files changed, 8 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > index 27aa93caec06..64c511b498cc 100644
> > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > @@ -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);
> > > > > > >     }
> > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > index 83639238a1fe..234b8ed02644 100644
> > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > @@ -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.
> > > > > 
> > > > > 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 <chao@kernel.org>
> > > Subject: [PATCH] f2fs: fix to do sanity check for inline inode
> > > 
> > > Signed-off-by: Chao Yu <chao.yu@oppo.com>
> > > ---
> > >    fs/f2fs/f2fs.h  |  7 +++++++
> > >    fs/f2fs/inode.c | 11 +++++++----
> > >    2 files changed, 14 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 0f8c426aed50..13a9212d6cb6 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -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);
> > >    }
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index 02630c17da93..a98614a24ad0 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -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;
> > >    	}
> > > 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


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