* [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
0 siblings, 0 replies; 38+ 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] 38+ messages in thread* [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 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-06 10:33 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() 2024-05-06 10:33 ` Chao Yu @ 2024-05-06 10:33 ` Chao Yu -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() @ 2024-05-06 10:33 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-06 10:33 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu 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 ^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() 2024-05-06 10:33 ` Chao Yu @ 2024-05-09 0:46 ` Jaegeuk Kim -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() @ 2024-05-09 0:46 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-09 0:46 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() @ 2024-05-09 2:49 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-09 2:49 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() @ 2024-05-09 15:50 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-09 15:50 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 38+ 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-10 3:36 ` Chao Yu -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [f2fs-dev] [PATCH 2/3] f2fs: fix to add missing iput() in gc_data_segment() @ 2024-05-10 3:36 ` Chao Yu 0 siblings, 0 replies; 38+ 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 ^ permalink raw reply [flat|nested] 38+ 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 ` Chao Yu @ 2024-05-06 10:33 ` Chao Yu -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-06 10:33 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-06 10:33 UTC (permalink / raw) To: jaegeuk Cc: linux-f2fs-devel, linux-kernel, Chao Yu, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply related [flat|nested] 38+ 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 ` Chao Yu @ 2024-05-09 15:52 ` Jaegeuk Kim -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-09 15:52 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-09 15:52 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-10 2:14 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-10 2:14 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-10 3:36 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-10 3:36 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-10 14:16 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-10 14:16 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-11 0:38 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-11 0:38 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-11 3:07 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-11 3:07 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-14 16:07 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-14 16:07 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-15 1:34 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-15 1:34 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-15 4:39 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-15 4:39 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-15 6:12 ` Chao Yu 0 siblings, 0 replies; 38+ messages in thread From: Chao Yu @ 2024-05-15 6:12 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 -1 siblings, 0 replies; 38+ 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] 38+ messages in thread
* Re: [PATCH 3/3] f2fs: fix to do sanity check on i_nid for inline_data inode @ 2024-05-20 16:32 ` Jaegeuk Kim 0 siblings, 0 replies; 38+ messages in thread From: Jaegeuk Kim @ 2024-05-20 16:32 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, syzbot+848062ba19c8782ca5c8 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 ^ permalink raw reply [flat|nested] 38+ 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 ` Chao Yu @ 2024-05-11 0:50 ` patchwork-bot+f2fs -1 siblings, 0 replies; 38+ 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] 38+ 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-11 0:50 ` patchwork-bot+f2fs 0 siblings, 0 replies; 38+ 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2024-05-20 16:32 UTC | newest] Thread overview: 38+ 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 ` 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 0:46 ` [f2fs-dev] " Jaegeuk Kim 2024-05-09 0:46 ` Jaegeuk Kim 2024-05-09 2:49 ` [f2fs-dev] " Chao Yu 2024-05-09 2:49 ` Chao Yu 2024-05-09 15:50 ` [f2fs-dev] " Jaegeuk Kim 2024-05-09 15:50 ` Jaegeuk Kim 2024-05-10 3:36 ` [f2fs-dev] " Chao Yu 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-06 10:33 ` Chao Yu 2024-05-09 15:52 ` [f2fs-dev] " Jaegeuk Kim 2024-05-09 15:52 ` Jaegeuk Kim 2024-05-10 2:14 ` [f2fs-dev] " Chao Yu 2024-05-10 2:14 ` Chao Yu 2024-05-10 3:36 ` [f2fs-dev] " Jaegeuk Kim 2024-05-10 3:36 ` Jaegeuk Kim 2024-05-10 14:16 ` [f2fs-dev] " Chao Yu 2024-05-10 14:16 ` Chao Yu 2024-05-11 0:38 ` [f2fs-dev] " Jaegeuk Kim 2024-05-11 0:38 ` Jaegeuk Kim 2024-05-11 3:07 ` [f2fs-dev] " Chao Yu 2024-05-11 3:07 ` Chao Yu 2024-05-14 16:07 ` [f2fs-dev] " Jaegeuk Kim 2024-05-14 16:07 ` Jaegeuk Kim 2024-05-15 1:34 ` [f2fs-dev] " Chao Yu 2024-05-15 1:34 ` Chao Yu 2024-05-15 4:39 ` [f2fs-dev] " Jaegeuk Kim 2024-05-15 4:39 ` Jaegeuk Kim 2024-05-15 6:12 ` [f2fs-dev] " Chao Yu 2024-05-15 6:12 ` Chao Yu 2024-05-20 16:32 ` [f2fs-dev] " Jaegeuk Kim 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 2024-05-11 0:50 ` patchwork-bot+f2fs
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.