* [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page()
@ 2024-05-06 10:33 Chao Yu
2024-05-06 10:33 ` [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() Chao Yu
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Chao Yu @ 2024-05-06 10:33 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel
It missed to call dec_valid_node_count() to release node block count
in error path, fix it.
Fixes: 141170b759e0 ("f2fs: fix to avoid use f2fs_bug_on() in f2fs_new_node_page()")
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/f2fs/node.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index b3de6d6cdb02..ae39971825bc 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1313,15 +1313,14 @@ struct page *f2fs_new_node_page(struct dnode_of_data *dn, unsigned int ofs)
#ifdef CONFIG_F2FS_CHECK_FS
err = f2fs_get_node_info(sbi, dn->nid, &new_ni, false);
- if (err) {
- dec_valid_node_count(sbi, dn->inode, !ofs);
- goto fail;
- }
+ if (err)
+ goto out_dec;
+
if (unlikely(new_ni.blk_addr != NULL_ADDR)) {
err = -EFSCORRUPTED;
set_sbi_flag(sbi, SBI_NEED_FSCK);
f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
- goto fail;
+ goto out_dec;
}
#endif
new_ni.nid = dn->nid;
@@ -1345,7 +1344,8 @@ struct page *f2fs_new_node_page(struct dnode_of_data *dn, unsigned int ofs)
if (ofs == 0)
inc_valid_inode_count(sbi);
return page;
-
+out_dec:
+ dec_valid_node_count(sbi, dn->inode, !ofs);
fail:
clear_node_page_dirty(page);
f2fs_put_page(page, 1);
--
2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment()
2024-05-06 10:33 [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() Chao Yu
@ 2024-05-06 10:33 ` Chao Yu
2024-05-09 0:46 ` Jaegeuk Kim
2024-05-06 10:33 ` [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode Chao Yu
2024-05-11 0:50 ` [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() patchwork-bot+f2fs
2 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-06 10:33 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel
During gc_data_segment(), if inode state is abnormal, it missed to call
iput(), fix it.
Fixes: 132e3209789c ("f2fs: remove false alarm on iget failure during GC")
Fixes: 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/f2fs/gc.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 8852814dab7f..e86c7f01539a 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1554,10 +1554,15 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
int err;
inode = f2fs_iget(sb, dni.ino);
- if (IS_ERR(inode) || is_bad_inode(inode) ||
- special_file(inode->i_mode))
+ if (IS_ERR(inode))
continue;
+ if (is_bad_inode(inode) ||
+ special_file(inode->i_mode)) {
+ iput(inode);
+ continue;
+ }
+
err = f2fs_gc_pinned_control(inode, gc_type, segno);
if (err == -EAGAIN) {
iput(inode);
--
2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-06 10:33 [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() Chao Yu
2024-05-06 10:33 ` [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() Chao Yu
@ 2024-05-06 10:33 ` Chao Yu
2024-05-09 15:52 ` Jaegeuk Kim
2024-05-11 0:50 ` [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() patchwork-bot+f2fs
2 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-06 10:33 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
syzbot reports a f2fs bug as below:
------------[ cut here ]------------
kernel BUG at fs/f2fs/inline.c:258!
CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
Call Trace:
f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
__f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
do_writepages+0x35b/0x870 mm/page-writeback.c:2612
__writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
wb_do_writeback fs/fs-writeback.c:2264 [inline]
wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
process_one_work kernel/workqueue.c:3254 [inline]
process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
kthread+0x2f2/0x390 kernel/kthread.c:388
ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
The root cause is: inline_data inode can be fuzzed, so that there may
be valid blkaddr in its direct node, once f2fs triggers background GC
to migrate the block, it will hit f2fs_bug_on() during dirty page
writeback.
Let's add sanity check on i_nid field for inline_data inode, meanwhile,
forbid to migrate inline_data inode's data block to fix this issue.
Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/f2fs/f2fs.h | 2 +-
fs/f2fs/gc.c | 6 ++++++
fs/f2fs/inline.c | 17 ++++++++++++++++-
fs/f2fs/inode.c | 2 +-
4 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index fced2b7652f4..c876813b5532 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
* inline.c
*/
bool f2fs_may_inline_data(struct inode *inode);
-bool f2fs_sanity_check_inline_data(struct inode *inode);
+bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
bool f2fs_may_inline_dentry(struct inode *inode);
void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
void f2fs_truncate_inline_inode(struct inode *inode,
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index e86c7f01539a..041957750478 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
continue;
}
+ if (f2fs_has_inline_data(inode)) {
+ iput(inode);
+ set_sbi_flag(sbi, SBI_NEED_FSCK);
+ continue;
+ }
+
err = f2fs_gc_pinned_control(inode, gc_type, segno);
if (err == -EAGAIN) {
iput(inode);
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index ac00423f117b..067600fed3d4 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
return !f2fs_post_read_required(inode);
}
-bool f2fs_sanity_check_inline_data(struct inode *inode)
+static bool has_node_blocks(struct inode *inode, struct page *ipage)
+{
+ struct f2fs_inode *ri = F2FS_INODE(ipage);
+ int i;
+
+ for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
+ if (ri->i_nid[i])
+ return true;
+ }
+ return false;
+}
+
+bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
{
if (!f2fs_has_inline_data(inode))
return false;
+ if (has_node_blocks(inode, ipage))
+ return false;
+
if (!support_inline_data(inode))
return true;
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index c26effdce9aa..1423cd27a477 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
}
}
- if (f2fs_sanity_check_inline_data(inode)) {
+ if (f2fs_sanity_check_inline_data(inode, node_page)) {
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);
return false;
--
2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment()
2024-05-06 10:33 ` [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() Chao Yu
@ 2024-05-09 0:46 ` Jaegeuk Kim
2024-05-09 2:49 ` Chao Yu
0 siblings, 1 reply; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-09 0:46 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 05/06, Chao Yu wrote:
> During gc_data_segment(), if inode state is abnormal, it missed to call
> iput(), fix it.
>
> Fixes: 132e3209789c ("f2fs: remove false alarm on iget failure during GC")
> Fixes: 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/f2fs/gc.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 8852814dab7f..e86c7f01539a 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1554,10 +1554,15 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> int err;
>
> inode = f2fs_iget(sb, dni.ino);
> - if (IS_ERR(inode) || is_bad_inode(inode) ||
> - special_file(inode->i_mode))
> + if (IS_ERR(inode))
> continue;
>
> + if (is_bad_inode(inode) ||
> + special_file(inode->i_mode)) {
> + iput(inode);
iget_failed() called iput()?
> + continue;
> + }
> +
> err = f2fs_gc_pinned_control(inode, gc_type, segno);
> if (err == -EAGAIN) {
> iput(inode);
> --
> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment()
2024-05-09 0:46 ` Jaegeuk Kim
@ 2024-05-09 2:49 ` Chao Yu
2024-05-09 15:50 ` Jaegeuk Kim
2024-05-10 3:36 ` Chao Yu
0 siblings, 2 replies; 19+ messages in thread
From: Chao Yu @ 2024-05-09 2:49 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2024/5/9 8:46, Jaegeuk Kim wrote:
> On 05/06, Chao Yu wrote:
>> During gc_data_segment(), if inode state is abnormal, it missed to call
>> iput(), fix it.
>>
>> Fixes: 132e3209789c ("f2fs: remove false alarm on iget failure during GC")
>> Fixes: 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>> fs/f2fs/gc.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 8852814dab7f..e86c7f01539a 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -1554,10 +1554,15 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>> int err;
>>
>> inode = f2fs_iget(sb, dni.ino);
>> - if (IS_ERR(inode) || is_bad_inode(inode) ||
>> - special_file(inode->i_mode))
>> + if (IS_ERR(inode))
>> continue;
>>
>> + if (is_bad_inode(inode) ||
>> + special_file(inode->i_mode)) {
>> + iput(inode);
>
> iget_failed() called iput()?
It looks the bad inode was referenced in this context, it needs to be iput()ed
here.
The bad inode was made in other thread, please check description in commit
b73e52824c89 ("f2fs: reposition unlock_new_inode to prevent accessing invalid
inode").
Thanks,
>
>
>> + continue;
>> + }
>> +
>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>> if (err == -EAGAIN) {
>> iput(inode);
>> --
>> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment()
2024-05-09 2:49 ` Chao Yu
@ 2024-05-09 15:50 ` Jaegeuk Kim
2024-05-10 3:36 ` Chao Yu
1 sibling, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-09 15:50 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 05/09, Chao Yu wrote:
> On 2024/5/9 8:46, Jaegeuk Kim wrote:
> > On 05/06, Chao Yu wrote:
> > > During gc_data_segment(), if inode state is abnormal, it missed to call
> > > iput(), fix it.
> > >
> > > Fixes: 132e3209789c ("f2fs: remove false alarm on iget failure during GC")
> > > Fixes: 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
> > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > ---
> > > fs/f2fs/gc.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > index 8852814dab7f..e86c7f01539a 100644
> > > --- a/fs/f2fs/gc.c
> > > +++ b/fs/f2fs/gc.c
> > > @@ -1554,10 +1554,15 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > > int err;
> > > inode = f2fs_iget(sb, dni.ino);
> > > - if (IS_ERR(inode) || is_bad_inode(inode) ||
> > > - special_file(inode->i_mode))
> > > + if (IS_ERR(inode))
> > > continue;
> > > + if (is_bad_inode(inode) ||
> > > + special_file(inode->i_mode)) {
> > > + iput(inode);
> >
> > iget_failed() called iput()?
>
> It looks the bad inode was referenced in this context, it needs to be iput()ed
> here.
>
> The bad inode was made in other thread, please check description in commit
> b73e52824c89 ("f2fs: reposition unlock_new_inode to prevent accessing invalid
> inode").
Ah, it's non-error case. Right.
>
> Thanks,
>
> >
> >
> > > + continue;
> > > + }
> > > +
> > > err = f2fs_gc_pinned_control(inode, gc_type, segno);
> > > if (err == -EAGAIN) {
> > > iput(inode);
> > > --
> > > 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-06 10:33 ` [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode Chao Yu
@ 2024-05-09 15:52 ` Jaegeuk Kim
2024-05-10 2:14 ` Chao Yu
0 siblings, 1 reply; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-09 15:52 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 05/06, Chao Yu wrote:
> syzbot reports a f2fs bug as below:
>
> ------------[ cut here ]------------
> kernel BUG at fs/f2fs/inline.c:258!
> CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
> RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
> Call Trace:
> f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
> f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
> __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
> f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
> do_writepages+0x35b/0x870 mm/page-writeback.c:2612
> __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
> writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
> wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
> wb_do_writeback fs/fs-writeback.c:2264 [inline]
> wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
> process_one_work kernel/workqueue.c:3254 [inline]
> process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
> worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
> kthread+0x2f2/0x390 kernel/kthread.c:388
> ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>
> The root cause is: inline_data inode can be fuzzed, so that there may
> be valid blkaddr in its direct node, once f2fs triggers background GC
> to migrate the block, it will hit f2fs_bug_on() during dirty page
> writeback.
>
> Let's add sanity check on i_nid field for inline_data inode, meanwhile,
> forbid to migrate inline_data inode's data block to fix this issue.
>
> Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/f2fs/f2fs.h | 2 +-
> fs/f2fs/gc.c | 6 ++++++
> fs/f2fs/inline.c | 17 ++++++++++++++++-
> fs/f2fs/inode.c | 2 +-
> 4 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index fced2b7652f4..c876813b5532 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
> * inline.c
> */
> bool f2fs_may_inline_data(struct inode *inode);
> -bool f2fs_sanity_check_inline_data(struct inode *inode);
> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
> bool f2fs_may_inline_dentry(struct inode *inode);
> void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
> void f2fs_truncate_inline_inode(struct inode *inode,
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index e86c7f01539a..041957750478 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> continue;
> }
>
> + if (f2fs_has_inline_data(inode)) {
> + iput(inode);
> + set_sbi_flag(sbi, SBI_NEED_FSCK);
> + continue;
Any race condtion to get this as false alarm?
> + }
> +
> err = f2fs_gc_pinned_control(inode, gc_type, segno);
> if (err == -EAGAIN) {
> iput(inode);
> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> index ac00423f117b..067600fed3d4 100644
> --- a/fs/f2fs/inline.c
> +++ b/fs/f2fs/inline.c
> @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
> return !f2fs_post_read_required(inode);
> }
>
> -bool f2fs_sanity_check_inline_data(struct inode *inode)
> +static bool has_node_blocks(struct inode *inode, struct page *ipage)
> +{
> + struct f2fs_inode *ri = F2FS_INODE(ipage);
> + int i;
> +
> + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
> + if (ri->i_nid[i])
> + return true;
> + }
> + return false;
> +}
> +
> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
> {
> if (!f2fs_has_inline_data(inode))
> return false;
>
> + if (has_node_blocks(inode, ipage))
> + return false;
> +
> if (!support_inline_data(inode))
> return true;
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index c26effdce9aa..1423cd27a477 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> }
> }
>
> - if (f2fs_sanity_check_inline_data(inode)) {
> + if (f2fs_sanity_check_inline_data(inode, node_page)) {
> 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);
> return false;
> --
> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-09 15:52 ` Jaegeuk Kim
@ 2024-05-10 2:14 ` Chao Yu
2024-05-10 3:36 ` Jaegeuk Kim
0 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-10 2:14 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 2024/5/9 23:52, Jaegeuk Kim wrote:
> On 05/06, Chao Yu wrote:
>> syzbot reports a f2fs bug as below:
>>
>> ------------[ cut here ]------------
>> kernel BUG at fs/f2fs/inline.c:258!
>> CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
>> RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
>> Call Trace:
>> f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
>> f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
>> __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
>> f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
>> do_writepages+0x35b/0x870 mm/page-writeback.c:2612
>> __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
>> writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
>> wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
>> wb_do_writeback fs/fs-writeback.c:2264 [inline]
>> wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
>> process_one_work kernel/workqueue.c:3254 [inline]
>> process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
>> worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
>> kthread+0x2f2/0x390 kernel/kthread.c:388
>> ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>>
>> The root cause is: inline_data inode can be fuzzed, so that there may
>> be valid blkaddr in its direct node, once f2fs triggers background GC
>> to migrate the block, it will hit f2fs_bug_on() during dirty page
>> writeback.
>>
>> Let's add sanity check on i_nid field for inline_data inode, meanwhile,
>> forbid to migrate inline_data inode's data block to fix this issue.
>>
>> Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
>> Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>> fs/f2fs/f2fs.h | 2 +-
>> fs/f2fs/gc.c | 6 ++++++
>> fs/f2fs/inline.c | 17 ++++++++++++++++-
>> fs/f2fs/inode.c | 2 +-
>> 4 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index fced2b7652f4..c876813b5532 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
>> * inline.c
>> */
>> bool f2fs_may_inline_data(struct inode *inode);
>> -bool f2fs_sanity_check_inline_data(struct inode *inode);
>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
>> bool f2fs_may_inline_dentry(struct inode *inode);
>> void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
>> void f2fs_truncate_inline_inode(struct inode *inode,
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index e86c7f01539a..041957750478 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>> continue;
>> }
>>
>> + if (f2fs_has_inline_data(inode)) {
>> + iput(inode);
>> + set_sbi_flag(sbi, SBI_NEED_FSCK);
>> + continue;
>
> Any race condtion to get this as false alarm?
Since there is no reproducer for the bug, I doubt it was caused by metadata
fuzzing, something like this:
- inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
- SIT/SSA entry of the block is valid;
- background GC migrates the block;
- kworker writeback it, and trigger the bug_on().
Thoughts?
Thanks,
>
>> + }
>> +
>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>> if (err == -EAGAIN) {
>> iput(inode);
>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>> index ac00423f117b..067600fed3d4 100644
>> --- a/fs/f2fs/inline.c
>> +++ b/fs/f2fs/inline.c
>> @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
>> return !f2fs_post_read_required(inode);
>> }
>>
>> -bool f2fs_sanity_check_inline_data(struct inode *inode)
>> +static bool has_node_blocks(struct inode *inode, struct page *ipage)
>> +{
>> + struct f2fs_inode *ri = F2FS_INODE(ipage);
>> + int i;
>> +
>> + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
>> + if (ri->i_nid[i])
>> + return true;
>> + }
>> + return false;
>> +}
>> +
>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
>> {
>> if (!f2fs_has_inline_data(inode))
>> return false;
>>
>> + if (has_node_blocks(inode, ipage))
>> + return false;
>> +
>> if (!support_inline_data(inode))
>> return true;
>>
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index c26effdce9aa..1423cd27a477 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>> }
>> }
>>
>> - if (f2fs_sanity_check_inline_data(inode)) {
>> + if (f2fs_sanity_check_inline_data(inode, node_page)) {
>> 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);
>> return false;
>> --
>> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment()
2024-05-09 2:49 ` Chao Yu
2024-05-09 15:50 ` Jaegeuk Kim
@ 2024-05-10 3:36 ` Chao Yu
1 sibling, 0 replies; 19+ messages in thread
From: Chao Yu @ 2024-05-10 3:36 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2024/5/9 10:49, Chao Yu wrote:
> On 2024/5/9 8:46, Jaegeuk Kim wrote:
>> On 05/06, Chao Yu wrote:
>>> During gc_data_segment(), if inode state is abnormal, it missed to call
>>> iput(), fix it.
>>>
>>> Fixes: 132e3209789c ("f2fs: remove false alarm on iget failure during GC")
Oh, this line should be replaced w/ below one, let me revise the patch.
Fixes: b73e52824c89 ("f2fs: reposition unlock_new_inode to prevent accessing invalid inode").
Thanks,
>>> Fixes: 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>> ---
>>> fs/f2fs/gc.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 8852814dab7f..e86c7f01539a 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -1554,10 +1554,15 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>> int err;
>>> inode = f2fs_iget(sb, dni.ino);
>>> - if (IS_ERR(inode) || is_bad_inode(inode) ||
>>> - special_file(inode->i_mode))
>>> + if (IS_ERR(inode))
>>> continue;
>>> + if (is_bad_inode(inode) ||
>>> + special_file(inode->i_mode)) {
>>> + iput(inode);
>>
>> iget_failed() called iput()?
>
> It looks the bad inode was referenced in this context, it needs to be iput()ed
> here.
>
> The bad inode was made in other thread, please check description in commit
> b73e52824c89 ("f2fs: reposition unlock_new_inode to prevent accessing invalid
> inode").
>
> Thanks,
>
>>
>>
>>> + continue;
>>> + }
>>> +
>>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>>> if (err == -EAGAIN) {
>>> iput(inode);
>>> --
>>> 2.40.1
>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-10 2:14 ` Chao Yu
@ 2024-05-10 3:36 ` Jaegeuk Kim
2024-05-10 14:16 ` Chao Yu
0 siblings, 1 reply; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-10 3:36 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 05/10, Chao Yu wrote:
> On 2024/5/9 23:52, Jaegeuk Kim wrote:
> > On 05/06, Chao Yu wrote:
> > > syzbot reports a f2fs bug as below:
> > >
> > > ------------[ cut here ]------------
> > > kernel BUG at fs/f2fs/inline.c:258!
> > > CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
> > > RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
> > > Call Trace:
> > > f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
> > > f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
> > > __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
> > > f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
> > > do_writepages+0x35b/0x870 mm/page-writeback.c:2612
> > > __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
> > > writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
> > > wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
> > > wb_do_writeback fs/fs-writeback.c:2264 [inline]
> > > wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
> > > process_one_work kernel/workqueue.c:3254 [inline]
> > > process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
> > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
> > > kthread+0x2f2/0x390 kernel/kthread.c:388
> > > ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
> > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> > >
> > > The root cause is: inline_data inode can be fuzzed, so that there may
> > > be valid blkaddr in its direct node, once f2fs triggers background GC
> > > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > > writeback.
> > >
> > > Let's add sanity check on i_nid field for inline_data inode, meanwhile,
> > > forbid to migrate inline_data inode's data block to fix this issue.
> > >
> > > Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
> > > Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
> > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > ---
> > > fs/f2fs/f2fs.h | 2 +-
> > > fs/f2fs/gc.c | 6 ++++++
> > > fs/f2fs/inline.c | 17 ++++++++++++++++-
> > > fs/f2fs/inode.c | 2 +-
> > > 4 files changed, 24 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index fced2b7652f4..c876813b5532 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
> > > * inline.c
> > > */
> > > bool f2fs_may_inline_data(struct inode *inode);
> > > -bool f2fs_sanity_check_inline_data(struct inode *inode);
> > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
> > > bool f2fs_may_inline_dentry(struct inode *inode);
> > > void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
> > > void f2fs_truncate_inline_inode(struct inode *inode,
> > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > index e86c7f01539a..041957750478 100644
> > > --- a/fs/f2fs/gc.c
> > > +++ b/fs/f2fs/gc.c
> > > @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > > continue;
> > > }
> > > + if (f2fs_has_inline_data(inode)) {
> > > + iput(inode);
> > > + set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > + continue;
> >
> > Any race condtion to get this as false alarm?
>
> Since there is no reproducer for the bug, I doubt it was caused by metadata
> fuzzing, something like this:
>
> - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
> - SIT/SSA entry of the block is valid;
> - background GC migrates the block;
> - kworker writeback it, and trigger the bug_on().
Wasn't detected by sanity_check_inode?
>
> Thoughts?
>
> Thanks,
>
> >
> > > + }
> > > +
> > > err = f2fs_gc_pinned_control(inode, gc_type, segno);
> > > if (err == -EAGAIN) {
> > > iput(inode);
> > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > > index ac00423f117b..067600fed3d4 100644
> > > --- a/fs/f2fs/inline.c
> > > +++ b/fs/f2fs/inline.c
> > > @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
> > > return !f2fs_post_read_required(inode);
> > > }
> > > -bool f2fs_sanity_check_inline_data(struct inode *inode)
> > > +static bool has_node_blocks(struct inode *inode, struct page *ipage)
> > > +{
> > > + struct f2fs_inode *ri = F2FS_INODE(ipage);
> > > + int i;
> > > +
> > > + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
> > > + if (ri->i_nid[i])
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
> > > {
> > > if (!f2fs_has_inline_data(inode))
> > > return false;
> > > + if (has_node_blocks(inode, ipage))
> > > + return false;
> > > +
> > > if (!support_inline_data(inode))
> > > return true;
> > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > index c26effdce9aa..1423cd27a477 100644
> > > --- a/fs/f2fs/inode.c
> > > +++ b/fs/f2fs/inode.c
> > > @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > }
> > > }
> > > - if (f2fs_sanity_check_inline_data(inode)) {
> > > + if (f2fs_sanity_check_inline_data(inode, node_page)) {
> > > 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);
> > > return false;
> > > --
> > > 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-10 3:36 ` Jaegeuk Kim
@ 2024-05-10 14:16 ` Chao Yu
2024-05-11 0:38 ` Jaegeuk Kim
0 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-10 14:16 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 2024/5/10 11:36, Jaegeuk Kim wrote:
> On 05/10, Chao Yu wrote:
>> On 2024/5/9 23:52, Jaegeuk Kim wrote:
>>> On 05/06, Chao Yu wrote:
>>>> syzbot reports a f2fs bug as below:
>>>>
>>>> ------------[ cut here ]------------
>>>> kernel BUG at fs/f2fs/inline.c:258!
>>>> CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
>>>> RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
>>>> Call Trace:
>>>> f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
>>>> f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
>>>> __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
>>>> f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
>>>> do_writepages+0x35b/0x870 mm/page-writeback.c:2612
>>>> __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
>>>> writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
>>>> wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
>>>> wb_do_writeback fs/fs-writeback.c:2264 [inline]
>>>> wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
>>>> process_one_work kernel/workqueue.c:3254 [inline]
>>>> process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
>>>> worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
>>>> kthread+0x2f2/0x390 kernel/kthread.c:388
>>>> ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>>>> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>>>>
>>>> The root cause is: inline_data inode can be fuzzed, so that there may
>>>> be valid blkaddr in its direct node, once f2fs triggers background GC
>>>> to migrate the block, it will hit f2fs_bug_on() during dirty page
>>>> writeback.
>>>>
>>>> Let's add sanity check on i_nid field for inline_data inode, meanwhile,
>>>> forbid to migrate inline_data inode's data block to fix this issue.
>>>>
>>>> Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
>>>> Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>> ---
>>>> fs/f2fs/f2fs.h | 2 +-
>>>> fs/f2fs/gc.c | 6 ++++++
>>>> fs/f2fs/inline.c | 17 ++++++++++++++++-
>>>> fs/f2fs/inode.c | 2 +-
>>>> 4 files changed, 24 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index fced2b7652f4..c876813b5532 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
>>>> * inline.c
>>>> */
>>>> bool f2fs_may_inline_data(struct inode *inode);
>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode);
>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
>>>> bool f2fs_may_inline_dentry(struct inode *inode);
>>>> void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
>>>> void f2fs_truncate_inline_inode(struct inode *inode,
>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>> index e86c7f01539a..041957750478 100644
>>>> --- a/fs/f2fs/gc.c
>>>> +++ b/fs/f2fs/gc.c
>>>> @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>> continue;
>>>> }
>>>> + if (f2fs_has_inline_data(inode)) {
>>>> + iput(inode);
>>>> + set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>> + continue;
>>>
>>> Any race condtion to get this as false alarm?
>>
>> Since there is no reproducer for the bug, I doubt it was caused by metadata
>> fuzzing, something like this:
>>
>> - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
>> - SIT/SSA entry of the block is valid;
>> - background GC migrates the block;
>> - kworker writeback it, and trigger the bug_on().
>
> Wasn't detected by sanity_check_inode?
I fuzzed non-inline inode w/ below metadata fields:
- i_blocks = 1
- i_size = 2048
- i_inline |= 0x02
sanity_check_inode() doesn't complain.
Thanks,
>
>>
>> Thoughts?
>>
>> Thanks,
>>
>>>
>>>> + }
>>>> +
>>>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>>>> if (err == -EAGAIN) {
>>>> iput(inode);
>>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>>> index ac00423f117b..067600fed3d4 100644
>>>> --- a/fs/f2fs/inline.c
>>>> +++ b/fs/f2fs/inline.c
>>>> @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
>>>> return !f2fs_post_read_required(inode);
>>>> }
>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode)
>>>> +static bool has_node_blocks(struct inode *inode, struct page *ipage)
>>>> +{
>>>> + struct f2fs_inode *ri = F2FS_INODE(ipage);
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
>>>> + if (ri->i_nid[i])
>>>> + return true;
>>>> + }
>>>> + return false;
>>>> +}
>>>> +
>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
>>>> {
>>>> if (!f2fs_has_inline_data(inode))
>>>> return false;
>>>> + if (has_node_blocks(inode, ipage))
>>>> + return false;
>>>> +
>>>> if (!support_inline_data(inode))
>>>> return true;
>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>> index c26effdce9aa..1423cd27a477 100644
>>>> --- a/fs/f2fs/inode.c
>>>> +++ b/fs/f2fs/inode.c
>>>> @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>>> }
>>>> }
>>>> - if (f2fs_sanity_check_inline_data(inode)) {
>>>> + if (f2fs_sanity_check_inline_data(inode, node_page)) {
>>>> 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);
>>>> return false;
>>>> --
>>>> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-10 14:16 ` Chao Yu
@ 2024-05-11 0:38 ` Jaegeuk Kim
2024-05-11 3:07 ` Chao Yu
0 siblings, 1 reply; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-11 0:38 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 05/10, Chao Yu wrote:
> On 2024/5/10 11:36, Jaegeuk Kim wrote:
> > On 05/10, Chao Yu wrote:
> > > On 2024/5/9 23:52, Jaegeuk Kim wrote:
> > > > On 05/06, Chao Yu wrote:
> > > > > syzbot reports a f2fs bug as below:
> > > > >
> > > > > ------------[ cut here ]------------
> > > > > kernel BUG at fs/f2fs/inline.c:258!
> > > > > CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
> > > > > RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
> > > > > Call Trace:
> > > > > f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
> > > > > f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
> > > > > __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
> > > > > f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
> > > > > do_writepages+0x35b/0x870 mm/page-writeback.c:2612
> > > > > __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
> > > > > writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
> > > > > wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
> > > > > wb_do_writeback fs/fs-writeback.c:2264 [inline]
> > > > > wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
> > > > > process_one_work kernel/workqueue.c:3254 [inline]
> > > > > process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
> > > > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
> > > > > kthread+0x2f2/0x390 kernel/kthread.c:388
> > > > > ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
> > > > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> > > > >
> > > > > The root cause is: inline_data inode can be fuzzed, so that there may
> > > > > be valid blkaddr in its direct node, once f2fs triggers background GC
> > > > > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > > > > writeback.
> > > > >
> > > > > Let's add sanity check on i_nid field for inline_data inode, meanwhile,
> > > > > forbid to migrate inline_data inode's data block to fix this issue.
> > > > >
> > > > > Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
> > > > > Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
> > > > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > > > ---
> > > > > fs/f2fs/f2fs.h | 2 +-
> > > > > fs/f2fs/gc.c | 6 ++++++
> > > > > fs/f2fs/inline.c | 17 ++++++++++++++++-
> > > > > fs/f2fs/inode.c | 2 +-
> > > > > 4 files changed, 24 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > index fced2b7652f4..c876813b5532 100644
> > > > > --- a/fs/f2fs/f2fs.h
> > > > > +++ b/fs/f2fs/f2fs.h
> > > > > @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
> > > > > * inline.c
> > > > > */
> > > > > bool f2fs_may_inline_data(struct inode *inode);
> > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode);
> > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
> > > > > bool f2fs_may_inline_dentry(struct inode *inode);
> > > > > void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
> > > > > void f2fs_truncate_inline_inode(struct inode *inode,
> > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > > index e86c7f01539a..041957750478 100644
> > > > > --- a/fs/f2fs/gc.c
> > > > > +++ b/fs/f2fs/gc.c
> > > > > @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > > > > continue;
> > > > > }
> > > > > + if (f2fs_has_inline_data(inode)) {
> > > > > + iput(inode);
> > > > > + set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > > > + continue;
> > > >
> > > > Any race condtion to get this as false alarm?
> > >
> > > Since there is no reproducer for the bug, I doubt it was caused by metadata
> > > fuzzing, something like this:
> > >
> > > - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
> > > - SIT/SSA entry of the block is valid;
> > > - background GC migrates the block;
> > > - kworker writeback it, and trigger the bug_on().
> >
> > Wasn't detected by sanity_check_inode?
>
> I fuzzed non-inline inode w/ below metadata fields:
> - i_blocks = 1
> - i_size = 2048
> - i_inline |= 0x02
>
> sanity_check_inode() doesn't complain.
I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
whether we really need to check it in the gc path.
>
> Thanks,
>
> >
> > >
> > > Thoughts?
> > >
> > > Thanks,
> > >
> > > >
> > > > > + }
> > > > > +
> > > > > err = f2fs_gc_pinned_control(inode, gc_type, segno);
> > > > > if (err == -EAGAIN) {
> > > > > iput(inode);
> > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > > > > index ac00423f117b..067600fed3d4 100644
> > > > > --- a/fs/f2fs/inline.c
> > > > > +++ b/fs/f2fs/inline.c
> > > > > @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
> > > > > return !f2fs_post_read_required(inode);
> > > > > }
> > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode)
> > > > > +static bool has_node_blocks(struct inode *inode, struct page *ipage)
> > > > > +{
> > > > > + struct f2fs_inode *ri = F2FS_INODE(ipage);
> > > > > + int i;
> > > > > +
> > > > > + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
> > > > > + if (ri->i_nid[i])
> > > > > + return true;
> > > > > + }
> > > > > + return false;
> > > > > +}
> > > > > +
> > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
> > > > > {
> > > > > if (!f2fs_has_inline_data(inode))
> > > > > return false;
> > > > > + if (has_node_blocks(inode, ipage))
> > > > > + return false;
> > > > > +
> > > > > if (!support_inline_data(inode))
> > > > > return true;
> > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > index c26effdce9aa..1423cd27a477 100644
> > > > > --- a/fs/f2fs/inode.c
> > > > > +++ b/fs/f2fs/inode.c
> > > > > @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > > > }
> > > > > }
> > > > > - if (f2fs_sanity_check_inline_data(inode)) {
> > > > > + if (f2fs_sanity_check_inline_data(inode, node_page)) {
> > > > > 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);
> > > > > return false;
> > > > > --
> > > > > 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page()
2024-05-06 10:33 [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() Chao Yu
2024-05-06 10:33 ` [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() Chao Yu
2024-05-06 10:33 ` [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode Chao Yu
@ 2024-05-11 0:50 ` patchwork-bot+f2fs
2 siblings, 0 replies; 19+ messages in thread
From: patchwork-bot+f2fs @ 2024-05-11 0:50 UTC (permalink / raw)
To: Chao Yu; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel
Hello:
This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:
On Mon, 6 May 2024 18:33:11 +0800 you wrote:
> It missed to call dec_valid_node_count() to release node block count
> in error path, fix it.
>
> Fixes: 141170b759e0 ("f2fs: fix to avoid use f2fs_bug_on() in f2fs_new_node_page()")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/f2fs/node.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Here is the summary with links:
- [f2fs-dev,1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page()
(no matching commit)
- [f2fs-dev,2/3] f2fs: fix to add missing iput() in gc_data_segment()
https://git.kernel.org/jaegeuk/f2fs/c/a798ff17cd2d
- [f2fs-dev,3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
(no matching commit)
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-11 0:38 ` Jaegeuk Kim
@ 2024-05-11 3:07 ` Chao Yu
2024-05-14 16:07 ` Jaegeuk Kim
0 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-11 3:07 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 2024/5/11 8:38, Jaegeuk Kim wrote:
> On 05/10, Chao Yu wrote:
>> On 2024/5/10 11:36, Jaegeuk Kim wrote:
>>> On 05/10, Chao Yu wrote:
>>>> On 2024/5/9 23:52, Jaegeuk Kim wrote:
>>>>> On 05/06, Chao Yu wrote:
>>>>>> syzbot reports a f2fs bug as below:
>>>>>>
>>>>>> ------------[ cut here ]------------
>>>>>> kernel BUG at fs/f2fs/inline.c:258!
>>>>>> CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
>>>>>> RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
>>>>>> Call Trace:
>>>>>> f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
>>>>>> f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
>>>>>> __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
>>>>>> f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
>>>>>> do_writepages+0x35b/0x870 mm/page-writeback.c:2612
>>>>>> __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
>>>>>> writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
>>>>>> wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
>>>>>> wb_do_writeback fs/fs-writeback.c:2264 [inline]
>>>>>> wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
>>>>>> process_one_work kernel/workqueue.c:3254 [inline]
>>>>>> process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
>>>>>> worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
>>>>>> kthread+0x2f2/0x390 kernel/kthread.c:388
>>>>>> ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>>>>>> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>>>>>>
>>>>>> The root cause is: inline_data inode can be fuzzed, so that there may
>>>>>> be valid blkaddr in its direct node, once f2fs triggers background GC
>>>>>> to migrate the block, it will hit f2fs_bug_on() during dirty page
>>>>>> writeback.
>>>>>>
>>>>>> Let's add sanity check on i_nid field for inline_data inode, meanwhile,
>>>>>> forbid to migrate inline_data inode's data block to fix this issue.
>>>>>>
>>>>>> Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
>>>>>> Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
>>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>>> ---
>>>>>> fs/f2fs/f2fs.h | 2 +-
>>>>>> fs/f2fs/gc.c | 6 ++++++
>>>>>> fs/f2fs/inline.c | 17 ++++++++++++++++-
>>>>>> fs/f2fs/inode.c | 2 +-
>>>>>> 4 files changed, 24 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>> index fced2b7652f4..c876813b5532 100644
>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>> @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
>>>>>> * inline.c
>>>>>> */
>>>>>> bool f2fs_may_inline_data(struct inode *inode);
>>>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode);
>>>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
>>>>>> bool f2fs_may_inline_dentry(struct inode *inode);
>>>>>> void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
>>>>>> void f2fs_truncate_inline_inode(struct inode *inode,
>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>> index e86c7f01539a..041957750478 100644
>>>>>> --- a/fs/f2fs/gc.c
>>>>>> +++ b/fs/f2fs/gc.c
>>>>>> @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>>>> continue;
>>>>>> }
>>>>>> + if (f2fs_has_inline_data(inode)) {
>>>>>> + iput(inode);
>>>>>> + set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>>> + continue;
>>>>>
>>>>> Any race condtion to get this as false alarm?
>>>>
>>>> Since there is no reproducer for the bug, I doubt it was caused by metadata
>>>> fuzzing, something like this:
>>>>
>>>> - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
>>>> - SIT/SSA entry of the block is valid;
>>>> - background GC migrates the block;
>>>> - kworker writeback it, and trigger the bug_on().
>>>
>>> Wasn't detected by sanity_check_inode?
>>
>> I fuzzed non-inline inode w/ below metadata fields:
>> - i_blocks = 1
>> - i_size = 2048
>> - i_inline |= 0x02
>>
>> sanity_check_inode() doesn't complain.
>
> I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
I didn't figure out a generic way in sanity_check_inode() to catch all fuzzed cases.
e.g.
case #1
- blkaddr, its dnode, SSA and SIT are consistent
- dnode.footer.ino points to inline inode
- inline inode doesn't link to the donde
Something like fuzzed special file, please check details in below commit:
9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
case #2
- blkaddr, its dnode, SSA and SIT are consistent
- blkaddr locates in inline inode's i_addr
Thanks,
> whether we really need to check it in the gc path.
>
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thoughts?
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>> + }
>>>>>> +
>>>>>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>>>>>> if (err == -EAGAIN) {
>>>>>> iput(inode);
>>>>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>>>>> index ac00423f117b..067600fed3d4 100644
>>>>>> --- a/fs/f2fs/inline.c
>>>>>> +++ b/fs/f2fs/inline.c
>>>>>> @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
>>>>>> return !f2fs_post_read_required(inode);
>>>>>> }
>>>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode)
>>>>>> +static bool has_node_blocks(struct inode *inode, struct page *ipage)
>>>>>> +{
>>>>>> + struct f2fs_inode *ri = F2FS_INODE(ipage);
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
>>>>>> + if (ri->i_nid[i])
>>>>>> + return true;
>>>>>> + }
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
>>>>>> {
>>>>>> if (!f2fs_has_inline_data(inode))
>>>>>> return false;
>>>>>> + if (has_node_blocks(inode, ipage))
>>>>>> + return false;
>>>>>> +
>>>>>> if (!support_inline_data(inode))
>>>>>> return true;
>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>> index c26effdce9aa..1423cd27a477 100644
>>>>>> --- a/fs/f2fs/inode.c
>>>>>> +++ b/fs/f2fs/inode.c
>>>>>> @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>>>>> }
>>>>>> }
>>>>>> - if (f2fs_sanity_check_inline_data(inode)) {
>>>>>> + if (f2fs_sanity_check_inline_data(inode, node_page)) {
>>>>>> 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);
>>>>>> return false;
>>>>>> --
>>>>>> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-11 3:07 ` Chao Yu
@ 2024-05-14 16:07 ` Jaegeuk Kim
2024-05-15 1:34 ` Chao Yu
0 siblings, 1 reply; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-14 16:07 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 05/11, Chao Yu wrote:
> On 2024/5/11 8:38, Jaegeuk Kim wrote:
> > On 05/10, Chao Yu wrote:
> > > On 2024/5/10 11:36, Jaegeuk Kim wrote:
> > > > On 05/10, Chao Yu wrote:
> > > > > On 2024/5/9 23:52, Jaegeuk Kim wrote:
> > > > > > On 05/06, Chao Yu wrote:
> > > > > > > syzbot reports a f2fs bug as below:
> > > > > > >
> > > > > > > ------------[ cut here ]------------
> > > > > > > kernel BUG at fs/f2fs/inline.c:258!
> > > > > > > CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
> > > > > > > RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
> > > > > > > Call Trace:
> > > > > > > f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
> > > > > > > f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
> > > > > > > __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
> > > > > > > f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
> > > > > > > do_writepages+0x35b/0x870 mm/page-writeback.c:2612
> > > > > > > __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
> > > > > > > writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
> > > > > > > wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
> > > > > > > wb_do_writeback fs/fs-writeback.c:2264 [inline]
> > > > > > > wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
> > > > > > > process_one_work kernel/workqueue.c:3254 [inline]
> > > > > > > process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
> > > > > > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
> > > > > > > kthread+0x2f2/0x390 kernel/kthread.c:388
> > > > > > > ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
> > > > > > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> > > > > > >
> > > > > > > The root cause is: inline_data inode can be fuzzed, so that there may
> > > > > > > be valid blkaddr in its direct node, once f2fs triggers background GC
> > > > > > > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > > > > > > writeback.
> > > > > > >
> > > > > > > Let's add sanity check on i_nid field for inline_data inode, meanwhile,
> > > > > > > forbid to migrate inline_data inode's data block to fix this issue.
> > > > > > >
> > > > > > > Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
> > > > > > > Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
> > > > > > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > > > > > ---
> > > > > > > fs/f2fs/f2fs.h | 2 +-
> > > > > > > fs/f2fs/gc.c | 6 ++++++
> > > > > > > fs/f2fs/inline.c | 17 ++++++++++++++++-
> > > > > > > fs/f2fs/inode.c | 2 +-
> > > > > > > 4 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > index fced2b7652f4..c876813b5532 100644
> > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
> > > > > > > * inline.c
> > > > > > > */
> > > > > > > bool f2fs_may_inline_data(struct inode *inode);
> > > > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode);
> > > > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
> > > > > > > bool f2fs_may_inline_dentry(struct inode *inode);
> > > > > > > void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
> > > > > > > void f2fs_truncate_inline_inode(struct inode *inode,
> > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > > > > index e86c7f01539a..041957750478 100644
> > > > > > > --- a/fs/f2fs/gc.c
> > > > > > > +++ b/fs/f2fs/gc.c
> > > > > > > @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > > > > > > continue;
> > > > > > > }
> > > > > > > + if (f2fs_has_inline_data(inode)) {
> > > > > > > + iput(inode);
> > > > > > > + set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > > > > > + continue;
> > > > > >
> > > > > > Any race condtion to get this as false alarm?
> > > > >
> > > > > Since there is no reproducer for the bug, I doubt it was caused by metadata
> > > > > fuzzing, something like this:
> > > > >
> > > > > - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
> > > > > - SIT/SSA entry of the block is valid;
> > > > > - background GC migrates the block;
> > > > > - kworker writeback it, and trigger the bug_on().
> > > >
> > > > Wasn't detected by sanity_check_inode?
> > >
> > > I fuzzed non-inline inode w/ below metadata fields:
> > > - i_blocks = 1
> > > - i_size = 2048
> > > - i_inline |= 0x02
> > >
> > > sanity_check_inode() doesn't complain.
> >
> > I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
>
> I didn't figure out a generic way in sanity_check_inode() to catch all fuzzed cases.
The patch described:
"The root cause is: inline_data inode can be fuzzed, so that there may
be valid blkaddr in its direct node, once f2fs triggers background GC
to migrate the block, it will hit f2fs_bug_on() during dirty page
writeback."
Do you suspect the node block address was suddenly assigned after f2fs_iget()?
Otherwise, it looks checking them in sanity_check_inode would be enough.
>
> e.g.
> case #1
> - blkaddr, its dnode, SSA and SIT are consistent
> - dnode.footer.ino points to inline inode
> - inline inode doesn't link to the donde
>
> Something like fuzzed special file, please check details in below commit:
>
> 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
>
> case #2
> - blkaddr, its dnode, SSA and SIT are consistent
> - blkaddr locates in inline inode's i_addr
>
> Thanks,
>
> > whether we really need to check it in the gc path.
> >
> > >
> > > Thanks,
> > >
> > > >
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > Thanks,
> > > > >
> > > > > >
> > > > > > > + }
> > > > > > > +
> > > > > > > err = f2fs_gc_pinned_control(inode, gc_type, segno);
> > > > > > > if (err == -EAGAIN) {
> > > > > > > iput(inode);
> > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > > > > > > index ac00423f117b..067600fed3d4 100644
> > > > > > > --- a/fs/f2fs/inline.c
> > > > > > > +++ b/fs/f2fs/inline.c
> > > > > > > @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
> > > > > > > return !f2fs_post_read_required(inode);
> > > > > > > }
> > > > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode)
> > > > > > > +static bool has_node_blocks(struct inode *inode, struct page *ipage)
> > > > > > > +{
> > > > > > > + struct f2fs_inode *ri = F2FS_INODE(ipage);
> > > > > > > + int i;
> > > > > > > +
> > > > > > > + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
> > > > > > > + if (ri->i_nid[i])
> > > > > > > + return true;
> > > > > > > + }
> > > > > > > + return false;
> > > > > > > +}
> > > > > > > +
> > > > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
> > > > > > > {
> > > > > > > if (!f2fs_has_inline_data(inode))
> > > > > > > return false;
> > > > > > > + if (has_node_blocks(inode, ipage))
> > > > > > > + return false;
> > > > > > > +
> > > > > > > if (!support_inline_data(inode))
> > > > > > > return true;
> > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > index c26effdce9aa..1423cd27a477 100644
> > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > > > > > }
> > > > > > > }
> > > > > > > - if (f2fs_sanity_check_inline_data(inode)) {
> > > > > > > + if (f2fs_sanity_check_inline_data(inode, node_page)) {
> > > > > > > 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);
> > > > > > > return false;
> > > > > > > --
> > > > > > > 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-14 16:07 ` Jaegeuk Kim
@ 2024-05-15 1:34 ` Chao Yu
2024-05-15 4:39 ` Jaegeuk Kim
0 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-15 1:34 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 2024/5/15 0:07, Jaegeuk Kim wrote:
> 外部邮件/External Mail
>
>
> On 05/11, Chao Yu wrote:
>> On 2024/5/11 8:38, Jaegeuk Kim wrote:
>>> On 05/10, Chao Yu wrote:
>>>> On 2024/5/10 11:36, Jaegeuk Kim wrote:
>>>>> On 05/10, Chao Yu wrote:
>>>>>> On 2024/5/9 23:52, Jaegeuk Kim wrote:
>>>>>>> On 05/06, Chao Yu wrote:
>>>>>>>> syzbot reports a f2fs bug as below:
>>>>>>>>
>>>>>>>> ------------[ cut here ]------------
>>>>>>>> kernel BUG at fs/f2fs/inline.c:258!
>>>>>>>> CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
>>>>>>>> RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
>>>>>>>> Call Trace:
>>>>>>>> f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
>>>>>>>> f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
>>>>>>>> __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
>>>>>>>> f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
>>>>>>>> do_writepages+0x35b/0x870 mm/page-writeback.c:2612
>>>>>>>> __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
>>>>>>>> writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
>>>>>>>> wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
>>>>>>>> wb_do_writeback fs/fs-writeback.c:2264 [inline]
>>>>>>>> wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
>>>>>>>> process_one_work kernel/workqueue.c:3254 [inline]
>>>>>>>> process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
>>>>>>>> worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
>>>>>>>> kthread+0x2f2/0x390 kernel/kthread.c:388
>>>>>>>> ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>>>>>>>> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>>>>>>>>
>>>>>>>> The root cause is: inline_data inode can be fuzzed, so that there may
>>>>>>>> be valid blkaddr in its direct node, once f2fs triggers background GC
>>>>>>>> to migrate the block, it will hit f2fs_bug_on() during dirty page
>>>>>>>> writeback.
>>>>>>>>
>>>>>>>> Let's add sanity check on i_nid field for inline_data inode, meanwhile,
>>>>>>>> forbid to migrate inline_data inode's data block to fix this issue.
>>>>>>>>
>>>>>>>> Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
>>>>>>>> Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
>>>>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>>>>> ---
>>>>>>>> fs/f2fs/f2fs.h | 2 +-
>>>>>>>> fs/f2fs/gc.c | 6 ++++++
>>>>>>>> fs/f2fs/inline.c | 17 ++++++++++++++++-
>>>>>>>> fs/f2fs/inode.c | 2 +-
>>>>>>>> 4 files changed, 24 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>>> index fced2b7652f4..c876813b5532 100644
>>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>>> @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
>>>>>>>> * inline.c
>>>>>>>> */
>>>>>>>> bool f2fs_may_inline_data(struct inode *inode);
>>>>>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode);
>>>>>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
>>>>>>>> bool f2fs_may_inline_dentry(struct inode *inode);
>>>>>>>> void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
>>>>>>>> void f2fs_truncate_inline_inode(struct inode *inode,
>>>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>>>> index e86c7f01539a..041957750478 100644
>>>>>>>> --- a/fs/f2fs/gc.c
>>>>>>>> +++ b/fs/f2fs/gc.c
>>>>>>>> @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>>>>>> continue;
>>>>>>>> }
>>>>>>>> + if (f2fs_has_inline_data(inode)) {
>>>>>>>> + iput(inode);
>>>>>>>> + set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>>>>> + continue;
>>>>>>>
>>>>>>> Any race condtion to get this as false alarm?
>>>>>>
>>>>>> Since there is no reproducer for the bug, I doubt it was caused by metadata
>>>>>> fuzzing, something like this:
>>>>>>
>>>>>> - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
>>>>>> - SIT/SSA entry of the block is valid;
>>>>>> - background GC migrates the block;
>>>>>> - kworker writeback it, and trigger the bug_on().
>>>>>
>>>>> Wasn't detected by sanity_check_inode?
>>>>
>>>> I fuzzed non-inline inode w/ below metadata fields:
>>>> - i_blocks = 1
>>>> - i_size = 2048
>>>> - i_inline |= 0x02
>>>>
>>>> sanity_check_inode() doesn't complain.
>>>
>>> I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
>>
>> I didn't figure out a generic way in sanity_check_inode() to catch all fuzzed cases.
>
>
> The patch described:
> "The root cause is: inline_data inode can be fuzzed, so that there may
> be valid blkaddr in its direct node, once f2fs triggers background GC
> to migrate the block, it will hit f2fs_bug_on() during dirty page
> writeback."
>
> Do you suspect the node block address was suddenly assigned after f2fs_iget()?
No, I suspect that the image was fuzzed by tools offline, not in runtime after
mount().
> Otherwise, it looks checking them in sanity_check_inode would be enough.
>
>>
>> e.g.
>> case #1
>> - blkaddr, its dnode, SSA and SIT are consistent
>> - dnode.footer.ino points to inline inode
>> - inline inode doesn't link to the donde
>>
>> Something like fuzzed special file, please check details in below commit:
>>
>> 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
>>
>> case #2
>> - blkaddr, its dnode, SSA and SIT are consistent
>> - blkaddr locates in inline inode's i_addr
The image status is something like above as I described.
Thanks,
>>
>> Thanks,
>>
>>> whether we really need to check it in the gc path.
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>>>>>>>> if (err == -EAGAIN) {
>>>>>>>> iput(inode);
>>>>>>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>>>>>>> index ac00423f117b..067600fed3d4 100644
>>>>>>>> --- a/fs/f2fs/inline.c
>>>>>>>> +++ b/fs/f2fs/inline.c
>>>>>>>> @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
>>>>>>>> return !f2fs_post_read_required(inode);
>>>>>>>> }
>>>>>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode)
>>>>>>>> +static bool has_node_blocks(struct inode *inode, struct page *ipage)
>>>>>>>> +{
>>>>>>>> + struct f2fs_inode *ri = F2FS_INODE(ipage);
>>>>>>>> + int i;
>>>>>>>> +
>>>>>>>> + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
>>>>>>>> + if (ri->i_nid[i])
>>>>>>>> + return true;
>>>>>>>> + }
>>>>>>>> + return false;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
>>>>>>>> {
>>>>>>>> if (!f2fs_has_inline_data(inode))
>>>>>>>> return false;
>>>>>>>> + if (has_node_blocks(inode, ipage))
>>>>>>>> + return false;
>>>>>>>> +
>>>>>>>> if (!support_inline_data(inode))
>>>>>>>> return true;
>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>>> index c26effdce9aa..1423cd27a477 100644
>>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>>> @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>>>>>>> }
>>>>>>>> }
>>>>>>>> - if (f2fs_sanity_check_inline_data(inode)) {
>>>>>>>> + if (f2fs_sanity_check_inline_data(inode, node_page)) {
>>>>>>>> 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);
>>>>>>>> return false;
>>>>>>>> --
>>>>>>>> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-15 1:34 ` Chao Yu
@ 2024-05-15 4:39 ` Jaegeuk Kim
2024-05-15 6:12 ` Chao Yu
0 siblings, 1 reply; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-15 4:39 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 05/15, Chao Yu wrote:
> On 2024/5/15 0:07, Jaegeuk Kim wrote:
> > 外部邮件/External Mail
> >
> >
> > On 05/11, Chao Yu wrote:
> > > On 2024/5/11 8:38, Jaegeuk Kim wrote:
> > > > On 05/10, Chao Yu wrote:
> > > > > On 2024/5/10 11:36, Jaegeuk Kim wrote:
> > > > > > On 05/10, Chao Yu wrote:
> > > > > > > On 2024/5/9 23:52, Jaegeuk Kim wrote:
> > > > > > > > On 05/06, Chao Yu wrote:
> > > > > > > > > syzbot reports a f2fs bug as below:
> > > > > > > > >
> > > > > > > > > ------------[ cut here ]------------
> > > > > > > > > kernel BUG at fs/f2fs/inline.c:258!
> > > > > > > > > CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
> > > > > > > > > RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
> > > > > > > > > Call Trace:
> > > > > > > > > f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
> > > > > > > > > f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
> > > > > > > > > __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
> > > > > > > > > f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
> > > > > > > > > do_writepages+0x35b/0x870 mm/page-writeback.c:2612
> > > > > > > > > __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
> > > > > > > > > writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
> > > > > > > > > wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
> > > > > > > > > wb_do_writeback fs/fs-writeback.c:2264 [inline]
> > > > > > > > > wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
> > > > > > > > > process_one_work kernel/workqueue.c:3254 [inline]
> > > > > > > > > process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
> > > > > > > > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
> > > > > > > > > kthread+0x2f2/0x390 kernel/kthread.c:388
> > > > > > > > > ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
> > > > > > > > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> > > > > > > > >
> > > > > > > > > The root cause is: inline_data inode can be fuzzed, so that there may
> > > > > > > > > be valid blkaddr in its direct node, once f2fs triggers background GC
> > > > > > > > > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > > > > > > > > writeback.
> > > > > > > > >
> > > > > > > > > Let's add sanity check on i_nid field for inline_data inode, meanwhile,
> > > > > > > > > forbid to migrate inline_data inode's data block to fix this issue.
> > > > > > > > >
> > > > > > > > > Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
> > > > > > > > > Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
> > > > > > > > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > > > > > > > ---
> > > > > > > > > fs/f2fs/f2fs.h | 2 +-
> > > > > > > > > fs/f2fs/gc.c | 6 ++++++
> > > > > > > > > fs/f2fs/inline.c | 17 ++++++++++++++++-
> > > > > > > > > fs/f2fs/inode.c | 2 +-
> > > > > > > > > 4 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > > > index fced2b7652f4..c876813b5532 100644
> > > > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > > > @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
> > > > > > > > > * inline.c
> > > > > > > > > */
> > > > > > > > > bool f2fs_may_inline_data(struct inode *inode);
> > > > > > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode);
> > > > > > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
> > > > > > > > > bool f2fs_may_inline_dentry(struct inode *inode);
> > > > > > > > > void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
> > > > > > > > > void f2fs_truncate_inline_inode(struct inode *inode,
> > > > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > > > > > > index e86c7f01539a..041957750478 100644
> > > > > > > > > --- a/fs/f2fs/gc.c
> > > > > > > > > +++ b/fs/f2fs/gc.c
> > > > > > > > > @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > > > > > > > > continue;
> > > > > > > > > }
> > > > > > > > > + if (f2fs_has_inline_data(inode)) {
> > > > > > > > > + iput(inode);
> > > > > > > > > + set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > > > > > > > + continue;
> > > > > > > >
> > > > > > > > Any race condtion to get this as false alarm?
> > > > > > >
> > > > > > > Since there is no reproducer for the bug, I doubt it was caused by metadata
> > > > > > > fuzzing, something like this:
> > > > > > >
> > > > > > > - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
> > > > > > > - SIT/SSA entry of the block is valid;
> > > > > > > - background GC migrates the block;
> > > > > > > - kworker writeback it, and trigger the bug_on().
> > > > > >
> > > > > > Wasn't detected by sanity_check_inode?
> > > > >
> > > > > I fuzzed non-inline inode w/ below metadata fields:
> > > > > - i_blocks = 1
> > > > > - i_size = 2048
> > > > > - i_inline |= 0x02
> > > > >
> > > > > sanity_check_inode() doesn't complain.
> > > >
> > > > I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
> > >
> > > I didn't figure out a generic way in sanity_check_inode() to catch all fuzzed cases.
> >
> >
> > The patch described:
> > "The root cause is: inline_data inode can be fuzzed, so that there may
> > be valid blkaddr in its direct node, once f2fs triggers background GC
> > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > writeback."
> >
> > Do you suspect the node block address was suddenly assigned after f2fs_iget()?
>
> No, I suspect that the image was fuzzed by tools offline, not in runtime after
> mount().
>
> > Otherwise, it looks checking them in sanity_check_inode would be enough.
> >
> > >
> > > e.g.
> > > case #1
> > > - blkaddr, its dnode, SSA and SIT are consistent
> > > - dnode.footer.ino points to inline inode
> > > - inline inode doesn't link to the donde
> > >
> > > Something like fuzzed special file, please check details in below commit:
> > >
> > > 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
> > >
> > > case #2
> > > - blkaddr, its dnode, SSA and SIT are consistent
> > > - blkaddr locates in inline inode's i_addr
>
> The image status is something like above as I described.
Then, why not just checking the gc path only?
>
> Thanks,
>
> > >
> > > Thanks,
> > >
> > > > whether we really need to check it in the gc path.
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > >
> > > > > > >
> > > > > > > Thoughts?
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > >
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > err = f2fs_gc_pinned_control(inode, gc_type, segno);
> > > > > > > > > if (err == -EAGAIN) {
> > > > > > > > > iput(inode);
> > > > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > > > > > > > > index ac00423f117b..067600fed3d4 100644
> > > > > > > > > --- a/fs/f2fs/inline.c
> > > > > > > > > +++ b/fs/f2fs/inline.c
> > > > > > > > > @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
> > > > > > > > > return !f2fs_post_read_required(inode);
> > > > > > > > > }
> > > > > > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode)
> > > > > > > > > +static bool has_node_blocks(struct inode *inode, struct page *ipage)
> > > > > > > > > +{
> > > > > > > > > + struct f2fs_inode *ri = F2FS_INODE(ipage);
> > > > > > > > > + int i;
> > > > > > > > > +
> > > > > > > > > + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
> > > > > > > > > + if (ri->i_nid[i])
> > > > > > > > > + return true;
> > > > > > > > > + }
> > > > > > > > > + return false;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
> > > > > > > > > {
> > > > > > > > > if (!f2fs_has_inline_data(inode))
> > > > > > > > > return false;
> > > > > > > > > + if (has_node_blocks(inode, ipage))
> > > > > > > > > + return false;
> > > > > > > > > +
> > > > > > > > > if (!support_inline_data(inode))
> > > > > > > > > return true;
> > > > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > > > index c26effdce9aa..1423cd27a477 100644
> > > > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > > > @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > - if (f2fs_sanity_check_inline_data(inode)) {
> > > > > > > > > + if (f2fs_sanity_check_inline_data(inode, node_page)) {
> > > > > > > > > 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);
> > > > > > > > > return false;
> > > > > > > > > --
> > > > > > > > > 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-15 4:39 ` Jaegeuk Kim
@ 2024-05-15 6:12 ` Chao Yu
2024-05-20 16:32 ` Jaegeuk Kim
0 siblings, 1 reply; 19+ messages in thread
From: Chao Yu @ 2024-05-15 6:12 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 2024/5/15 12:39, Jaegeuk Kim wrote:
> On 05/15, Chao Yu wrote:
>> On 2024/5/15 0:07, Jaegeuk Kim wrote:
>>> 外部邮件/External Mail
>>>
>>>
>>> On 05/11, Chao Yu wrote:
>>>> On 2024/5/11 8:38, Jaegeuk Kim wrote:
>>>>> On 05/10, Chao Yu wrote:
>>>>>> On 2024/5/10 11:36, Jaegeuk Kim wrote:
>>>>>>> On 05/10, Chao Yu wrote:
>>>>>>>> On 2024/5/9 23:52, Jaegeuk Kim wrote:
>>>>>>>>> On 05/06, Chao Yu wrote:
>>>>>>>>>> syzbot reports a f2fs bug as below:
>>>>>>>>>>
>>>>>>>>>> ------------[ cut here ]------------
>>>>>>>>>> kernel BUG at fs/f2fs/inline.c:258!
>>>>>>>>>> CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
>>>>>>>>>> RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
>>>>>>>>>> Call Trace:
>>>>>>>>>> f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
>>>>>>>>>> f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
>>>>>>>>>> __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
>>>>>>>>>> f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
>>>>>>>>>> do_writepages+0x35b/0x870 mm/page-writeback.c:2612
>>>>>>>>>> __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
>>>>>>>>>> writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
>>>>>>>>>> wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
>>>>>>>>>> wb_do_writeback fs/fs-writeback.c:2264 [inline]
>>>>>>>>>> wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
>>>>>>>>>> process_one_work kernel/workqueue.c:3254 [inline]
>>>>>>>>>> process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
>>>>>>>>>> worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
>>>>>>>>>> kthread+0x2f2/0x390 kernel/kthread.c:388
>>>>>>>>>> ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
>>>>>>>>>> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>>>>>>>>>>
>>>>>>>>>> The root cause is: inline_data inode can be fuzzed, so that there may
>>>>>>>>>> be valid blkaddr in its direct node, once f2fs triggers background GC
>>>>>>>>>> to migrate the block, it will hit f2fs_bug_on() during dirty page
>>>>>>>>>> writeback.
>>>>>>>>>>
>>>>>>>>>> Let's add sanity check on i_nid field for inline_data inode, meanwhile,
>>>>>>>>>> forbid to migrate inline_data inode's data block to fix this issue.
>>>>>>>>>>
>>>>>>>>>> Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
>>>>>>>>>> Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
>>>>>>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>>>>>>> ---
>>>>>>>>>> fs/f2fs/f2fs.h | 2 +-
>>>>>>>>>> fs/f2fs/gc.c | 6 ++++++
>>>>>>>>>> fs/f2fs/inline.c | 17 ++++++++++++++++-
>>>>>>>>>> fs/f2fs/inode.c | 2 +-
>>>>>>>>>> 4 files changed, 24 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>>>>> index fced2b7652f4..c876813b5532 100644
>>>>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>>>>> @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
>>>>>>>>>> * inline.c
>>>>>>>>>> */
>>>>>>>>>> bool f2fs_may_inline_data(struct inode *inode);
>>>>>>>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode);
>>>>>>>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
>>>>>>>>>> bool f2fs_may_inline_dentry(struct inode *inode);
>>>>>>>>>> void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
>>>>>>>>>> void f2fs_truncate_inline_inode(struct inode *inode,
>>>>>>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>>>>>>> index e86c7f01539a..041957750478 100644
>>>>>>>>>> --- a/fs/f2fs/gc.c
>>>>>>>>>> +++ b/fs/f2fs/gc.c
>>>>>>>>>> @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>>>>>>>>> continue;
>>>>>>>>>> }
>>>>>>>>>> + if (f2fs_has_inline_data(inode)) {
>>>>>>>>>> + iput(inode);
>>>>>>>>>> + set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>>>>>>> + continue;
>>>>>>>>>
>>>>>>>>> Any race condtion to get this as false alarm?
>>>>>>>>
>>>>>>>> Since there is no reproducer for the bug, I doubt it was caused by metadata
>>>>>>>> fuzzing, something like this:
>>>>>>>>
>>>>>>>> - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
>>>>>>>> - SIT/SSA entry of the block is valid;
>>>>>>>> - background GC migrates the block;
>>>>>>>> - kworker writeback it, and trigger the bug_on().
>>>>>>>
>>>>>>> Wasn't detected by sanity_check_inode?
>>>>>>
>>>>>> I fuzzed non-inline inode w/ below metadata fields:
>>>>>> - i_blocks = 1
>>>>>> - i_size = 2048
>>>>>> - i_inline |= 0x02
>>>>>>
>>>>>> sanity_check_inode() doesn't complain.
>>>>>
>>>>> I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
>>>>
>>>> I didn't figure out a generic way in sanity_check_inode() to catch all fuzzed cases.
>>>
>>>
>>> The patch described:
>>> "The root cause is: inline_data inode can be fuzzed, so that there may
>>> be valid blkaddr in its direct node, once f2fs triggers background GC
>>> to migrate the block, it will hit f2fs_bug_on() during dirty page
>>> writeback."
>>>
>>> Do you suspect the node block address was suddenly assigned after f2fs_iget()?
>>
>> No, I suspect that the image was fuzzed by tools offline, not in runtime after
>> mount().
>>
>>> Otherwise, it looks checking them in sanity_check_inode would be enough.
>>>
>>>>
>>>> e.g.
>>>> case #1
>>>> - blkaddr, its dnode, SSA and SIT are consistent
>>>> - dnode.footer.ino points to inline inode
>>>> - inline inode doesn't link to the donde
>>>>
>>>> Something like fuzzed special file, please check details in below commit:
>>>>
>>>> 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
>>>>
>>>> case #2
>>>> - blkaddr, its dnode, SSA and SIT are consistent
>>>> - blkaddr locates in inline inode's i_addr
>>
>> The image status is something like above as I described.
>
> Then, why not just checking the gc path only?
Yes, we can.
has_node_blocks() is added for using a quick check to see whether i_nid
and inline_data flag are inconsistent, should we change this in a separated
patch?
Thanks,
>
>>
>> Thanks,
>>
>>>>
>>>> Thanks,
>>>>
>>>>> whether we really need to check it in the gc path.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> + }
>>>>>>>>>> +
>>>>>>>>>> err = f2fs_gc_pinned_control(inode, gc_type, segno);
>>>>>>>>>> if (err == -EAGAIN) {
>>>>>>>>>> iput(inode);
>>>>>>>>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
>>>>>>>>>> index ac00423f117b..067600fed3d4 100644
>>>>>>>>>> --- a/fs/f2fs/inline.c
>>>>>>>>>> +++ b/fs/f2fs/inline.c
>>>>>>>>>> @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
>>>>>>>>>> return !f2fs_post_read_required(inode);
>>>>>>>>>> }
>>>>>>>>>> -bool f2fs_sanity_check_inline_data(struct inode *inode)
>>>>>>>>>> +static bool has_node_blocks(struct inode *inode, struct page *ipage)
>>>>>>>>>> +{
>>>>>>>>>> + struct f2fs_inode *ri = F2FS_INODE(ipage);
>>>>>>>>>> + int i;
>>>>>>>>>> +
>>>>>>>>>> + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
>>>>>>>>>> + if (ri->i_nid[i])
>>>>>>>>>> + return true;
>>>>>>>>>> + }
>>>>>>>>>> + return false;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
>>>>>>>>>> {
>>>>>>>>>> if (!f2fs_has_inline_data(inode))
>>>>>>>>>> return false;
>>>>>>>>>> + if (has_node_blocks(inode, ipage))
>>>>>>>>>> + return false;
>>>>>>>>>> +
>>>>>>>>>> if (!support_inline_data(inode))
>>>>>>>>>> return true;
>>>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>>>>> index c26effdce9aa..1423cd27a477 100644
>>>>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>>>>> @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>> - if (f2fs_sanity_check_inline_data(inode)) {
>>>>>>>>>> + if (f2fs_sanity_check_inline_data(inode, node_page)) {
>>>>>>>>>> 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);
>>>>>>>>>> return false;
>>>>>>>>>> --
>>>>>>>>>> 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode
2024-05-15 6:12 ` Chao Yu
@ 2024-05-20 16:32 ` Jaegeuk Kim
0 siblings, 0 replies; 19+ messages in thread
From: Jaegeuk Kim @ 2024-05-20 16:32 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, syzbot+848062ba19c8782ca5c8, linux-f2fs-devel
On 05/15, Chao Yu wrote:
> On 2024/5/15 12:39, Jaegeuk Kim wrote:
> > On 05/15, Chao Yu wrote:
> > > On 2024/5/15 0:07, Jaegeuk Kim wrote:
> > > > 外部邮件/External Mail
> > > >
> > > >
> > > > On 05/11, Chao Yu wrote:
> > > > > On 2024/5/11 8:38, Jaegeuk Kim wrote:
> > > > > > On 05/10, Chao Yu wrote:
> > > > > > > On 2024/5/10 11:36, Jaegeuk Kim wrote:
> > > > > > > > On 05/10, Chao Yu wrote:
> > > > > > > > > On 2024/5/9 23:52, Jaegeuk Kim wrote:
> > > > > > > > > > On 05/06, Chao Yu wrote:
> > > > > > > > > > > syzbot reports a f2fs bug as below:
> > > > > > > > > > >
> > > > > > > > > > > ------------[ cut here ]------------
> > > > > > > > > > > kernel BUG at fs/f2fs/inline.c:258!
> > > > > > > > > > > CPU: 1 PID: 34 Comm: kworker/u8:2 Not tainted 6.9.0-rc6-syzkaller-00012-g9e4bc4bcae01 #0
> > > > > > > > > > > RIP: 0010:f2fs_write_inline_data+0x781/0x790 fs/f2fs/inline.c:258
> > > > > > > > > > > Call Trace:
> > > > > > > > > > > f2fs_write_single_data_page+0xb65/0x1d60 fs/f2fs/data.c:2834
> > > > > > > > > > > f2fs_write_cache_pages fs/f2fs/data.c:3133 [inline]
> > > > > > > > > > > __f2fs_write_data_pages fs/f2fs/data.c:3288 [inline]
> > > > > > > > > > > f2fs_write_data_pages+0x1efe/0x3a90 fs/f2fs/data.c:3315
> > > > > > > > > > > do_writepages+0x35b/0x870 mm/page-writeback.c:2612
> > > > > > > > > > > __writeback_single_inode+0x165/0x10b0 fs/fs-writeback.c:1650
> > > > > > > > > > > writeback_sb_inodes+0x905/0x1260 fs/fs-writeback.c:1941
> > > > > > > > > > > wb_writeback+0x457/0xce0 fs/fs-writeback.c:2117
> > > > > > > > > > > wb_do_writeback fs/fs-writeback.c:2264 [inline]
> > > > > > > > > > > wb_workfn+0x410/0x1090 fs/fs-writeback.c:2304
> > > > > > > > > > > process_one_work kernel/workqueue.c:3254 [inline]
> > > > > > > > > > > process_scheduled_works+0xa12/0x17c0 kernel/workqueue.c:3335
> > > > > > > > > > > worker_thread+0x86d/0xd70 kernel/workqueue.c:3416
> > > > > > > > > > > kthread+0x2f2/0x390 kernel/kthread.c:388
> > > > > > > > > > > ret_from_fork+0x4d/0x80 arch/x86/kernel/process.c:147
> > > > > > > > > > > ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
> > > > > > > > > > >
> > > > > > > > > > > The root cause is: inline_data inode can be fuzzed, so that there may
> > > > > > > > > > > be valid blkaddr in its direct node, once f2fs triggers background GC
> > > > > > > > > > > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > > > > > > > > > > writeback.
> > > > > > > > > > >
> > > > > > > > > > > Let's add sanity check on i_nid field for inline_data inode, meanwhile,
> > > > > > > > > > > forbid to migrate inline_data inode's data block to fix this issue.
> > > > > > > > > > >
> > > > > > > > > > > Reported-by: syzbot+848062ba19c8782ca5c8@syzkaller.appspotmail.com
> > > > > > > > > > > Closes: https://lore.kernel.org/linux-f2fs-devel/000000000000d103ce06174d7ec3@google.com
> > > > > > > > > > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > > > > > > > > > ---
> > > > > > > > > > > fs/f2fs/f2fs.h | 2 +-
> > > > > > > > > > > fs/f2fs/gc.c | 6 ++++++
> > > > > > > > > > > fs/f2fs/inline.c | 17 ++++++++++++++++-
> > > > > > > > > > > fs/f2fs/inode.c | 2 +-
> > > > > > > > > > > 4 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > > > > > > > index fced2b7652f4..c876813b5532 100644
> > > > > > > > > > > --- a/fs/f2fs/f2fs.h
> > > > > > > > > > > +++ b/fs/f2fs/f2fs.h
> > > > > > > > > > > @@ -4146,7 +4146,7 @@ extern struct kmem_cache *f2fs_inode_entry_slab;
> > > > > > > > > > > * inline.c
> > > > > > > > > > > */
> > > > > > > > > > > bool f2fs_may_inline_data(struct inode *inode);
> > > > > > > > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode);
> > > > > > > > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage);
> > > > > > > > > > > bool f2fs_may_inline_dentry(struct inode *inode);
> > > > > > > > > > > void f2fs_do_read_inline_data(struct page *page, struct page *ipage);
> > > > > > > > > > > void f2fs_truncate_inline_inode(struct inode *inode,
> > > > > > > > > > > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > > > > > > > > > > index e86c7f01539a..041957750478 100644
> > > > > > > > > > > --- a/fs/f2fs/gc.c
> > > > > > > > > > > +++ b/fs/f2fs/gc.c
> > > > > > > > > > > @@ -1563,6 +1563,12 @@ static int gc_data_segment(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > > > > > > > > > > continue;
> > > > > > > > > > > }
> > > > > > > > > > > + if (f2fs_has_inline_data(inode)) {
> > > > > > > > > > > + iput(inode);
> > > > > > > > > > > + set_sbi_flag(sbi, SBI_NEED_FSCK);
> > > > > > > > > > > + continue;
> > > > > > > > > >
> > > > > > > > > > Any race condtion to get this as false alarm?
> > > > > > > > >
> > > > > > > > > Since there is no reproducer for the bug, I doubt it was caused by metadata
> > > > > > > > > fuzzing, something like this:
> > > > > > > > >
> > > > > > > > > - inline inode has one valid blkaddr in i_addr or in dnode reference by i_nid;
> > > > > > > > > - SIT/SSA entry of the block is valid;
> > > > > > > > > - background GC migrates the block;
> > > > > > > > > - kworker writeback it, and trigger the bug_on().
> > > > > > > >
> > > > > > > > Wasn't detected by sanity_check_inode?
> > > > > > >
> > > > > > > I fuzzed non-inline inode w/ below metadata fields:
> > > > > > > - i_blocks = 1
> > > > > > > - i_size = 2048
> > > > > > > - i_inline |= 0x02
> > > > > > >
> > > > > > > sanity_check_inode() doesn't complain.
> > > > > >
> > > > > > I mean, the below sanity_check_inode() can cover the fuzzed case? I'm wondering
> > > > >
> > > > > I didn't figure out a generic way in sanity_check_inode() to catch all fuzzed cases.
> > > >
> > > >
> > > > The patch described:
> > > > "The root cause is: inline_data inode can be fuzzed, so that there may
> > > > be valid blkaddr in its direct node, once f2fs triggers background GC
> > > > to migrate the block, it will hit f2fs_bug_on() during dirty page
> > > > writeback."
> > > >
> > > > Do you suspect the node block address was suddenly assigned after f2fs_iget()?
> > >
> > > No, I suspect that the image was fuzzed by tools offline, not in runtime after
> > > mount().
> > >
> > > > Otherwise, it looks checking them in sanity_check_inode would be enough.
> > > >
> > > > >
> > > > > e.g.
> > > > > case #1
> > > > > - blkaddr, its dnode, SSA and SIT are consistent
> > > > > - dnode.footer.ino points to inline inode
> > > > > - inline inode doesn't link to the donde
> > > > >
> > > > > Something like fuzzed special file, please check details in below commit:
> > > > >
> > > > > 9056d6489f5a ("f2fs: fix to do sanity check on inode type during garbage collection")
> > > > >
> > > > > case #2
> > > > > - blkaddr, its dnode, SSA and SIT are consistent
> > > > > - blkaddr locates in inline inode's i_addr
> > >
> > > The image status is something like above as I described.
> >
> > Then, why not just checking the gc path only?
>
> Yes, we can.
>
> has_node_blocks() is added for using a quick check to see whether i_nid
> and inline_data flag are inconsistent, should we change this in a separated
> patch?
Yup, I think it'd be better to have a patch per issue to attack the exact
problem.
>
> Thanks,
>
> >
> > >
> > > Thanks,
> > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > > whether we really need to check it in the gc path.
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Thoughts?
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > + }
> > > > > > > > > > > +
> > > > > > > > > > > err = f2fs_gc_pinned_control(inode, gc_type, segno);
> > > > > > > > > > > if (err == -EAGAIN) {
> > > > > > > > > > > iput(inode);
> > > > > > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > > > > > > > > > > index ac00423f117b..067600fed3d4 100644
> > > > > > > > > > > --- a/fs/f2fs/inline.c
> > > > > > > > > > > +++ b/fs/f2fs/inline.c
> > > > > > > > > > > @@ -33,11 +33,26 @@ bool f2fs_may_inline_data(struct inode *inode)
> > > > > > > > > > > return !f2fs_post_read_required(inode);
> > > > > > > > > > > }
> > > > > > > > > > > -bool f2fs_sanity_check_inline_data(struct inode *inode)
> > > > > > > > > > > +static bool has_node_blocks(struct inode *inode, struct page *ipage)
> > > > > > > > > > > +{
> > > > > > > > > > > + struct f2fs_inode *ri = F2FS_INODE(ipage);
> > > > > > > > > > > + int i;
> > > > > > > > > > > +
> > > > > > > > > > > + for (i = 0; i < DEF_NIDS_PER_INODE; i++) {
> > > > > > > > > > > + if (ri->i_nid[i])
> > > > > > > > > > > + return true;
> > > > > > > > > > > + }
> > > > > > > > > > > + return false;
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +bool f2fs_sanity_check_inline_data(struct inode *inode, struct page *ipage)
> > > > > > > > > > > {
> > > > > > > > > > > if (!f2fs_has_inline_data(inode))
> > > > > > > > > > > return false;
> > > > > > > > > > > + if (has_node_blocks(inode, ipage))
> > > > > > > > > > > + return false;
> > > > > > > > > > > +
> > > > > > > > > > > if (!support_inline_data(inode))
> > > > > > > > > > > return true;
> > > > > > > > > > > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > > > > > > > > > > index c26effdce9aa..1423cd27a477 100644
> > > > > > > > > > > --- a/fs/f2fs/inode.c
> > > > > > > > > > > +++ b/fs/f2fs/inode.c
> > > > > > > > > > > @@ -343,7 +343,7 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
> > > > > > > > > > > }
> > > > > > > > > > > }
> > > > > > > > > > > - if (f2fs_sanity_check_inline_data(inode)) {
> > > > > > > > > > > + if (f2fs_sanity_check_inline_data(inode, node_page)) {
> > > > > > > > > > > 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);
> > > > > > > > > > > return false;
> > > > > > > > > > > --
> > > > > > > > > > > 2.40.1
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-05-20 16:32 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-06 10:33 [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() Chao Yu
2024-05-06 10:33 ` [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() Chao Yu
2024-05-09 0:46 ` Jaegeuk Kim
2024-05-09 2:49 ` Chao Yu
2024-05-09 15:50 ` Jaegeuk Kim
2024-05-10 3:36 ` Chao Yu
2024-05-06 10:33 ` [f2fs-dev] [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode Chao Yu
2024-05-09 15:52 ` Jaegeuk Kim
2024-05-10 2:14 ` Chao Yu
2024-05-10 3:36 ` Jaegeuk Kim
2024-05-10 14:16 ` Chao Yu
2024-05-11 0:38 ` Jaegeuk Kim
2024-05-11 3:07 ` Chao Yu
2024-05-14 16:07 ` Jaegeuk Kim
2024-05-15 1:34 ` Chao Yu
2024-05-15 4:39 ` Jaegeuk Kim
2024-05-15 6:12 ` Chao Yu
2024-05-20 16:32 ` Jaegeuk Kim
2024-05-11 0:50 ` [f2fs-dev] [PATCH 1/3] f2fs: fix to release node block count in error path of f2fs_new_node_page() patchwork-bot+f2fs
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).