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.

When key returned from bch2_btree_and_journal_iter_peek is NULL it means
that btree topology needs repair. Print error message with position at
which node wasn't found and its parent node information. Call
bch2_topology_error and return error code returned by it to ensure that
topology error is handled properly.

Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
Suggested-by: Alan Huang <mmpgouride@gmail.com>
Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>
---

Notes:
    changes in v2:
        - make commit message more verbose.
        - set topology error, print error message and return
          appropriate error code.

    link to v1: https://lore.kernel.org/linux-bcachefs/20241023072024.98915-3-pZ010001011111@proton.me/

 fs/bcachefs/btree_iter.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
index 15ac72b1af51..40c824779b15 100644
--- a/fs/bcachefs/btree_iter.c
+++ b/fs/bcachefs/btree_iter.c
@@ -880,6 +880,18 @@ 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) {
+		struct printbuf buf = PRINTBUF;
+
+		prt_str(&buf, "node not found at pos ");
+		bch2_bpos_to_text(&buf, path->pos);
+		prt_str(&buf, " within parent node ");
+		bch2_bkey_val_to_text(&buf, c, bkey_i_to_s_c(&l->b->key));
+
+		ret = bch2_fs_topology_error(c, "%s", buf.buf);
+		printbuf_exit(&buf);
+		goto err;
+	}
 
 	bch2_bkey_buf_reassemble(out, c, k);
 
@@ -887,6 +899,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 Sat, Oct 26, 2024 at 05:46:33PM +0000, Piotr Zalewski 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.
> 
> When key returned from bch2_btree_and_journal_iter_peek is NULL it means
> that btree topology needs repair. Print error message with position at
> which node wasn't found and its parent node information. Call
> bch2_topology_error and return error code returned by it to ensure that
> topology error is handled properly.
> 
> Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> Suggested-by: Alan Huang <mmpgouride@gmail.com>
> Suggested-by: Kent Overstreet <kent.overstreet@linux.dev>
> Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>
> ---
> 
> Notes:
>     changes in v2:
>         - make commit message more verbose.
>         - set topology error, print error message and return
>           appropriate error code.
> 
>     link to v1: https://lore.kernel.org/linux-bcachefs/20241023072024.98915-3-pZ010001011111@proton.me/
> 
>  fs/bcachefs/btree_iter.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> index 15ac72b1af51..40c824779b15 100644
> --- a/fs/bcachefs/btree_iter.c
> +++ b/fs/bcachefs/btree_iter.c
> @@ -880,6 +880,18 @@ 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) {
> +		struct printbuf buf = PRINTBUF;
> +
> +		prt_str(&buf, "node not found at pos ");
> +		bch2_bpos_to_text(&buf, path->pos);
> +		prt_str(&buf, " within parent node ");
> +		bch2_bkey_val_to_text(&buf, c, bkey_i_to_s_c(&l->b->key));
> +
> +		ret = bch2_fs_topology_error(c, "%s", buf.buf);
> +		printbuf_exit(&buf);
> +		goto err;
> +	}

We'll want to add at least the btree ID and level to that.

Could you also look over the other places we report topology errors and
inconstencies for any commonality? btree_cache.c has some stuff, and I
think there's a helper in there that might give you the error message
you want (instead of just the btree node key), and I'd have a glance at
the topology error reporting in btree_update_interior.c and btree_gc.c
as well.

>  
>  	bch2_bkey_buf_reassemble(out, c, k);
>  
> @@ -887,6 +899,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 Sunday, October 27th, 2024 at 2:28 AM, Kent Overstreet <kent.overstreet@linux.dev> wrote:

> On Sat, Oct 26, 2024 at 05:46:33PM +0000, Piotr Zalewski 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.
> > 
> > When key returned from bch2_btree_and_journal_iter_peek is NULL it means
> > that btree topology needs repair. Print error message with position at
> > which node wasn't found and its parent node information. Call
> > bch2_topology_error and return error code returned by it to ensure that
> > topology error is handled properly.
> > 
> > Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > Suggested-by: Alan Huang mmpgouride@gmail.com
> > Suggested-by: Kent Overstreet kent.overstreet@linux.dev
> > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me
> > ---
> > 
> > Notes:
> > changes in v2:
> > - make commit message more verbose.
> > - set topology error, print error message and return
> > appropriate error code.
> > 
> > link to v1: https://lore.kernel.org/linux-bcachefs/20241023072024.98915-3-pZ010001011111@proton.me/
> > 
> > fs/bcachefs/btree_iter.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> > 
> > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > index 15ac72b1af51..40c824779b15 100644
> > --- a/fs/bcachefs/btree_iter.c
> > +++ b/fs/bcachefs/btree_iter.c
> > @@ -880,6 +880,18 @@ 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) {
> > + struct printbuf buf = PRINTBUF;
> > +
> > + prt_str(&buf, "node not found at pos ");
> > + bch2_bpos_to_text(&buf, path->pos);
> > + prt_str(&buf, " within parent node ");
> > + bch2_bkey_val_to_text(&buf, c, bkey_i_to_s_c(&l->b->key));
> > +
> > + ret = bch2_fs_topology_error(c, "%s", buf.buf);
> > + printbuf_exit(&buf);
> > + goto err;
> > + }
> 
> 
> We'll want to add at least the btree ID and level to that.
> 
> Could you also look over the other places we report topology errors and
> inconstencies for any commonality? btree_cache.c has some stuff, and I
> think there's a helper in there that might give you the error message
> you want (instead of just the btree node key), and I'd have a glance at
> the topology error reporting in btree_update_interior.c and btree_gc.c
> as well.

I scanned through mentioned files and some others. There are commonalities
but every error message seem to be different in some way so that there is 
no truly common part.

Information contained is usually a mixture of:
  - btree id and level.
  - position at which node wasn't found or if it was found but something 
    else is wrong its key info.
  - parent node key information.
  - prev/next node key info.
  - min/max key pos info.

Only appropriate helpers I found were (what I already used) - 
bch2_fs_topology_error and, in btree_iter.c, bch2_btree_path_to_text_short
but there it doesn't print full parent key info just min_key pos and parent
key pos so I'm not sure.

To what you already said I could also add min pos info.

> > bch2_bkey_buf_reassemble(out, c, k);
> > 
> > @@ -887,6 +899,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 Sun, Oct 27, 2024 at 11:34:02AM +0000, Piotr Zalewski wrote:
> On Sunday, October 27th, 2024 at 2:28 AM, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> > On Sat, Oct 26, 2024 at 05:46:33PM +0000, Piotr Zalewski 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.
> > > 
> > > When key returned from bch2_btree_and_journal_iter_peek is NULL it means
> > > that btree topology needs repair. Print error message with position at
> > > which node wasn't found and its parent node information. Call
> > > bch2_topology_error and return error code returned by it to ensure that
> > > topology error is handled properly.
> > > 
> > > Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > > Suggested-by: Alan Huang mmpgouride@gmail.com
> > > Suggested-by: Kent Overstreet kent.overstreet@linux.dev
> > > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me
> > > ---
> > > 
> > > Notes:
> > > changes in v2:
> > > - make commit message more verbose.
> > > - set topology error, print error message and return
> > > appropriate error code.
> > > 
> > > link to v1: https://lore.kernel.org/linux-bcachefs/20241023072024.98915-3-pZ010001011111@proton.me/
> > > 
> > > fs/bcachefs/btree_iter.c | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > > index 15ac72b1af51..40c824779b15 100644
> > > --- a/fs/bcachefs/btree_iter.c
> > > +++ b/fs/bcachefs/btree_iter.c
> > > @@ -880,6 +880,18 @@ 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) {
> > > + struct printbuf buf = PRINTBUF;
> > > +
> > > + prt_str(&buf, "node not found at pos ");
> > > + bch2_bpos_to_text(&buf, path->pos);
> > > + prt_str(&buf, " within parent node ");
> > > + bch2_bkey_val_to_text(&buf, c, bkey_i_to_s_c(&l->b->key));
> > > +
> > > + ret = bch2_fs_topology_error(c, "%s", buf.buf);
> > > + printbuf_exit(&buf);
> > > + goto err;
> > > + }
> > 
> > 
> > We'll want to add at least the btree ID and level to that.
> > 
> > Could you also look over the other places we report topology errors and
> > inconstencies for any commonality? btree_cache.c has some stuff, and I
> > think there's a helper in there that might give you the error message
> > you want (instead of just the btree node key), and I'd have a glance at
> > the topology error reporting in btree_update_interior.c and btree_gc.c
> > as well.
> 
> I scanned through mentioned files and some others. There are commonalities
> but every error message seem to be different in some way so that there is 
> no truly common part.
> 
> Information contained is usually a mixture of:
>   - btree id and level.
>   - position at which node wasn't found or if it was found but something 
>     else is wrong its key info.
>   - parent node key information.
>   - prev/next node key info.
>   - min/max key pos info.
> 
> Only appropriate helpers I found were (what I already used) - 
> bch2_fs_topology_error and, in btree_iter.c, bch2_btree_path_to_text_short
> but there it doesn't print full parent key info just min_key pos and parent
> key pos so I'm not sure.
> 
> To what you already said I could also add min pos info.

bch2_btree_pos_to_text()?

Sent with Proton Mail secure email.

On Sunday, October 27th, 2024 at 6:53 PM, Kent Overstreet <kent.overstreet@linux.dev> wrote:

> On Sun, Oct 27, 2024 at 11:34:02AM +0000, Piotr Zalewski wrote:
> 
> > On Sunday, October 27th, 2024 at 2:28 AM, Kent Overstreet kent.overstreet@linux.dev wrote:
> > 
> > > On Sat, Oct 26, 2024 at 05:46:33PM +0000, Piotr Zalewski 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.
> > > > 
> > > > When key returned from bch2_btree_and_journal_iter_peek is NULL it means
> > > > that btree topology needs repair. Print error message with position at
> > > > which node wasn't found and its parent node information. Call
> > > > bch2_topology_error and return error code returned by it to ensure that
> > > > topology error is handled properly.
> > > > 
> > > > Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > > > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > > > Suggested-by: Alan Huang mmpgouride@gmail.com
> > > > Suggested-by: Kent Overstreet kent.overstreet@linux.dev
> > > > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me
> > > > ---
> > > > 
> > > > Notes:
> > > > changes in v2:
> > > > - make commit message more verbose.
> > > > - set topology error, print error message and return
> > > > appropriate error code.
> > > > 
> > > > link to v1: https://lore.kernel.org/linux-bcachefs/20241023072024.98915-3-pZ010001011111@proton.me/
> > > > 
> > > > fs/bcachefs/btree_iter.c | 13 +++++++++++++
> > > > 1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > > > index 15ac72b1af51..40c824779b15 100644
> > > > --- a/fs/bcachefs/btree_iter.c
> > > > +++ b/fs/bcachefs/btree_iter.c
> > > > @@ -880,6 +880,18 @@ 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) {
> > > > + struct printbuf buf = PRINTBUF;
> > > > +
> > > > + prt_str(&buf, "node not found at pos ");
> > > > + bch2_bpos_to_text(&buf, path->pos);
> > > > + prt_str(&buf, " within parent node ");
> > > > + bch2_bkey_val_to_text(&buf, c, bkey_i_to_s_c(&l->b->key));
> > > > +
> > > > + ret = bch2_fs_topology_error(c, "%s", buf.buf);
> > > > + printbuf_exit(&buf);
> > > > + goto err;
> > > > + }
> > > 
> > > We'll want to add at least the btree ID and level to that.
> > > 
> > > Could you also look over the other places we report topology errors and
> > > inconstencies for any commonality? btree_cache.c has some stuff, and I
> > > think there's a helper in there that might give you the error message
> > > you want (instead of just the btree node key), and I'd have a glance at
> > > the topology error reporting in btree_update_interior.c and btree_gc.c
> > > as well.
> > 
> > I scanned through mentioned files and some others. There are commonalities
> > but every error message seem to be different in some way so that there is
> > no truly common part.
> > 
> > Information contained is usually a mixture of:
> > - btree id and level.
> > - position at which node wasn't found or if it was found but something
> > else is wrong its key info.
> > - parent node key information.
> > - prev/next node key info.
> > - min/max key pos info.
> > 
> > Only appropriate helpers I found were (what I already used) -
> > bch2_fs_topology_error and, in btree_iter.c, bch2_btree_path_to_text_short
> > but there it doesn't print full parent key info just min_key pos and parent
> > key pos so I'm not sure.
> > 
> > To what you already said I could also add min pos info.
> 
> bch2_btree_pos_to_text()?

Ah yes, I missed it - not used in many places and ending similar to 
bpos_to_text. I will print path->pos + bch2_btree_pos_to_text() in the next
version then.

On Sun, Oct 27, 2024 at 07:00:07PM +0000, Piotr Zalewski wrote:
> 
> 
> 
> 
> 
> Sent with Proton Mail secure email.
> 
> On Sunday, October 27th, 2024 at 6:53 PM, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> > On Sun, Oct 27, 2024 at 11:34:02AM +0000, Piotr Zalewski wrote:
> > 
> > > On Sunday, October 27th, 2024 at 2:28 AM, Kent Overstreet kent.overstreet@linux.dev wrote:
> > > 
> > > > On Sat, Oct 26, 2024 at 05:46:33PM +0000, Piotr Zalewski 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.
> > > > > 
> > > > > When key returned from bch2_btree_and_journal_iter_peek is NULL it means
> > > > > that btree topology needs repair. Print error message with position at
> > > > > which node wasn't found and its parent node information. Call
> > > > > bch2_topology_error and return error code returned by it to ensure that
> > > > > topology error is handled properly.
> > > > > 
> > > > > Reported-by: syzbot+005ef9aa519f30d97657@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> > > > > Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> > > > > Suggested-by: Alan Huang mmpgouride@gmail.com
> > > > > Suggested-by: Kent Overstreet kent.overstreet@linux.dev
> > > > > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me
> > > > > ---
> > > > > 
> > > > > Notes:
> > > > > changes in v2:
> > > > > - make commit message more verbose.
> > > > > - set topology error, print error message and return
> > > > > appropriate error code.
> > > > > 
> > > > > link to v1: https://lore.kernel.org/linux-bcachefs/20241023072024.98915-3-pZ010001011111@proton.me/
> > > > > 
> > > > > fs/bcachefs/btree_iter.c | 13 +++++++++++++
> > > > > 1 file changed, 13 insertions(+)
> > > > > 
> > > > > diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
> > > > > index 15ac72b1af51..40c824779b15 100644
> > > > > --- a/fs/bcachefs/btree_iter.c
> > > > > +++ b/fs/bcachefs/btree_iter.c
> > > > > @@ -880,6 +880,18 @@ 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) {
> > > > > + struct printbuf buf = PRINTBUF;
> > > > > +
> > > > > + prt_str(&buf, "node not found at pos ");
> > > > > + bch2_bpos_to_text(&buf, path->pos);
> > > > > + prt_str(&buf, " within parent node ");
> > > > > + bch2_bkey_val_to_text(&buf, c, bkey_i_to_s_c(&l->b->key));
> > > > > +
> > > > > + ret = bch2_fs_topology_error(c, "%s", buf.buf);
> > > > > + printbuf_exit(&buf);
> > > > > + goto err;
> > > > > + }
> > > > 
> > > > We'll want to add at least the btree ID and level to that.
> > > > 
> > > > Could you also look over the other places we report topology errors and
> > > > inconstencies for any commonality? btree_cache.c has some stuff, and I
> > > > think there's a helper in there that might give you the error message
> > > > you want (instead of just the btree node key), and I'd have a glance at
> > > > the topology error reporting in btree_update_interior.c and btree_gc.c
> > > > as well.
> > > 
> > > I scanned through mentioned files and some others. There are commonalities
> > > but every error message seem to be different in some way so that there is
> > > no truly common part.
> > > 
> > > Information contained is usually a mixture of:
> > > - btree id and level.
> > > - position at which node wasn't found or if it was found but something
> > > else is wrong its key info.
> > > - parent node key information.
> > > - prev/next node key info.
> > > - min/max key pos info.
> > > 
> > > Only appropriate helpers I found were (what I already used) -
> > > bch2_fs_topology_error and, in btree_iter.c, bch2_btree_path_to_text_short
> > > but there it doesn't print full parent key info just min_key pos and parent
> > > key pos so I'm not sure.
> > > 
> > > To what you already said I could also add min pos info.
> > 
> > bch2_btree_pos_to_text()?
> 
> Ah yes, I missed it - not used in many places and ending similar to 
> bpos_to_text. I will print path->pos + bch2_btree_pos_to_text() in the next
> version then.

yeah, keeping things organized is a never ending battle :)