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

[Kent Overstreet]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=005ef9aa519f30d97657
> Fixes: 5222a4607cd8 ("bcachefs: BTREE_ITER_WITH_JOURNAL")
> Suggested-by: Alan Huang <mmpgouride@xxxxxxxxx>
> Suggested-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> Signed-off-by: Piotr Zalewski <pZ010001011111@xxxxxxxxx>
> ---
> 
> 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@xxxxxxxxx/
> 
>  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
> 
>