Re: [PATCH] xfs: fail recovery on a committed log item with no regions

[Christoph Hellwig <hch@infradead.org>]

On Mon, Jun 29, 2026 at 09:24:53AM -0700, Weiming Shi wrote:
> xlog_recover_add_to_trans() turns the first op of a transaction into a
> recovery item.  If that op is a bare transaction header
> (len == sizeof(struct xfs_trans_header)), xlog_recover_add_item() adds
> an item but attaches no region, so it sits on r_itemq with ri_cnt == 0
> and ri_buf == NULL.
> 
> When the following op is a commit, xlog_recover_reorder_trans() runs
> ITEM_TYPE() on each item to look up its ops vector.  ITEM_TYPE() reads
> *(unsigned short *)item->ri_buf[0].iov_base, which faults on the
> NULL ri_buf.  The commit handlers dereference ri_buf[0] too, so reorder
> is the first place that trips over it.
> 
> This is reachable at mount time from a crafted image whose log holds the
> op sequence START_TRANS / bare-header / COMMIT_TRANS:

Given that the log is checksummed this vould be a maliciously
tweaked image and not a corruption.  It would be good to state
this thread model.

>  
> +		/* corrupt log: an item with no regions has a NULL ri_buf */
> +		if (!item->ri_cnt || !item->ri_buf) {
> +			xfs_warn(log->l_mp,
> +				"%s: committed log item has no regions",
> +				__func__);
> +			ASSERT(0);
> +			if (!list_empty(&sort_list))
> +				list_splice_init(&sort_list, &trans->r_itemq);
> +			error = -EFSCORRUPTED;
> +			break;
> +		}

That being said, handling this sounds fine, but duplicating the
code for the listing splicing feels a bit annoying.

Also I think the ASSERT(0) for on-disk corruption might be a bit
backwards these days with all the fuzzing and injecting.

So maybe as a prep-patch remove the existing ASSERT(0), then as
a second prep patch move the list splicing after the loop:

	if (error) {
		/*
		 * Return the remaining items back to the transaction item list
		 * so they can be freed in caller.
                 */
		if (!list_empty(&sort_list))
			list_splice_init(&sort_list, &trans->r_itemq);
	} else {
		ASSERT(list_empty(&sort_list));
	}

and then add the new check.