Re: [PATCH] bcachefs: don't return early from __btree_err for bad or incompatible node read errors

[Bharadwaj Raju]

On Sun, Jun 15, 2025 at 5:49 AM Kent Overstreet
<kent.overstreet@xxxxxxxxx> wrote:
>
> On Sun, Jun 15, 2025 at 12:27:40AM +0530, Bharadwaj Raju wrote:
> > After cd3cdb1ef706 ("Single err message for btree node reads"),
> > all errors caused __btree_err to return BCH_ERR_fsck_fix no matter what
> > the actual error type was if the recovery pass was scanning for btree
> > nodes. This lead to the code continuing despite things like bad node
> > formats when they earlier would have caused a jump to fsck_err, because
> > btree_err only jumps when the return from __btree_err does not match
> > fsck_fix. Ultimately this lead to undefined behavior by attempting to
> > unpack a key based on an invalid format.
>
> Hang on, -BCH_ERR_fsck_fix should've caused us to fix fixable errors,
> not cause undefined behaviour.

-BCH_ERR_fsck_fix in btree_err (as _ret from __btree_err) prevents a jump
to fsck_err, so if that is the case, the code afterwards continues
as normal. That's where the UB in this bug comes from.

I think I should explain the path of the bug:
1. bch2_btree_node_read_done calls validate_bset, with a jump to
fsck_err if it returns an error.
2. validate_bset has btree_err_on(bch2_bkey_format_invalid(...), ...),
but in the bug case we were in btree-node-scan so __btree_err returns
fsck_fix, which means ret isn't modified and we don't jump to
fsck_err.
3. validate_bset doesn't return an error code, so
bch2_btree_node_read_done goes ahead and sets the invalid format --
and then UB happens when trying to unpack keys based on it.

> Or is the issue that we're returning -BCH_ERR_fsck_fix for non-fixable
> errors?
>
> Glancing at the code, I think the bug might not be limited to btree node
> scan; we now seem to be passing FSCK_CAN_FIX for all errors in the
> non-btree-node-scan case, and I don't think that's right for
> BCH_ERR_btree_node_read_err_must_retry cases.
>
> But I'll have to go digging through the git history to confirm that, and
> it sounds like you've already looked - does that sound like it?

Isn't the bch2_fsck_err_opt(c, FSCK_CAN_FIX, err_type) call only in
the node_read_err_fixable case?
In the must_retry case we return bad_node. But I'm not really familiar
with all this, so I think it'll be best if you do
take a look.

Thanks!