fs/bcachefs/btree_iter.c | 3 +++ 1 file changed, 3 insertions(+)
Add NULL check for key returned from bch2_btree_and_journal_iter_peek in
btree_node_iter_and_journal_peek to avoid NULL ptr dereference in
bch2_bkey_buf_reassemble.
Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>
---
fs/bcachefs/btree_iter.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
index 0883cf6e1a3e..625167ce191f 100644
--- a/fs/bcachefs/btree_iter.c
+++ b/fs/bcachefs/btree_iter.c
@@ -882,6 +882,8 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
__bch2_btree_and_journal_iter_init_node_iter(trans, &jiter, l->b, l->iter, path->pos);
k = bch2_btree_and_journal_iter_peek(&jiter);
+ if (!k.k)
+ goto err;
bch2_bkey_buf_reassemble(out, c, k);
@@ -889,6 +891,7 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
c->opts.btree_node_prefetch)
ret = btree_path_prefetch_j(trans, path, &jiter);
+err:
bch2_btree_and_journal_iter_exit(&jiter);
return ret;
}
--
2.47.0On Oct 23, 2024, at 15:21, Piotr Zalewski <pZ010001011111@proton.me> wrote:
>
> Add NULL check for key returned from bch2_btree_and_journal_iter_peek in
> btree_node_iter_and_journal_peek to avoid NULL ptr dereference in
> bch2_bkey_buf_reassemble.
It would be helpful if the commit message explained why k.k is null in this case
>
> Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>
> ---
> fs/bcachefs/btree_iter.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> index 0883cf6e1a3e..625167ce191f 100644
> --- a/fs/bcachefs/btree_iter.c
> +++ b/fs/bcachefs/btree_iter.c
> @@ -882,6 +882,8 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> __bch2_btree_and_journal_iter_init_node_iter(trans, &jiter, l->b, l->iter, path->pos);
>
> k = bch2_btree_and_journal_iter_peek(&jiter);
> + if (!k.k)
> + goto err;
>
> bch2_bkey_buf_reassemble(out, c, k);
>
> @@ -889,6 +891,7 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> c->opts.btree_node_prefetch)
> ret = btree_path_prefetch_j(trans, path, &jiter);
>
> +err:
> bch2_btree_and_journal_iter_exit(&jiter);
> return ret;
> }
> --
> 2.47.0
>
>
>
On Wed, Oct 23, 2024 at 03:33:22PM +0800, Alan Huang wrote: > On Oct 23, 2024, at 15:21, Piotr Zalewski <pZ010001011111@proton.me> wrote: > > > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in > > bch2_bkey_buf_reassemble. > > It would be helpful if the commit message explained why k.k is null in this case This code is only for iterating over interior btree nodes - k.k is only null when we have a bad btree topology (gaps). Piotr, could you add a comment to that effect?
On Fri, Oct 25, 2024 at 08:11:50PM -0400, Kent Overstreet wrote: > On Wed, Oct 23, 2024 at 03:33:22PM +0800, Alan Huang wrote: > > On Oct 23, 2024, at 15:21, Piotr Zalewski <pZ010001011111@proton.me> wrote: > > > > > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in > > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in > > > bch2_bkey_buf_reassemble. > > > > It would be helpful if the commit message explained why k.k is null in this case > > This code is only for iterating over interior btree nodes - k.k is only > null when we have a bad btree topology (gaps). > > Piotr, could you add a comment to that effect? Actually, not just that - when this happens we should flag the filesystem as having topology repairs, and possibly start topology repair. Calling bch2_topology_error() will do that. We definitely want to log an error message, too; it should reference the btree node we're iterating over and explain that it's missing child nodes.
Sent with Proton Mail secure email. On Saturday, October 26th, 2024 at 2:16 AM, Kent Overstreet <kent.overstreet@linux.dev> wrote: > On Fri, Oct 25, 2024 at 08:11:50PM -0400, Kent Overstreet wrote: > > > On Wed, Oct 23, 2024 at 03:33:22PM +0800, Alan Huang wrote: > > > > > On Oct 23, 2024, at 15:21, Piotr Zalewski pZ010001011111@proton.me wrote: > > > > > > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in > > > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in > > > > bch2_bkey_buf_reassemble. > > > > > > It would be helpful if the commit message explained why k.k is null in this case > > > > This code is only for iterating over interior btree nodes - k.k is only > > null when we have a bad btree topology (gaps). > > > > Piotr, could you add a comment to that effect? > > > Actually, not just that - when this happens we should flag the > filesystem as having topology repairs, and possibly start topology > repair. > > Calling bch2_topology_error() will do that. > > We definitely want to log an error message, too; it should reference the > btree node we're iterating over and explain that it's missing child > nodes. Thanks for the clarification. I will send v2 tomorrow :)
On Sat, Oct 26, 2024 at 12:23:40AM +0000, Piotr Zalewski wrote: > > > > > > Sent with Proton Mail secure email. > > On Saturday, October 26th, 2024 at 2:16 AM, Kent Overstreet <kent.overstreet@linux.dev> wrote: > > > On Fri, Oct 25, 2024 at 08:11:50PM -0400, Kent Overstreet wrote: > > > > > On Wed, Oct 23, 2024 at 03:33:22PM +0800, Alan Huang wrote: > > > > > > > On Oct 23, 2024, at 15:21, Piotr Zalewski pZ010001011111@proton.me wrote: > > > > > > > > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in > > > > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in > > > > > bch2_bkey_buf_reassemble. > > > > > > > > It would be helpful if the commit message explained why k.k is null in this case > > > > > > This code is only for iterating over interior btree nodes - k.k is only > > > null when we have a bad btree topology (gaps). > > > > > > Piotr, could you add a comment to that effect? > > > > > > Actually, not just that - when this happens we should flag the > > filesystem as having topology repairs, and possibly start topology > > repair. > > > > Calling bch2_topology_error() will do that. > > > > We definitely want to log an error message, too; it should reference the > > btree node we're iterating over and explain that it's missing child > > nodes. > > Thanks for the clarification. I will send v2 tomorrow :) Also, make sure we're returning an error code - your patch didn't do that, we (obviously) can't continue the btree lookup. bch2_topology_error() will give you the error code you want; the error code will tell recovery to rewind and run topology repair (if we're in recovery) or else something otherwise sensible.
Hi Alan,
On Wednesday, October 23rd, 2024 at 9:33 AM, Alan Huang <mmpgouride@gmail.com> wrote:
> On Oct 23, 2024, at 15:21, Piotr Zalewski pZ010001011111@proton.me wrote:
>
> > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in
> > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in
> > bch2_bkey_buf_reassemble.
>
>
> It would be helpful if the commit message explained why k.k is null in this case
I will debug this more thoroughly and provide details. For now I see that
during GC it sees journal replay hasn't finished but journal turns out to
be empty? Which seems weird, so maybe underlying issue should be solved on
a deeper level.
Log from the reproducer is:
```
[ 27.391332] bcachefs (loop0): accounting_read... done
[ 27.408141] bcachefs (loop0): alloc_read... done
[ 27.409118] bcachefs (loop0): stripes_read... done
[ 27.410059] bcachefs (loop0): snapshots_read... done
[ 27.411161] bcachefs (loop0): check_allocations...
[ 27.415003] bucket 0:26 data type btree ptr gen 0 missing in alloc btree
[ 27.415024] while marking u64s 11 type btree_ptr_v2 SPOS_MAX len 0 ver 0: seq ac62141f8dc7e261 written 24 min_kg
[ 27.422560] bucket 0:38 data type btree ptr gen 0 missing in alloc btree
[ 27.422571] while marking u64s 11 type btree_ptr_v2 SPOS_MAX len 0 ver 0: seq 7589ab5e0c11cc7a written 24 min_kg
[ 27.428033] bucket 0:41 data type btree ptr gen 0 missing in alloc btree
[ 27.428042] while marking u64s 11 type btree_ptr_v2 SPOS_MAX len 0 ver 0: seq 9aa2895aefce4bdf written 24 min_kg
[ 27.432315] bcachefs (loop0): btree topology error: attempting to get btree node with non-btree key u64s 0 type
[ 27.435343] bcachefs (loop0): inconsistency detected - emergency read only at journal seq 10
[ 27.436947] bcachefs (loop0): bch2_gc_btree(): error btree_need_topology_repair
[ 27.438756] btree node read error for xattrs, shutting down
[ 27.440081] bcachefs (loop0): bch2_gc_btrees(): error fsck_errors_not_fixed
[ 27.441915] bcachefs (loop0): bch2_check_allocations(): error fsck_errors_not_fixed
[ 27.443349] bcachefs (loop0): bch2_fs_recovery(): error fsck_errors_not_fixed
[ 27.444802] bcachefs (loop0): bch2_fs_start(): error starting filesystem fsck_errors_not_fixed
[ 27.446270] bcachefs (loop0): shutting down
[ 27.456042] bcachefs (loop0): shutdown complete
[ 27.835683] bcachefs: bch2_fs_get_tree() error: fsck_errors_not_fixed
```
> > Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me
> > ---
> > fs/bcachefs/btree_iter.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > index 0883cf6e1a3e..625167ce191f 100644
> > --- a/fs/bcachefs/btree_iter.c
> > +++ b/fs/bcachefs/btree_iter.c
> > @@ -882,6 +882,8 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> > __bch2_btree_and_journal_iter_init_node_iter(trans, &jiter, l->b, l->iter, path->pos);
> >
> > k = bch2_btree_and_journal_iter_peek(&jiter);
> > + if (!k.k)
> > + goto err;
> >
> > bch2_bkey_buf_reassemble(out, c, k);
> >
> > @@ -889,6 +891,7 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> > c->opts.btree_node_prefetch)
> > ret = btree_path_prefetch_j(trans, path, &jiter);
> >
> > +err:
> > bch2_btree_and_journal_iter_exit(&jiter);
> > return ret;
> > }
> > --
> > 2.47.0
Best regards, Piotr Zalewski
Hi Alan and all,
On Wednesday, October 23rd, 2024 at 10:06 AM, Piotr Zalewski <pZ010001011111@proton.me> wrote:
> Hi Alan,
>
> On Wednesday, October 23rd, 2024 at 9:33 AM, Alan Huang mmpgouride@gmail.com wrote:
>
> > On Oct 23, 2024, at 15:21, Piotr Zalewski pZ010001011111@proton.me wrote:
> >
> > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in
> > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in
> > > bch2_bkey_buf_reassemble.
> >
> > It would be helpful if the commit message explained why k.k is null in this case
>
>
> I will debug this more thoroughly and provide details. For now I see that
> during GC it sees journal replay hasn't finished but journal turns out to
> be empty? Which seems weird, so maybe underlying issue should be solved on
> a deeper level.
>
There is a clean shutdown detected during recovery. Journal has no entries
as it was confirmed by debugging bch2_fs_journal_start. seq and seq_ondisk
members are equal so journal is "quiesced". Also, journal_keys size is 0.
So in check_allocations when keys are being marked for GC journal replay is
not done so it peeks into journal and there is nothing.
Now, maybe in case journal is empty during start, replay done should be set
already in bch2_fs_journal_start? I tested it and it fixed the issue as well.
```
diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 2cf8f24d50cc..67b342d23346 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1287,6 +1287,9 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
spin_lock(&j->lock);
set_bit(JOURNAL_running, &j->flags);
+ if (!had_entries) {
+ set_bit(JOURNAL_replay_done, &j->flags);
+ }
j->last_flush_write = jiffies;
j->reservations.idx = j->reservations.unwritten_idx = journal_cur_seq(j);
```
(But at the same time there is a check for whether there were entries just
above the code which sets journal running so it seems unlikely that if it's
correct approach it's not yet there).
(In need of some pointer/explanation)
>
> > > Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me
> > > ---
> > > fs/bcachefs/btree_iter.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > > index 0883cf6e1a3e..625167ce191f 100644
> > > --- a/fs/bcachefs/btree_iter.c
> > > +++ b/fs/bcachefs/btree_iter.c
> > > @@ -882,6 +882,8 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> > > __bch2_btree_and_journal_iter_init_node_iter(trans, &jiter, l->b, l->iter, path->pos);
> > >
> > > k = bch2_btree_and_journal_iter_peek(&jiter);
> > > + if (!k.k)
> > > + goto err;
> > >
> > > bch2_bkey_buf_reassemble(out, c, k);
> > >
> > > @@ -889,6 +891,7 @@ static noinline int btree_node_iter_and_journal_peek(struct btree_trans *trans,
> > > c->opts.btree_node_prefetch)
> > > ret = btree_path_prefetch_j(trans, path, &jiter);
> > >
> > > +err:
> > > bch2_btree_and_journal_iter_exit(&jiter);
> > > return ret;
> > > }
> > > --
> > > 2.47.0
>
Best regards, Piotr Zalewski
On Fri, Oct 25, 2024 at 11:39:38PM +0000, Piotr Zalewski wrote: > Hi Alan and all, > > On Wednesday, October 23rd, 2024 at 10:06 AM, Piotr Zalewski <pZ010001011111@proton.me> wrote: > > > Hi Alan, > > > > On Wednesday, October 23rd, 2024 at 9:33 AM, Alan Huang mmpgouride@gmail.com wrote: > > > > > On Oct 23, 2024, at 15:21, Piotr Zalewski pZ010001011111@proton.me wrote: > > > > > > > Add NULL check for key returned from bch2_btree_and_journal_iter_peek in > > > > btree_node_iter_and_journal_peek to avoid NULL ptr dereference in > > > > bch2_bkey_buf_reassemble. > > > > > > It would be helpful if the commit message explained why k.k is null in this case > > > > > > I will debug this more thoroughly and provide details. For now I see that > > during GC it sees journal replay hasn't finished but journal turns out to > > be empty? Which seems weird, so maybe underlying issue should be solved on > > a deeper level. > > > > There is a clean shutdown detected during recovery. Journal has no entries > as it was confirmed by debugging bch2_fs_journal_start. seq and seq_ondisk > members are equal so journal is "quiesced". Also, journal_keys size is 0. > > So in check_allocations when keys are being marked for GC journal replay is > not done so it peeks into journal and there is nothing. > > Now, maybe in case journal is empty during start, replay done should be set > already in bch2_fs_journal_start? I tested it and it fixed the issue as well. this is "btree_and_journal_iter_peek()" - it's iterating over the btree node and overlaying keys from the journal, so the journal being empty is fine. It's because syzbot is feeding us filesystem images that are broken in interesting ways :)
© 2016 - 2024 Red Hat, Inc.