Re: [PATCH v2] xfs: bound logged region access in inode buffer recovery

[Dave Chinner]

On Tue, Sep 08, 2026 at 10:17:06AM +0800, Hongling Zeng wrote:
> 
> 在 2026年09月08日 05:36, Dave Chinner 写道:
> > On Mon, Sep 07, 2026 at 04:04:50PM +0800, Hongling Zeng wrote:
> > > xlog_recover_do_inode_buffer() reads the logged di_next_unlinked field
> > > from a log record buffer at a computed offset:
> > > 
> > >          logged_nextp = item->ri_buf[item_index].iov_base +
> > >                          next_unlinked_offset - reg_buf_offset;
> > >          *buffer_nextp = *logged_nextp;
> > > 
> > > The only protection against reading past the log record buffer are
> > > ASSERT()s, which compile away on non-DEBUG kernels.  The existing
> > > XFS_IS_CORRUPT(*logged_nextp == 0) check also dereferences the pointer
> > > before validating that the computed offset lies within the logged region.
> > > 
> > > A crafted log record can make the computed offset exceed iov_len, causing
> > > an out-of-bounds read from the log record buffer during inode buffer
> > > recovery.
> > > 
> > > Convert the relevant ASSERT-only checks into runtime corruption checks and
> > > verify that the logged di_next_unlinked field lies entirely within the log
> > > iovec before dereferencing it.
> > > 
> > > Fixes: 1094d3f12363 ("xfs: refactor log recovery buffer item dispatch for pass2 commit functions")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> > <sigh>
> > 
> > I'm going to say what I've said before again here, and it applies to
> > the BLF bitmap range checks patches you posted yesterday, too:
> > 
> > Hacking the same types of region size and range checks into every
> > log item type in an ad-hoc manner is not the right way to address
> > these log item verification issues.
> > 
> > We need to add a robust verification layer to the journal to verify
> > all the journal level metadata (e.g. ophdrs, transaction headers,
> > initial log item regions, per-log item type verification, etc) so
> > that we check *all* the journal items for sanity before we use them.
> > 
> > This is the same architecture we use for metadata (the verifier
> > layer) and it applies to the journal for the same reasons and
> > provides the same benefits (i.e. validate at first access, rest of
> > the code can assume validity and not have to clutter logic with
> > random validity checks to prevent bad behaviour.)
> > 
> > The high level design doc and rough plan I put together last time I
> > brought this up is in the patch below. If you're not willing or able
> > to spent time and tokens on fixing this entire class of problems for
> > everyone, then let please let me know ASAP.
> > 
> > -Dave.
>   Hi Dave,
> 
>   Thanks for the detailed feedback. I understand the concern: the ad-hoc
>   region checks I posted don't address log recovery validation
>   systematically.
> 
>   I'll withdraw the inode buffer and BLF bitmap patches and switch to
>   the design-driven approach. I'll post your patch 5 first (the
>   unchecked ri_buf[ri_cnt-1] in add_to_cont_trans(), the unbounded
>   kvrealloc() there, the missing oh_len alignment check in
>   process_data() - all reachable bugs, no restructuring), then work
>   through the rest of Phase 1.
> 
>   Three points to confirm before coding:

Don't take the design doc as being complete or correct - it's a
working document and really only serves as the initial high level
plan I fleshed out.

Indeed, I implemented a chunk of it yesterday afternoon (i.e.
before I saw your patch this morning) and my findings change quite a
bit of the generic infrastructure to make it handle the transaction
header without having to special case it. So from that perspective,
it's already out of date...

I'll post what I have in the series later this afternoon so you can
get up to speed.

>   1. I don't see a write-side guarantee that a region is split at most
>      once - xlog_write_partial() can emit multiple continuations. So
>      beyond your patch 2 treating the header as a normal accumulated
>      region, note that today a third fragment walks into
>      ri_buf[ri_cnt-1] with ri_cnt == 0 / ri_buf == NULL. Correct?

The iterator structure behind the item decoding should handle
decoding regions split into an arbitrary number of ophdr regions
without issue.

That's kinda the point of it; ensure the region is
fully extracted from the journal before we try to decode any of it.
Hence we have to handle CONTINUE/WAS_CONT regions as partial
additives until we get to the final WAS_CONT region that closes off
the region as a whole.

>   2. For the zero-length first fragment: if this is old-log
>      compatibility only,

Real thing, go look at xlog_write_get_iclog_space() and
consider what happens when the lv_chain passed to xlog_write() gets
it's first iclog with only space for two ophdrs left in it....

>      I'd document it that way and keep the special
>      handling as narrow as possible rather than introducing a generic
>      anonymous item state. Note the doc's ri_in_continuation has no
>      owner at that point since no item exists yet - narrow handling
>      avoids that entirely.

The special case handling for it is awful, and IMO needs to go away
because it is actually buggy and makes it much harder to reason
about what is a valid item in a transaction....

>   3. For testing I'll cover each phase with crafted-log images and run
>      xfstests.

This is why I have been rewriting xfs_logprint in rust: so it has
the same capabilities as xfs_db in terms of being able to walk,
parse and modify individual items in the journal. i.e. to be able to
explicitly fuzz the journal in a systematic, programmable and
reproducable way and hence avoid the need for hundreds of corrupted
images to test all the corner cases....

And, FWIW, the iterator + generic item handling design of the
validator for the kernel code is a fairly close translation of the
layered generic item type verification my new rust logprint code
already uses. 

>      My bigger worry is over-strict validation rejecting valid
>      logs, which only shows up replaying real crash logs - what
>      coverage do you expect there? And should the design doc go in
>      first so the series can reference it?

We have extensive journal recovery stress and validity tests in
fstests (e.g. the recoveryloop group), and so I'm really not worried
about structure verification being too strict. I'd much prefer to
err on the "extremely strict" side right now, and loosen if needed.
I don't think it will be an issue, because if the journal
verification comes across improperly formatted items then it points
to a runtime bug that needs to be fixed, not a journal recovery
issue.

i.e. I'd much prefer we have strict verification because that finds
logic bugs on both sides during testing, that way they do not end up
in production systems...

-Dave.

-- 
Dave Chinner
dgc@xxxxxxxxxx