Re: [PATCH v2] xfs: bound logged region access in inode buffer recovery
From: Hongling Zeng
Date: Tue Sep 08 2026 - 03:54:50 EST
在 2026年09月08日 14:33, Dave Chinner 写道:
On Tue, Sep 08, 2026 at 10:17:06AM +0800, Hongling Zeng wrote:Hi Dave,
在 2026年09月08日 05:36, Dave Chinner 写道:Don't take the design doc as being complete or correct - it's a
On Mon, Sep 07, 2026 at 04:04:50PM +0800, Hongling Zeng wrote:Hi Dave,
xlog_recover_do_inode_buffer() reads the logged di_next_unlinked field<sigh>
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>
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.
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:
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 mostThe iterator structure behind the item decoding should handle
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?
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-logReal thing, go look at xlog_write_get_iclog_space() and
compatibility only,
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 specialThe special case handling for it is awful, and IMO needs to go away
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.
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 runThis is why I have been rewriting xfs_logprint in rust: so it has
xfstests.
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 validWe have extensive journal recovery stress and validity tests in
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?
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.
Understood - I'll treat the doc as a working draft and wait for your series rather than building on the plan as posted.
Thanks for the answers. On Q2 I mis-derived "old-log only" from
xlog_write_partial()'s refill check - the start record being its own
empty ophdr in xlog_cil_build_trans_hdr() makes the point, and the
pre-refactor writer could emit the zero-length trans header fragment
at the iclog boundary outright. Accepted, and agreed that special
case needs to go.
Once your series is posted I'll start with review and the
recoveryloop / logprint-based testing, then take the per-type
validate_region() / validate_item() implementations on top of your
iterator.
Thanks,
Hongling