[PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Piotr Zalewski]

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.0

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Alan Huang]

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

> 
> 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
> 
> 
> 

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Piotr Zalewski]

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

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[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

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Kent Overstreet]

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?

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Kent Overstreet]

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 :)

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Kent Overstreet]

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.

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Piotr Zalewski]

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 :)

Re: [PATCH] bcachefs: Fix NULL ptr dereference in btree_node_iter_and_journal_peek

[Kent Overstreet]

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.