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

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

[Hongling Zeng]

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@vger.kernel.org
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
Change in v2:
 --Rebase top of for-next and re-send.
---
 fs/xfs/xfs_buf_item_recover.c | 40 ++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_buf_item_recover.c b/fs/xfs/xfs_buf_item_recover.c
index 7148716366ba..8e06a68db310 100644
--- a/fs/xfs/xfs_buf_item_recover.c
+++ b/fs/xfs/xfs_buf_item_recover.c
@@ -651,6 +651,9 @@ xlog_recover_do_inode_buffer(
 	int				inodes_per_buf;
 	xfs_agino_t			*logged_nextp;
 	xfs_agino_t			*buffer_nextp;
+	size_t				buf_size;
+	size_t				iov_len;
+	size_t				rel_off;
 
 	trace_xfs_log_recover_buf_inode_buf(mp->m_log, buf_f);
 
@@ -714,17 +717,48 @@ xlog_recover_do_inode_buffer(
 			return -EFSCORRUPTED;
 		}
 
+		buf_size = BBTOB(bp->b_length);
+		if (XFS_IS_CORRUPT(mp, reg_buf_bytes > buf_size ||
+			   reg_buf_offset > buf_size - reg_buf_bytes)) {
+			xfs_alert(mp,
+	"Bad inode buffer log bitmap region (off %d, len %d, buf_size %zu).",
+				reg_buf_offset, reg_buf_bytes, buf_size);
+			return -EFSCORRUPTED;
+		}
+
+		if (XFS_IS_CORRUPT(mp,
+				item->ri_buf[item_index].iov_base == NULL)) {
+			xfs_alert(mp, "NULL inode buffer log record.");
+			return -EFSCORRUPTED;
+		}
+
+		iov_len = item->ri_buf[item_index].iov_len;
+		if (XFS_IS_CORRUPT(mp, iov_len < reg_buf_bytes)) {
+			xfs_alert(mp,
+	"Bad inode buffer log record length (iov_len %zu, region len %d).",
+				iov_len, reg_buf_bytes);
+			return -EFSCORRUPTED;
+		}
+
 		ASSERT(item->ri_buf[item_index].iov_base != NULL);
 		ASSERT((item->ri_buf[item_index].iov_len % XFS_BLF_CHUNK) == 0);
 		ASSERT((reg_buf_offset + reg_buf_bytes) <= BBTOB(bp->b_length));
 
+		rel_off = next_unlinked_offset - reg_buf_offset;
+		if (XFS_IS_CORRUPT(mp, rel_off > iov_len ||
+			   sizeof(xfs_agino_t) > iov_len - rel_off)) {
+			xfs_alert(mp,
+	"Bad inode buffer log record offset (rel_off %zu, iov_len %zu).",
+				rel_off, iov_len);
+			return -EFSCORRUPTED;
+		}
+
 		/*
 		 * The current logged region contains a copy of the
 		 * current di_next_unlinked field.  Extract its value
-		 * and copy it to the buffer copy.
+		 * and copy it to the on disk inode buffer.
 		 */
-		logged_nextp = item->ri_buf[item_index].iov_base +
-				next_unlinked_offset - reg_buf_offset;
+		logged_nextp = item->ri_buf[item_index].iov_base + rel_off;
 		if (XFS_IS_CORRUPT(mp, *logged_nextp == 0)) {
 			xfs_alert(mp,
 		"Bad inode buffer log record (ptr = "PTR_FMT", bp = "PTR_FMT"). "
-- 
2.25.1

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

[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@vger.kernel.org
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

<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.
-- 
Dave Chinner
dgc@kernel.org

xfs: add log recovery item validation design document

Add a design document describing a systematic approach to validating log
items during journal recovery. Log item data read from the journal cannot
be fully trusted - corruption from torn writes, hardware errors or
software bugs can produce invalid type codes, wrong region counts,
truncated regions or internally inconsistent format structures that the
current recovery code largely uses unchecked.

The document describes a three-layer validation scheme (generic region
header decode, per-region type-specific validation, and full cross-region
item validation), the treatment of the transaction header as a validated
region type, and a set of additional safety fixes for the generic region
assembly code. It also lays out a phased implementation plan.

Assisted-by: LLM
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 Documentation/filesystems/xfs/index.rst            |   1 +
 .../xfs/xfs-log-recovery-validation-design.rst     | 477 +++++++++++++++++++++
 2 files changed, 478 insertions(+)

diff --git a/Documentation/filesystems/xfs/index.rst b/Documentation/filesystems/xfs/index.rst
index ab66c57a5d18..231c1382b27a 100644
--- a/Documentation/filesystems/xfs/index.rst
+++ b/Documentation/filesystems/xfs/index.rst
@@ -9,6 +9,7 @@ XFS Filesystem Documentation
    :numbered:
 
    xfs-delayed-logging-design
+   xfs-log-recovery-validation-design
    xfs-maintainer-entry-profile
    xfs-self-describing-metadata
    xfs-online-fsck-design
diff --git a/Documentation/filesystems/xfs/xfs-log-recovery-validation-design.rst b/Documentation/filesystems/xfs/xfs-log-recovery-validation-design.rst
new file mode 100644
index 000000000000..188b1bebf171
--- /dev/null
+++ b/Documentation/filesystems/xfs/xfs-log-recovery-validation-design.rst
@@ -0,0 +1,477 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. _xfs_log_recovery_validation:
+
+===================================
+XFS Log Recovery Item Validation
+===================================
+
+Problem
+=======
+
+When recovering the journal, ``xlog_recover_add_to_trans()`` decodes ophdr
+regions to rebuild log items from the journal data. The first 4 bytes of
+each new item's first region are used to determine the item type (2 bytes)
+and region count (2 bytes). These values drive memory allocation, region
+accumulation, and later the ``commit_pass1``/``commit_pass2`` handlers cast
+the accumulated region data to type-specific format structures and use fields
+from them to drive buffer reads, inode updates, and intent replay.
+
+The transaction header (``struct xfs_trans_header``) is also decoded inline
+in ``add_to_trans`` with its own bespoke validation (magic number check, max
+length check). It does not handle the zero-length first fragment case
+that occurs when the iclog has exactly enough space for the start record
+ophdr plus one more ophdr but no data — in that case ``add_to_trans``
+silently returns at the ``len == 0`` check before ever reaching the header
+parsing code.
+
+The problem is that this data comes from the journal and cannot be fully
+trusted. Corruption — whether from torn writes, hardware errors, or
+software bugs — can produce invalid type codes, wrong region counts,
+truncated regions, or internally inconsistent format structures. The
+current code has minimal validation:
+
+- ``ilf_size`` is checked against 0 and ``XLOG_MAX_REGIONS_IN_ITEM``
+- ``oh_len`` is checked against the log record boundary
+- The transaction header checks magic and max length but not ``len == 0``
+- Some commit handlers check individual field sizes
+
+However, there is no systematic validation, and many commit handlers cast
+``ri_buf[N].iov_base`` to format structures without checking
+``ri_cnt >= N+1`` or ``iov_len >= sizeof(format_struct)``. This risks
+crashes, buffer overruns, and use of garbage data to drive disk I/O during
+recovery.
+
+Design
+======
+
+Add three layers of validation, each catching problems at the earliest
+possible point.
+
+Layer 1: Region header decode (generic)
+---------------------------------------
+
+When: In ``xlog_recover_add_to_trans()`` when ``ri_total == 0`` (first
+region of a new item).
+
+Currently the code reads ``ilf_size`` from the region data to set
+``ri_total``, and the item type is not looked up until much later in
+``xlog_recover_reorder_trans()``. The transaction header is handled as a
+special case with inline validation. Move all first-region validation
+earlier and make it uniform:
+
+a) Validate ``len >= 4`` (minimum to read type + size fields). All log
+   regions are 32-bit aligned, so the minimum fragment of any region
+   is 4 bytes. A first fragment smaller than 4 bytes is corruption.
+   Note: ``len == 0`` is valid for the transaction header when the iclog
+   has exactly enough space for the start ophdr plus one more ophdr
+   but no data — this must be handled as a continuation (see below).
+b) Read the item type from the first 2 bytes
+c) Look up the item ops via ``xlog_find_item_ops()``
+d) If the type is unknown, reject immediately with ``-EFSCORRUPTED``
+e) Store ``item->ri_ops`` at this point (currently done in
+   ``reorder_trans``)
+f) Validate ``ilf_size`` against ``ops->min_regions`` and
+   ``ops->max_regions``
+g) Validate ``len >= ops->min_hdr_len`` (the minimum format header size)
+
+Transaction header as a validated type
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The transaction header (``struct xfs_trans_header``) is currently decoded
+inline in ``add_to_trans`` with bespoke magic number and length checks, and
+its continuation is handled separately in ``add_to_cont_trans``. Instead,
+treat it as a regular validated type within the same framework:
+
+- Add a pseudo item type (e.g. ``XFS_LI_TRANS_HDR`` or use the magic
+  number ``XFS_TRANS_HEADER_MAGIC`` as the type code) with its own
+  ``xlog_recover_item_ops`` entry.
+- ``min_regions = 1``, ``max_regions = 1``
+- ``min_hdr_len = sizeof(struct xfs_trans_header)``
+- ``validate_region`` checks ``iov_len == sizeof(struct xfs_trans_header)``
+  and the magic number
+- No ``commit_pass1``/``commit_pass2`` callbacks — after validation, the
+  decoded transaction header is copied to ``trans->r_theader`` as before
+
+This eliminates the special-case parsing in ``add_to_trans`` and
+``add_to_cont_trans`` for the transaction header. The zero-length first
+fragment case is handled uniformly: it arrives as a continuation
+(``oh_len == 0``, ``XLOG_CONTINUE_TRANS`` set), and the continuation
+infrastructure assembles the complete region before ``validate_region``
+checks it.
+
+The ``ilf_size`` field of the transaction header format is a bit different
+— ``xfs_trans_header`` uses ``th_num_items`` rather than the generic
+``ilf_size`` at offset 2. Since the transaction header always has exactly
+1 region, we don't need to read ``ilf_size`` from it. The
+``ops->min_regions == ops->max_regions == 1`` is sufficient.
+
+Alternatively, the transaction header could be handled without a full
+ops entry by having the generic code recognise it as a special case
+at step (b) and apply its fixed constraints directly. Either approach
+works; the ops entry is cleaner but the special case is simpler.
+
+New fields in ``struct xlog_recover_item_ops``::
+
+    uint16_t    min_regions;    /* minimum valid ri_total */
+    uint16_t    max_regions;    /* maximum valid ri_total */
+    uint16_t    min_hdr_len;    /* minimum ri_buf[0].iov_len */
+
+These are compile-time constants per item type. Examples:
+
+===========  ===========  ===========  ==============================
+Item Type    min_regions  max_regions  min_hdr_len
+===========  ===========  ===========  ==============================
+TRANS_HDR    1            1            sizeof(xfs_trans_header)
+BUF          2            XLOG_MAX..   sizeof(xfs_buf_log_format)
+INODE        2            4            sizeof(xfs_inode_log_format)
+DQUOT        2            2            sizeof(xfs_dq_logformat)
+EFI          1            1            sizeof(xfs_efi_log_format)
+EFD          1            1            sizeof(xfs_efd_log_format)
+RUI          1            1            sizeof(xfs_rui_log_format)
+RUD          1            1            sizeof(xfs_rud_log_format)
+CUI          1            1            sizeof(xfs_cui_log_format)
+CUD          1            1            sizeof(xfs_cud_log_format)
+BUI          1            1            sizeof(xfs_bui_log_format)
+BUD          1            1            sizeof(xfs_bud_log_format)
+ATTRI        2            5            sizeof(xfs_attri_log_format)
+ATTRD        1            1            sizeof(xfs_attrd_log_format)
+XMI          1            1            sizeof(xfs_xmi_log_format)
+XMD          1            1            sizeof(xfs_xmd_log_format)
+ICREATE      1            1            sizeof(xfs_icreate_log)
+QUOTAOFF     1            1            sizeof(xfs_qoff_logformat)
+===========  ===========  ===========  ==============================
+
+(RT variants same as their non-RT counterparts.)
+
+Handling first region split across continuations
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+All log regions are 32-bit aligned, so the minimum first fragment size
+is 4 bytes — enough to read both the type and size fields. If ``len < 4``
+on a non-continuation ophdr, this is unconditionally corrupt.
+
+The exception is the transaction header: when the iclog has exactly
+enough space for the ``XLOG_START_TRANS`` ophdr plus one more ophdr but no
+data, ``oh_len == 0`` for the transaction header ophdr and the data arrives
+entirely via continuation. This is a valid zero-length first fragment
+that the generic code must handle:
+
+- When ``len == 0`` and this is the first region of a new item (``r_itemq``
+  is empty or the current item is full), we cannot read the type.
+  Since only the transaction header can legitimately have ``len == 0``
+  at this point, and it will always arrive as the first item in the
+  transaction, we can identify this case by checking that the
+  transaction's item list is empty (i.e. we haven't seen the
+  transaction header yet). Set ``ri_in_continuation`` and defer all
+  validation to when the continuation completes the region.
+
+- For all other items (non-empty ``r_itemq``), ``len == 0`` on a new region
+  is corruption.
+
+When the first region is split but ``len >= 4`` (the common continuation
+case for log items), the ops lookup and ``ilf_size`` validation can be done
+immediately. The ``min_hdr_len`` check is deferred until the continuation
+completes the region, at which point ``validate_region`` runs on the
+complete ``ri_buf[0]``.
+
+The continuation path (``add_to_cont_trans``) uses ``kvrealloc`` to grow
+the region buffer. Since ``ri_ops`` is set (or will be set when the
+continuation completes for the deferred transaction header case), the
+accumulated size (``old_len + len``) can be bounds-checked against a
+type-specific maximum for the current region index before doing the
+realloc.
+
+The BUF item is special: it always has at least 2 regions (the format
+header and at least one data region), but the format header's size is
+variable because it contains an inline bitmap. The ``min_hdr_len`` should
+be the fixed portion of ``xfs_buf_log_format`` (without the bitmap), and
+per-region validation (layer 2) checks the full header size including
+the bitmap.
+
+Layer 2: Per-region validation (type-specific)
+----------------------------------------------
+
+When: In ``xlog_recover_add_to_trans()`` after each region is added to the
+item (after ``ri_cnt`` is incremented), and in
+``xlog_recover_add_to_cont_trans()`` after a continuation region is
+appended.
+
+New callback in ``struct xlog_recover_item_ops``::
+
+    int (*validate_region)(struct xlog *log,
+                           struct xlog_recover_item *item,
+                           int region_index);
+
+Called after the region at ``region_index`` has been fully assembled.
+
+For regions that arrive complete in a single ophdr, ``validate_region`` is
+called from ``xlog_recover_add_to_trans()`` immediately after the region
+is added.
+
+For regions that are split across op records (continuations), the region
+is built incrementally by ``xlog_recover_add_to_cont_trans()`` which
+reallocates and appends data. Two levels of validation apply:
+
+a) Before the realloc in ``add_to_cont_trans``: bounds check the
+   accumulated size (``old_len + len``) against the type-specific maximum
+   for the current region index. This uses the ``ops->max_region_size``
+   field or a simple per-type upper bound to prevent unbounded memory
+   allocation from a corrupt continuation stream.
+
+b) After the continuation region is complete: call ``validate_region`` to
+   do the full type-specific validation. A continuation is complete
+   when the next non-continuation ophdr arrives (either a new region
+   via ``add_to_trans`` or a commit via ``xlog_recover_commit_trans``).
+
+   To track this, add a boolean ``ri_in_continuation`` flag to
+   ``struct xlog_recover_item``. Set it in ``add_to_cont_trans`` when data
+   is appended. When ``add_to_trans`` is next called and the tail item has
+   ``ri_in_continuation`` set, the previous region was completed by the
+   continuation — call ``validate_region`` for it (at ``ri_cnt - 1``) and
+   clear the flag before proceeding with the new region. Similarly,
+   when ``xlog_recover_commit_trans`` is called, check the tail item for
+   ``ri_in_continuation`` and validate the final region if needed.
+
+Each item type implements ``validate_region`` to check size bounds:
+
+INODE
+    | region 0: ``iov_len == sizeof(xfs_inode_log_format)`` or
+      ``iov_len == sizeof(xfs_inode_log_format_32)``
+    | region 1: ``iov_len >= sizeof(xfs_dinode)``,
+      ``iov_len <= xfs_log_dinode_size(mp)``
+    | region 2: ``iov_len >= 0`` (data fork, variable size),
+      ``iov_len <= XFS_DFORK_DSIZE(...)`` (need inode core to check)
+    | region 3: ``iov_len >= 0`` (attr fork, variable size),
+      ``iov_len <= XFS_DFORK_ASIZE(...)``
+
+BUF
+    | region 0: ``iov_len >= sizeof(xfs_buf_log_format)`` base size,
+      ``blf_map_size`` is consistent with ``iov_len``
+    | region 1+: data regions, ``iov_len > 0``,
+      ``iov_len <= blf_len * BBSIZE`` (can't exceed buffer size),
+      ``iov_len % XFS_BLF_CHUNK == 0``
+
+DQUOT
+    | region 0: ``iov_len == sizeof(xfs_dq_logformat)``
+    | region 1: ``iov_len >= sizeof(xfs_disk_dquot)``
+
+EFI/RUI/CUI/BUI (single-region intent items)
+    | region 0: ``iov_len ==`` calculated size based on ``nextents`` field,
+      ``nextents >= 1``, ``nextents <=`` type-specific maximum
+
+EFD/RUD/CUD/BUD/ATTRD/XMD (single-region done items)
+    | region 0: ``iov_len == sizeof(format_struct)``
+
+ATTRI
+    | region 0: ``iov_len == sizeof(xfs_attri_log_format)``
+    | region 1+: name/value regions, sizes bounded by
+      ``XATTR_NAME_MAX``, ``XATTR_SIZE_MAX``
+
+XMI
+    | region 0: ``iov_len == sizeof(xfs_xmi_log_format)``
+
+ICREATE
+    | region 0: ``iov_len == sizeof(xfs_icreate_log)``
+
+QUOTAOFF
+    | region 0: ``iov_len == sizeof(xfs_qoff_logformat)``
+
+Layer 3: Full item validation (type-specific, cross-region)
+-----------------------------------------------------------
+
+When: After the item is fully assembled (``ri_cnt == ri_total``). This can
+be checked in ``xlog_recover_add_to_trans()`` right after incrementing
+``ri_cnt``, or in ``xlog_recover_reorder_trans()`` before the item is
+sorted into the replay lists. The latter is simpler as it's a single call
+site, but the former catches problems before the item is added to the
+transaction's item list.
+
+Preferred: validate in ``xlog_recover_reorder_trans()`` after the ops
+lookup (which will now be a simple ``ri_ops`` dereference since we set it in
+layer 1). This keeps the validation in one place and runs after all
+continuations have been resolved.
+
+New callback in ``struct xlog_recover_item_ops``::
+
+    int (*validate_item)(struct xlog *log,
+                         struct xlog_recover_item *item);
+
+Each item type implements ``validate_item`` to do cross-region structural
+checks:
+
+INODE
+    - Verify ``ri_cnt`` matches the fields set in ``ilf_fields`` (e.g. if
+      ``XFS_ILOG_DDATA`` is set, ``ri_buf[2]`` must exist and have sane
+      size)
+    - Verify dinode core fields in ``ri_buf[1]`` are self-consistent:
+      fork format vs fork size vs region length
+    - Verify ``ilf_ino`` is within valid range
+
+BUF
+    - Verify ``blf_blkno + blf_len`` doesn't exceed filesystem size
+    - Verify the number of data regions matches the bitmap in the
+      format header
+    - Verify ``blf_flags`` contains only valid flag bits
+
+DQUOT
+    - Verify ``dq_id``, ``dq_type`` are valid
+    - Run ``xfs_dquot_verify()`` on ``ri_buf[1]`` data
+
+EFI
+    - Verify ``efi_nextents`` matches the region size
+    - Verify each extent's startblock/blockcount are within fs bounds
+
+ATTRI
+    - Verify ``alfi_op_flags`` matches a known operation
+    - Verify ``ri_cnt`` matches the expected region count for the operation
+    - Verify name/value region sizes match
+      ``alfi_name_len``/``alfi_value_len``
+
+Other intent/done items
+    similar field-level validation
+
+ICREATE
+    - Already has validation in
+      ``xlog_recover_icreate_commit_pass2()``, move the checks to
+      ``validate_item``
+
+QUOTAOFF
+    - Verify ``qf_flags`` contains only valid quota flags
+
+Additional fixes in the generic code
+------------------------------------
+
+Beyond the three validation layers, fix these issues in the generic
+region assembly code:
+
+1. ``xlog_recover_add_to_cont_trans()``:
+
+   - Check ``ri_cnt > 0`` before accessing ``ri_buf[ri_cnt-1]``
+   - Check ``item->ri_buf != NULL`` before accessing it
+   - Bounds-check ``(old_len + len)`` against type-specific max region
+     size before calling ``kvrealloc``
+
+2. ``xlog_recover_add_to_trans()``:
+
+   - Check ``len >= 4`` before reading the type and size fields (except
+     for the zero-length transaction header continuation case)
+   - After looking up ops, validate ``ilf_size`` against ops constraints
+     before using it for the ``kzalloc_objs`` allocation
+
+3. ``xlog_recover_process_data()``:
+
+   - Validate ``oh_len`` is 4-byte aligned (all regions must be 32-bit
+     aligned per the existing comment)
+
+4. ``ITEM_TYPE`` macro:
+
+   - Add a check in ``xlog_recover_reorder_trans()`` that
+     ``ri_buf[0].iov_len >= 4`` before accessing the type code (this is
+     now redundant with layer 1 but is a safety net)
+
+Implementation Plan
+===================
+
+The infrastructure fields and callbacks in ``struct
+xlog_recover_item_ops`` (the ``min_regions``/``max_regions``/``min_hdr_len``
+fields and the ``validate_region`` callback) are introduced first, before
+the transaction header ops entry that uses them. Later patches populate
+those fields and callbacks for the remaining item types and wire up the
+generic call sites. Each patch builds cleanly and is independently
+testable.
+
+Phase 1: Generic infrastructure, early ops lookup, transaction header
+---------------------------------------------------------------------
+
+Patch 1: Add validation infrastructure to the ops struct
+    - Add the ``min_regions``/``max_regions``/``min_hdr_len`` fields to
+      ``struct xlog_recover_item_ops``
+    - Add the ``validate_region`` callback to ``struct
+      xlog_recover_item_ops``
+    - No behaviour change yet: the fields are zero and the callback is
+      NULL for all existing item types; nothing reads them until later
+      patches
+
+Patch 2: Treat the transaction header as a validated region type
+    - Add an ops entry for the transaction header keyed on the low 16 bits
+      of ``XFS_TRANS_HEADER_MAGIC``, with ``min_regions = 1``,
+      ``max_regions = 1``, ``min_hdr_len = sizeof(struct xfs_trans_header)``
+      and a ``validate_region`` that checks ``iov_len`` and the magic
+      number
+    - Remove the bespoke transaction header parsing from ``add_to_trans``
+      and ``add_to_cont_trans``; route through the generic region assembly
+    - Handle the zero-length first fragment case (``len == 0`` with empty
+      ``r_itemq``) by deferring to the continuation path
+    - Add the ``validate_region`` call site(s) needed for the transaction
+      header (generic call sites for all other types are wired in Patch 7)
+    - After validation, copy the decoded header to ``trans->r_theader``
+      as before
+
+Patch 3: Move item ops lookup to add_to_trans (first region decode)
+    - Look up and store ``ri_ops`` when ``ri_total == 0``
+    - Validate ``len >= 4`` before reading type/size
+    - Reject unknown item types immediately
+    - Remove the ops lookup from ``xlog_recover_reorder_trans()`` (it
+      becomes a simple NULL check / assertion)
+
+Patch 4: Populate min_regions/max_regions/min_hdr_len for all item types
+    - The three fields were added to ``struct xlog_recover_item_ops`` in
+      Patch 1; populate them for all remaining item types
+    - Add generic checks in ``add_to_trans`` after ops lookup:
+      ``ilf_size >= ops->min_regions && ilf_size <= ops->max_regions``,
+      ``len >= ops->min_hdr_len`` (for non-continuation first regions)
+
+Patch 5: Fix generic safety issues
+    - ``add_to_cont_trans``: check ``ri_cnt > 0`` and ``ri_buf != NULL``
+    - ``add_to_cont_trans``: bounds-check accumulated region size before
+      ``kvrealloc``
+    - ``process_data``: validate ``oh_len`` alignment
+
+Patch 6: Add ri_in_continuation tracking
+    - Add ``ri_in_continuation`` flag to ``struct xlog_recover_item``
+    - Set in ``add_to_cont_trans`` when data is appended
+    - Check and clear in ``add_to_trans`` when a new region starts
+      (previous continuation region is now complete)
+    - Check in ``xlog_recover_commit_trans`` for the final region case
+
+Phase 2: Per-region validation
+------------------------------
+
+Patch 7: Wire up generic validate_region call sites
+    - The ``validate_region`` callback was added to the ops struct in
+      Patch 1; wire up the generic call sites for all item types
+    - Call it from ``add_to_trans`` after each complete region is added
+    - Call it from ``add_to_trans`` when ``ri_in_continuation`` is cleared
+      (just-completed continuation region)
+    - Call it from ``xlog_recover_commit_trans`` if the final region was
+      a continuation
+
+Patches 8-N: Implement validate_region for each item type
+    - Start with the transaction header (magic, exact size)
+    - Then simple fixed-size types (done items, ICREATE, QUOTAOFF)
+    - Then single-region variable types (EFI, RUI, CUI, BUI)
+    - Then multi-region types (DQUOT, BUF)
+    - Finally complex types (INODE, ATTRI)
+
+Phase 3: Full item validation
+-----------------------------
+
+Patch M: Add validate_item callback infrastructure
+    - Add the callback to the ops struct
+    - Call it from ``xlog_recover_reorder_trans()`` after ops verification
+    - Return ``-EFSCORRUPTED`` on failure, aborting recovery
+
+Patches M+1 to M+N: Implement validate_item for each item type
+    - Move existing validation out of ``commit_pass2`` into
+      ``validate_item`` where possible (e.g. ICREATE field checks)
+    - Add new cross-region validation
+    - Order: simple types first, complex types last
+    - INODE validation is the most complex (fork format vs region size)
+
+Phase 4: Cleanup
+----------------
+
+- Remove redundant validation from ``commit_pass1``/``commit_pass2`` that
+  is now covered by ``validate_region``/``validate_item``
+- Add ``ASSERT()``\ s in commit handlers to verify validation has run
+- Review and update error messages for consistency

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

[Hongling Zeng]

在 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@vger.kernel.org
>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> <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:

   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?

   2. For the zero-length first fragment: if this is old-log
      compatibility only, 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.

   3. For testing I'll cover each phase with crafted-log images and run
      xfstests. 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?

   Thanks,
   Hongling

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@vger.kernel.org
> > > Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
> > <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@kernel.org

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

[Hongling Zeng]

在 2026年09月08日 14:33, 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@vger.kernel.org
>>>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>>> <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.
>
  Hi 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

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

[Dave Chinner]

On Tue, Sep 08, 2026 at 03:48:03PM +0800, Hongling Zeng wrote:
> 在 2026年09月08日 14:33, Dave Chinner 写道:
> > 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.

Ok, I just posted my current WIP to the log-verification-1 branch in
my kernel.org repo
(https://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git)

This is completely untested, I've only made sure it compiles. Don't
expect it to work. If you do start adding to it, build on top of it
and point me to the git repo where all your new work can be found.

What I've implemented so far is the refactoring necessary to
implement generic handling of log item verification and decoding,
converted the special case transhdr decoding to use the generic
infrastructure, and implemented fairly complete ophdr validation,
including validation of the log unmount record (which isn't
validated in any way right now).

Design has changed to use methods for item specific region count
checking, as well as adding a completion method that allows item
types to consume the item rather than queuing it for later recovery
(both needed for the transhdr conversion). Implementation has been
refined to avoid decoding ophdrs until sufficient validation has
been performed to guarantee the buffer pointer is sane and is long
enough to contain a full ophdr, and then it valdates the rest of the
ophdr before passing it to the processing code.

The next steps are to start implementing the per-item type
validation functions.

I also noticed that the head/tail search code that reads log
record headers doesn't really do much validation on the log record
headers. We probably need to address that, too, so we can detect
corrupted headers during the head/tail search and avoid finding
incorrect head/tail records as a result.

> 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.

I think it's probably better that you first read the code yourself
and develop an understanding of what needs to be done without the
aid of an LLM. See if you can find bugs in what I've already done
-without- an LLM - you will learn the code at the same time, and
then be in a much better place to guide an LLM through later stages
of development.

Cheers,

Dave.
-- 
Dave Chinner
dgc@kernel.org

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

[Hongling Zeng]

在 2026年09月09日 07:00, Dave Chinner 写道:
> On Tue, Sep 08, 2026 at 03:48:03PM +0800, Hongling Zeng wrote:
>> 在 2026年09月08日 14:33, Dave Chinner 写道:
>>> 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.
> Ok, I just posted my current WIP to the log-verification-1 branch in
> my kernel.org repo
> (https://git.kernel.org/pub/scm/linux/kernel/git/dgc/linux-xfs.git)
>
> This is completely untested, I've only made sure it compiles. Don't
> expect it to work. If you do start adding to it, build on top of it
> and point me to the git repo where all your new work can be found.
>
> What I've implemented so far is the refactoring necessary to
> implement generic handling of log item verification and decoding,
> converted the special case transhdr decoding to use the generic
> infrastructure, and implemented fairly complete ophdr validation,
> including validation of the log unmount record (which isn't
> validated in any way right now).
>
> Design has changed to use methods for item specific region count
> checking, as well as adding a completion method that allows item
> types to consume the item rather than queuing it for later recovery
> (both needed for the transhdr conversion). Implementation has been
> refined to avoid decoding ophdrs until sufficient validation has
> been performed to guarantee the buffer pointer is sane and is long
> enough to contain a full ophdr, and then it valdates the rest of the
> ophdr before passing it to the processing code.
>
> The next steps are to start implementing the per-item type
> validation functions.
>
> I also noticed that the head/tail search code that reads log
> record headers doesn't really do much validation on the log record
> headers. We probably need to address that, too, so we can detect
> corrupted headers during the head/tail search and avoid finding
> incorrect head/tail records as a result.
>
>> 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.
> I think it's probably better that you first read the code yourself
> and develop an understanding of what needs to be done without the
> aid of an LLM. See if you can find bugs in what I've already done
> -without- an LLM - you will learn the code at the same time, and
> then be in a much better place to guide an LLM through later stages
> of development.
>
> Cheers,
>
> Dave.
>
Thanks for posting the WIP series.

I’ll work from the log-verification-1 branch and first study the generic
iterator and item handling rather than extending the old special cases.
I’ll pay particular attention to arbitrary continuation fragments,
zero-length first fragments, transaction-header completion, and cleanup
on malformed input.

I’ll review the existing code for bugs before adding the per-item 
validation functions, and will point you to my repository once I have
work based on the branch.

Thanks,
Hongling

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

[Hongling Zeng]

在 2026年09月08日 14:33, 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@vger.kernel.org
>>>> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
>>> <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.
>
Hi Dave,

I reviewed part of the series per commit and tested recovery behaviour 
against the current tip (ecf8aa53, "handle zero length continuation op
headers during recovery"). A few notes and two followup fixes are below.

1. Per-commit observations

  - 5b115b1 ("lift transaction header parsing out of the region
    assembly") says it is behaviour-preserving. That is true for normal
    streams, but not for pre-ad3e3693182b zero-length first fragments:
    those used to be skipped and are rejected until ecf8aa53 restores the
    handling. This is fine within the series, but the changelog may be
    misleading if the refactor is backported alone.

  - In the window [895f4c5, ecf8aa53), a skipped zero-length first
    fragment can leave trans->r_cur_item NULL; the following WAS_CONT
    continuation then dereferences it in xlog_recover_add_to_cont_trans().
    So that window oopses during recovery rather than failing cleanly.

  - In the window [895f4c5, ca71ea1f), a 1-3 byte op header length can
    reach xlog_recover_add_to_trans(), which reads ilf_type/ilf_size from
    a kvmalloc'd buffer shorter than 4 bytes. ASAN catches this as a heap
    over-read. ca71ea1f closes it with the op header length/alignment
    validation.

  - Minor doc/changelog nits: 895f4c5's changelog and the rst say the
    zero-length first fragment case is handled uniformly, but that is only
    true after ecf8aa53. Also, ecf8aa53's doc update says the r_cur_item
    guard is in "Patch 5", but it lands in ecf8aa53 itself.

2. Behaviour testing

We extracted the recovery functions from 857882e, 895f4c5 and ecf8aa53
into a small userspace harness with ASAN, and replayed crafted op record
streams through them: unsplit and split transaction headers, zero-length
first and middle fragments, short fragments, stray WAS_CONT records,
truncated item plus commit, and normal transactions.

A few results:

  - A 3-way transaction header split such as 4+4+8 is silently assembled
    incorrectly at 857882e: the continuation tail-copy path writes the
    middle fragment to the wrong offset, later fragments overwrite it, and
    recovery still succeeds with a corrupted r_theader. 895f4c5 fixes this.

  - A stray WAS_CONT continuation crashes via the empty-queue list walk
    already at 857882e, so that predates this series. ecf8aa53 turns it
    into -EFSCORRUPTED.

  - Zero-length middle continuation fragments now assemble correctly via
    the generic path. The old bespoke transaction-header path rejected
    them.

  - Well-formed streams behaved the same across the three commits.

I can send the full matrix if useful.

3. Two followup fixes

These are against ecf8aa53 and are independent. I am including them
inline for now since the final base is not settled; happy to post proper
patches once it is.

First, xlog_recover_commit_trans() walks r_itemq only and ignores
trans->r_cur_item. If a commit record arrives while an item is still
being rebuilt, that item is silently dropped and the transaction commits
less metadata than the log records describe. A transaction whose only
content never completed can also commit as if it were empty.

Reject commits with a pending r_cur_item. This also catches the
zero-length-opener sentinel case. I did not change XLOG_UNMOUNT_TRANS,
where dropping the partial transaction matches the existing skip
semantics.

---
  fs/xfs/xfs_log_recover.c | 15 +++++++++++++++
  1 file changed, 15 insertions(+)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index d32b4de9215e..0ff898b71419 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2163,6 +2163,21 @@ xlog_recover_commit_trans(
         LIST_HEAD                       (ra_list);
         LIST_HEAD                       (done_list);

+       /*
+        * r_cur_item is queued only when its final fragment arrives.  If a
+        * commit record arrives while it is still set, recovery would 
otherwise
+        * drop the unfinished item and replay less metadata than the log
+        * records describe.  Treat that as a truncated transaction.
+        */
+       if (trans->r_cur_item) {
+               xfs_warn(log->l_mp, "%s: commit with incomplete item",
+                       __func__);
+               return -EFSCORRUPTED;
+       }
+
         #define XLOG_RECOVER_COMMIT_QUEUE_MAX 100

         hlist_del_init(&trans->r_list);

Second, xlog_recover_add_to_cont_trans() can grow the region under
assembly with kvrealloc on every continuation record. A corrupt log can
therefore grow a single region without a practical bound, and repeated
realloc/copy makes the work quadratic in the number of fragments.

Add a generic recovery-time cap. No writer-produced region should come
close to this; per-item limits can tighten it later.

---
  fs/xfs/libxfs/xfs_log_recover.h |  9 +++++++++
  fs/xfs/xfs_log_recover.c        | 14 ++++++++++++++
  2 files changed, 23 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_log_recover.h 
b/fs/xfs/libxfs/xfs_log_recover.h
index d2e128fca058..a6a29d5572be 100644
--- a/fs/xfs/libxfs/xfs_log_recover.h
+++ b/fs/xfs/libxfs/xfs_log_recover.h
@@ -149,6 +149,15 @@ extern const struct xlog_recover_item_ops 
xlog_rtcud_item_ops;

  #define XLOG_MAX_REGIONS_IN_ITEM   (XFS_MAX_BLOCKSIZE / XFS_BLF_CHUNK 
/ 2 + 1)

+/*
+ * Generic recovery-time cap for a single log region assembled from
+ * continuation op records.  This prevents corrupt continuation streams 
from
+ * growing a region without bound, or from driving unbounded 
realloc/copy work.
+ */
+#define XLOG_MAX_REGION_SIZE        (2 * XFS_MAX_BLOCKSIZE)
+
  /*
   * item headers are in ri_buf[0].  Additional buffers follow.
   */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index d32b4de9215e..02d28b610850 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2405,9 +2405,23 @@ xlog_recover_add_to_cont_trans(
                 return -EFSCORRUPTED;
         }

         old_ptr = item->ri_buf[item->ri_cnt - 1].iov_base;
         old_len = item->ri_buf[item->ri_cnt - 1].iov_len;

+       /*
+        * Bound the region being assembled so a corrupt continuation stream
+        * cannot grow it without limit.
+        */
+       if (old_len > XLOG_MAX_REGION_SIZE - len) {
+               xfs_warn(log->l_mp,
+                       "%s: continuation region too large (%d > %d)",
+                       __func__, old_len + len, XLOG_MAX_REGION_SIZE);
+               return -EFSCORRUPTED;
+       }
+
         ptr = xlog_kvmalloc(old_len + len);
         memcpy(ptr, old_ptr, old_len);
         memcpy(ptr + old_len, dp, len);

Both fixes were checked in the harness: truncated-item+commit and
zero-length-opener+commit now fail cleanly, and continuation streams that
grow past the cap return -EFSCORRUPTED. A region grown to about 64KB
through continuations still assembles and commits normally.

4. Small cleanups

  - xlog_find_item_ops() is a first-match linear scan, so duplicate
    item_type entries would silently shadow later ones. The table is unique
    today, but a debug/build-time check would catch future mistakes.

  - xlog_recover_nregions() warns about a bad number of regions "in inode
    log format", but the helper is now generic for item types without
    ->validate_nregions. The warning should probably be made generic too.

Happy to run these cases through recoveryloop or the logprint-rust fuzzer
once that is ready.

Thanks,
Hongling