| Message ID | 30acf23be32724aa2cae7e85a6b88e039f290773.1691054362.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix an ASSERT() triggered inside prepare_to_merge() | expand |
On Thu, Aug 3, 2023 at 10:45 AM Qu Wenruo <wqu@suse.com> wrote: > > [BUG] > Syzbot reported a crash that an ASSERT() got triggered inside > prepare_to_merge(). > > [CAUSE] > The root cause of the triggered ASSERT() is we can have a race between > quota tree creation and relocation. > > This leads us to create a duplicated quota tree in the > btrfs_read_fs_root() path, and since it's treated as fs tree, it would > have ROOT_SHAREABLE flag, causing us to create a reloc tree for it. > > The bug itself is fixed by a dedicated patch for it, but this already > taught us the ASSERT() is not something straightforward for > developers. > > [ENHANCEMENT] > Instead of using an ASSERT(), let's handle it gracefully and output > extra info about the mismatch reloc roots to help debug. > > Also with the above ASSERT() removed, we can trigger ASSERT(0)s inside > merge_reloc_roots() later. > Also replace those ASSERT(0)s with WARN_ON()s. > > Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Looks good now, thanks. > --- > fs/btrfs/relocation.c | 44 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 36 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 9db2e6fa2cb2..2bd97d2cb52e 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -1916,7 +1916,38 @@ int prepare_to_merge(struct reloc_control *rc, int err) > err = PTR_ERR(root); > break; > } > - ASSERT(root->reloc_root == reloc_root); > + > + if (unlikely(root->reloc_root != reloc_root)) { > + if (root->reloc_root) > + btrfs_err(fs_info, > +"reloc tree mismatch, root %lld has reloc root key (%lld %u %llu) gen %llu, expect reloc root key (%lld %u %llu) gen %llu", > + root->root_key.objectid, > + root->reloc_root->root_key.objectid, > + root->reloc_root->root_key.type, > + root->reloc_root->root_key.offset, > + btrfs_root_generation( > + &root->reloc_root->root_item), > + reloc_root->root_key.objectid, > + reloc_root->root_key.type, > + reloc_root->root_key.offset, > + btrfs_root_generation( > + &reloc_root->root_item)); > + else > + btrfs_err(fs_info, > +"reloc tree mismatch, root %lld has no reloc root, expect reloc root key (%lld %u %llu) gen %llu", > + root->root_key.objectid, > + reloc_root->root_key.objectid, > + reloc_root->root_key.type, > + reloc_root->root_key.offset, > + btrfs_root_generation( > + &reloc_root->root_item)); > + list_add(&reloc_root->root_list, &reloc_roots); > + btrfs_put_root(root); > + btrfs_abort_transaction(trans, -EUCLEAN); > + if (!err) > + err = -EUCLEAN; > + break; > + } > > /* > * set reference count to 1, so btrfs_recover_relocation > @@ -1989,7 +2020,7 @@ void merge_reloc_roots(struct reloc_control *rc) > root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, > false); > if (btrfs_root_refs(&reloc_root->root_item) > 0) { > - if (IS_ERR(root)) { > + if (WARN_ON(IS_ERR(root))) { > /* > * For recovery we read the fs roots on mount, > * and if we didn't find the root then we marked > @@ -1998,17 +2029,14 @@ void merge_reloc_roots(struct reloc_control *rc) > * memory. However there's no reason we can't > * handle the error properly here just in case. > */ > - ASSERT(0); > ret = PTR_ERR(root); > goto out; > } > - if (root->reloc_root != reloc_root) { > + if (WARN_ON(root->reloc_root != reloc_root)) { > /* > - * This is actually impossible without something > - * going really wrong (like weird race condition > - * or cosmic rays). > + * This can happen if on-disk metadata has some > + * corruption, e.g. bad reloc tree key offset. > */ > - ASSERT(0); > ret = -EINVAL; > goto out; > } > -- > 2.41.0 >
On Thu, Aug 03, 2023 at 05:20:42PM +0800, Qu Wenruo wrote: > [BUG] > Syzbot reported a crash that an ASSERT() got triggered inside > prepare_to_merge(). > > [CAUSE] > The root cause of the triggered ASSERT() is we can have a race between > quota tree creation and relocation. > > This leads us to create a duplicated quota tree in the > btrfs_read_fs_root() path, and since it's treated as fs tree, it would > have ROOT_SHAREABLE flag, causing us to create a reloc tree for it. > > The bug itself is fixed by a dedicated patch for it, but this already > taught us the ASSERT() is not something straightforward for > developers. > > [ENHANCEMENT] > Instead of using an ASSERT(), let's handle it gracefully and output > extra info about the mismatch reloc roots to help debug. > > Also with the above ASSERT() removed, we can trigger ASSERT(0)s inside > merge_reloc_roots() later. > Also replace those ASSERT(0)s with WARN_ON()s. > > Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/relocation.c | 44 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 36 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 9db2e6fa2cb2..2bd97d2cb52e 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -1916,7 +1916,38 @@ int prepare_to_merge(struct reloc_control *rc, int err) > err = PTR_ERR(root); > break; > } > - ASSERT(root->reloc_root == reloc_root); > + > + if (unlikely(root->reloc_root != reloc_root)) { > + if (root->reloc_root) > + btrfs_err(fs_info, > +"reloc tree mismatch, root %lld has reloc root key (%lld %u %llu) gen %llu, expect reloc root key (%lld %u %llu) gen %llu", > + root->root_key.objectid, > + root->reloc_root->root_key.objectid, > + root->reloc_root->root_key.type, > + root->reloc_root->root_key.offset, > + btrfs_root_generation( > + &root->reloc_root->root_item), > + reloc_root->root_key.objectid, > + reloc_root->root_key.type, > + reloc_root->root_key.offset, > + btrfs_root_generation( > + &reloc_root->root_item)); > + else > + btrfs_err(fs_info, > +"reloc tree mismatch, root %lld has no reloc root, expect reloc root key (%lld %u %llu) gen %llu", > + root->root_key.objectid, > + reloc_root->root_key.objectid, > + reloc_root->root_key.type, > + reloc_root->root_key.offset, > + btrfs_root_generation( > + &reloc_root->root_item)); > + list_add(&reloc_root->root_list, &reloc_roots); > + btrfs_put_root(root); > + btrfs_abort_transaction(trans, -EUCLEAN); > + if (!err) > + err = -EUCLEAN; > + break; > + } > > /* > * set reference count to 1, so btrfs_recover_relocation > @@ -1989,7 +2020,7 @@ void merge_reloc_roots(struct reloc_control *rc) > root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, > false); > if (btrfs_root_refs(&reloc_root->root_item) > 0) { > - if (IS_ERR(root)) { > + if (WARN_ON(IS_ERR(root))) { > /* > * For recovery we read the fs roots on mount, > * and if we didn't find the root then we marked > @@ -1998,17 +2029,14 @@ void merge_reloc_roots(struct reloc_control *rc) > * memory. However there's no reason we can't > * handle the error properly here just in case. > */ > - ASSERT(0); > ret = PTR_ERR(root); > goto out; > } > - if (root->reloc_root != reloc_root) { > + if (WARN_ON(root->reloc_root != reloc_root)) { > /* > - * This is actually impossible without something > - * going really wrong (like weird race condition > - * or cosmic rays). > + * This can happen if on-disk metadata has some > + * corruption, e.g. bad reloc tree key offset. Based on the comments, hitting that conditition is possible by corrupted on-disk, so the WARN_ON should not be here. I did not like the ASSERT(0) and I don't like the WARN_ON either. It's an anti-pattern and we should not spread it. Some hints and suggestions are documented here https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#handling-unexpected-conditions but we can make it more explicit how to them.
On 2023/8/10 03:24, David Sterba wrote: > On Thu, Aug 03, 2023 at 05:20:42PM +0800, Qu Wenruo wrote: >> [BUG] >> Syzbot reported a crash that an ASSERT() got triggered inside >> prepare_to_merge(). >> >> [CAUSE] >> The root cause of the triggered ASSERT() is we can have a race between >> quota tree creation and relocation. >> >> This leads us to create a duplicated quota tree in the >> btrfs_read_fs_root() path, and since it's treated as fs tree, it would >> have ROOT_SHAREABLE flag, causing us to create a reloc tree for it. >> >> The bug itself is fixed by a dedicated patch for it, but this already >> taught us the ASSERT() is not something straightforward for >> developers. >> >> [ENHANCEMENT] >> Instead of using an ASSERT(), let's handle it gracefully and output >> extra info about the mismatch reloc roots to help debug. >> >> Also with the above ASSERT() removed, we can trigger ASSERT(0)s inside >> merge_reloc_roots() later. >> Also replace those ASSERT(0)s with WARN_ON()s. >> >> Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/relocation.c | 44 +++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 36 insertions(+), 8 deletions(-) >> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >> index 9db2e6fa2cb2..2bd97d2cb52e 100644 >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -1916,7 +1916,38 @@ int prepare_to_merge(struct reloc_control *rc, int err) >> err = PTR_ERR(root); >> break; >> } >> - ASSERT(root->reloc_root == reloc_root); >> + >> + if (unlikely(root->reloc_root != reloc_root)) { >> + if (root->reloc_root) >> + btrfs_err(fs_info, >> +"reloc tree mismatch, root %lld has reloc root key (%lld %u %llu) gen %llu, expect reloc root key (%lld %u %llu) gen %llu", >> + root->root_key.objectid, >> + root->reloc_root->root_key.objectid, >> + root->reloc_root->root_key.type, >> + root->reloc_root->root_key.offset, >> + btrfs_root_generation( >> + &root->reloc_root->root_item), >> + reloc_root->root_key.objectid, >> + reloc_root->root_key.type, >> + reloc_root->root_key.offset, >> + btrfs_root_generation( >> + &reloc_root->root_item)); >> + else >> + btrfs_err(fs_info, >> +"reloc tree mismatch, root %lld has no reloc root, expect reloc root key (%lld %u %llu) gen %llu", >> + root->root_key.objectid, >> + reloc_root->root_key.objectid, >> + reloc_root->root_key.type, >> + reloc_root->root_key.offset, >> + btrfs_root_generation( >> + &reloc_root->root_item)); >> + list_add(&reloc_root->root_list, &reloc_roots); >> + btrfs_put_root(root); >> + btrfs_abort_transaction(trans, -EUCLEAN); >> + if (!err) >> + err = -EUCLEAN; >> + break; >> + } >> >> /* >> * set reference count to 1, so btrfs_recover_relocation >> @@ -1989,7 +2020,7 @@ void merge_reloc_roots(struct reloc_control *rc) >> root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, >> false); >> if (btrfs_root_refs(&reloc_root->root_item) > 0) { >> - if (IS_ERR(root)) { >> + if (WARN_ON(IS_ERR(root))) { >> /* >> * For recovery we read the fs roots on mount, >> * and if we didn't find the root then we marked >> @@ -1998,17 +2029,14 @@ void merge_reloc_roots(struct reloc_control *rc) >> * memory. However there's no reason we can't >> * handle the error properly here just in case. >> */ >> - ASSERT(0); >> ret = PTR_ERR(root); >> goto out; >> } >> - if (root->reloc_root != reloc_root) { >> + if (WARN_ON(root->reloc_root != reloc_root)) { >> /* >> - * This is actually impossible without something >> - * going really wrong (like weird race condition >> - * or cosmic rays). >> + * This can happen if on-disk metadata has some >> + * corruption, e.g. bad reloc tree key offset. > > Based on the comments, hitting that conditition is possible by corrupted > on-disk, so the WARN_ON should not be here. I did not like the ASSERT(0) > and I don't like the WARN_ON either. It's an anti-pattern and we should > not spread it. > > Some hints and suggestions are documented here > https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#handling-unexpected-conditions > but we can make it more explicit how to them. I don't like the WARN_ON() either, however I didn't have any better idea/practice at that time. For now, I believe WARN_ON() should only be utilized when there is a strong need for a backtrace, and it's not covered by any existing facility like btrfs_abort_transaction(). If we go that idea, the WARN_ON() is not needed at all, since the there is only one call chain leading to the error, and the error message alone is enough to locate the call chain. For other situations where there are multiple call chains involved, we may want to remove unnecessary WARN_ON(), with btrfs_abort_transaction() closer to the error. Would this idea be more practical? Thanks, Qu
On Thu, Aug 10, 2023 at 08:57:24AM +0800, Qu Wenruo wrote: > On 2023/8/10 03:24, David Sterba wrote: > > On Thu, Aug 03, 2023 at 05:20:42PM +0800, Qu Wenruo wrote: > >> [BUG] > >> Syzbot reported a crash that an ASSERT() got triggered inside > >> prepare_to_merge(). > >> > >> [CAUSE] > >> The root cause of the triggered ASSERT() is we can have a race between > >> quota tree creation and relocation. > >> > >> This leads us to create a duplicated quota tree in the > >> btrfs_read_fs_root() path, and since it's treated as fs tree, it would > >> have ROOT_SHAREABLE flag, causing us to create a reloc tree for it. > >> > >> The bug itself is fixed by a dedicated patch for it, but this already > >> taught us the ASSERT() is not something straightforward for > >> developers. > >> > >> [ENHANCEMENT] > >> Instead of using an ASSERT(), let's handle it gracefully and output > >> extra info about the mismatch reloc roots to help debug. > >> > >> Also with the above ASSERT() removed, we can trigger ASSERT(0)s inside > >> merge_reloc_roots() later. > >> Also replace those ASSERT(0)s with WARN_ON()s. > >> > >> Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> fs/btrfs/relocation.c | 44 +++++++++++++++++++++++++++++++++++-------- > >> 1 file changed, 36 insertions(+), 8 deletions(-) > >> > >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > >> index 9db2e6fa2cb2..2bd97d2cb52e 100644 > >> --- a/fs/btrfs/relocation.c > >> +++ b/fs/btrfs/relocation.c > >> @@ -1916,7 +1916,38 @@ int prepare_to_merge(struct reloc_control *rc, int err) > >> err = PTR_ERR(root); > >> break; > >> } > >> - ASSERT(root->reloc_root == reloc_root); > >> + > >> + if (unlikely(root->reloc_root != reloc_root)) { > >> + if (root->reloc_root) > >> + btrfs_err(fs_info, > >> +"reloc tree mismatch, root %lld has reloc root key (%lld %u %llu) gen %llu, expect reloc root key (%lld %u %llu) gen %llu", > >> + root->root_key.objectid, > >> + root->reloc_root->root_key.objectid, > >> + root->reloc_root->root_key.type, > >> + root->reloc_root->root_key.offset, > >> + btrfs_root_generation( > >> + &root->reloc_root->root_item), > >> + reloc_root->root_key.objectid, > >> + reloc_root->root_key.type, > >> + reloc_root->root_key.offset, > >> + btrfs_root_generation( > >> + &reloc_root->root_item)); > >> + else > >> + btrfs_err(fs_info, > >> +"reloc tree mismatch, root %lld has no reloc root, expect reloc root key (%lld %u %llu) gen %llu", > >> + root->root_key.objectid, > >> + reloc_root->root_key.objectid, > >> + reloc_root->root_key.type, > >> + reloc_root->root_key.offset, > >> + btrfs_root_generation( > >> + &reloc_root->root_item)); > >> + list_add(&reloc_root->root_list, &reloc_roots); > >> + btrfs_put_root(root); > >> + btrfs_abort_transaction(trans, -EUCLEAN); > >> + if (!err) > >> + err = -EUCLEAN; > >> + break; > >> + } > >> > >> /* > >> * set reference count to 1, so btrfs_recover_relocation > >> @@ -1989,7 +2020,7 @@ void merge_reloc_roots(struct reloc_control *rc) > >> root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, > >> false); > >> if (btrfs_root_refs(&reloc_root->root_item) > 0) { > >> - if (IS_ERR(root)) { > >> + if (WARN_ON(IS_ERR(root))) { > >> /* > >> * For recovery we read the fs roots on mount, > >> * and if we didn't find the root then we marked > >> @@ -1998,17 +2029,14 @@ void merge_reloc_roots(struct reloc_control *rc) > >> * memory. However there's no reason we can't > >> * handle the error properly here just in case. > >> */ > >> - ASSERT(0); > >> ret = PTR_ERR(root); > >> goto out; > >> } > >> - if (root->reloc_root != reloc_root) { > >> + if (WARN_ON(root->reloc_root != reloc_root)) { > >> /* > >> - * This is actually impossible without something > >> - * going really wrong (like weird race condition > >> - * or cosmic rays). > >> + * This can happen if on-disk metadata has some > >> + * corruption, e.g. bad reloc tree key offset. > > > > Based on the comments, hitting that conditition is possible by corrupted > > on-disk, so the WARN_ON should not be here. I did not like the ASSERT(0) > > and I don't like the WARN_ON either. It's an anti-pattern and we should > > not spread it. > > > > Some hints and suggestions are documented here > > https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#handling-unexpected-conditions > > but we can make it more explicit how to them. > > I don't like the WARN_ON() either, however I didn't have any better > idea/practice at that time. > > For now, I believe WARN_ON() should only be utilized when there is a > strong need for a backtrace, and it's not covered by any existing > facility like btrfs_abort_transaction(). > > If we go that idea, the WARN_ON() is not needed at all, since the there > is only one call chain leading to the error, and the error message alone > is enough to locate the call chain. > > For other situations where there are multiple call chains involved, we > may want to remove unnecessary WARN_ON(), with btrfs_abort_transaction() > closer to the error. > > Would this idea be more practical? As a general idea yes. Not in all cases where the call chain is different we need to know it but we'll deal with that case by case.
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 9db2e6fa2cb2..2bd97d2cb52e 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -1916,7 +1916,38 @@ int prepare_to_merge(struct reloc_control *rc, int err) err = PTR_ERR(root); break; } - ASSERT(root->reloc_root == reloc_root); + + if (unlikely(root->reloc_root != reloc_root)) { + if (root->reloc_root) + btrfs_err(fs_info, +"reloc tree mismatch, root %lld has reloc root key (%lld %u %llu) gen %llu, expect reloc root key (%lld %u %llu) gen %llu", + root->root_key.objectid, + root->reloc_root->root_key.objectid, + root->reloc_root->root_key.type, + root->reloc_root->root_key.offset, + btrfs_root_generation( + &root->reloc_root->root_item), + reloc_root->root_key.objectid, + reloc_root->root_key.type, + reloc_root->root_key.offset, + btrfs_root_generation( + &reloc_root->root_item)); + else + btrfs_err(fs_info, +"reloc tree mismatch, root %lld has no reloc root, expect reloc root key (%lld %u %llu) gen %llu", + root->root_key.objectid, + reloc_root->root_key.objectid, + reloc_root->root_key.type, + reloc_root->root_key.offset, + btrfs_root_generation( + &reloc_root->root_item)); + list_add(&reloc_root->root_list, &reloc_roots); + btrfs_put_root(root); + btrfs_abort_transaction(trans, -EUCLEAN); + if (!err) + err = -EUCLEAN; + break; + } /* * set reference count to 1, so btrfs_recover_relocation @@ -1989,7 +2020,7 @@ void merge_reloc_roots(struct reloc_control *rc) root = btrfs_get_fs_root(fs_info, reloc_root->root_key.offset, false); if (btrfs_root_refs(&reloc_root->root_item) > 0) { - if (IS_ERR(root)) { + if (WARN_ON(IS_ERR(root))) { /* * For recovery we read the fs roots on mount, * and if we didn't find the root then we marked @@ -1998,17 +2029,14 @@ void merge_reloc_roots(struct reloc_control *rc) * memory. However there's no reason we can't * handle the error properly here just in case. */ - ASSERT(0); ret = PTR_ERR(root); goto out; } - if (root->reloc_root != reloc_root) { + if (WARN_ON(root->reloc_root != reloc_root)) { /* - * This is actually impossible without something - * going really wrong (like weird race condition - * or cosmic rays). + * This can happen if on-disk metadata has some + * corruption, e.g. bad reloc tree key offset. */ - ASSERT(0); ret = -EINVAL; goto out; }
[BUG] Syzbot reported a crash that an ASSERT() got triggered inside prepare_to_merge(). [CAUSE] The root cause of the triggered ASSERT() is we can have a race between quota tree creation and relocation. This leads us to create a duplicated quota tree in the btrfs_read_fs_root() path, and since it's treated as fs tree, it would have ROOT_SHAREABLE flag, causing us to create a reloc tree for it. The bug itself is fixed by a dedicated patch for it, but this already taught us the ASSERT() is not something straightforward for developers. [ENHANCEMENT] Instead of using an ASSERT(), let's handle it gracefully and output extra info about the mismatch reloc roots to help debug. Also with the above ASSERT() removed, we can trigger ASSERT(0)s inside merge_reloc_roots() later. Also replace those ASSERT(0)s with WARN_ON()s. Reported-by: syzbot+ae97a827ae1c3336bbb4@syzkaller.appspotmail.com Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/relocation.c | 44 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)