* [PATCH] xfs: validate inode log item regions during recovery
@ 2026-07-16 17:48 Weiming Shi
2026-07-16 22:05 ` Dave Chinner
0 siblings, 1 reply; 3+ messages in thread
From: Weiming Shi @ 2026-07-16 17:48 UTC (permalink / raw)
To: Carlos Maiolino, Darrick J . Wong
Cc: linux-xfs, linux-kernel, xmei5, Weiming Shi
xlog_recover_inode_commit_pass2() replays an XFS_LI_INODE item straight
out of the log: it dereferences ri_buf[1] as the log dinode and copies the
data and attr forks from ri_buf[2]/ri_buf[attr_index], but never checks
that those regions were logged or that the dinode geometry agrees with the
mount. ri_buf is sized by the item's ilf_size, bounded only to
[1, XLOG_MAX_REGIONS_IN_ITEM]; the number of regions actually filled in
(ri_cnt) and the fork copies are guarded only by ASSERT()s, which compile
out in production.
A crafted image mounted for log recovery can thus hit a NULL dereference of
an absent region, an out-of-bounds read from a di_version or di_forkoff
that disagrees with the mount, or an overflow of the inode fork by an
oversized local or extent region. An item declaring a second region that
is never logged faults on the log dinode:
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
RIP: xlog_recover_inode_commit_pass2 (fs/xfs/xfs_inode_item_recover.c:370)
Call Trace:
xlog_recover_items_pass2 (fs/xfs/xfs_log_recover.c:2011)
xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2078)
xlog_recovery_process_trans (fs/xfs/xfs_log_recover.c:2328)
xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2502)
xlog_recover (fs/xfs/xfs_log_recover.c:3486)
xfs_log_mount (fs/xfs/xfs_log.c:667)
xfs_mountfs (fs/xfs/xfs_mount.c:1039)
xfs_fs_fill_super (fs/xfs/xfs_super.c:1965)
get_tree_bdev_flags (fs/super.c:1680)
vfs_get_tree (fs/super.c:1803)
__x64_sys_mount (fs/namespace.c:4433)
Validate the regions before use: require ri_cnt to cover the regions
ilf_fields implies, the log dinode to be present and at least a log dinode
in size, and di_version to match the mount. Replace the di_forkoff check,
which can never fire (di_forkoff is a u8 of 8-byte units, sb_inodesize a
byte count of at least 256), with xfs_dinode_verify_forkoff() (exported
here), and bound the local and extent fork copies against the destination
fork. The btree-root formats are logged in their larger in-core form and
converted on the way in, so they are not bounded by the on-disk fork size.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Xiang Mei <xmei5@asu.edu>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Weiming Shi <bestswngs@gmail.com>
---
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/libxfs/xfs_inode_buf.h | 2 ++
fs/xfs/xfs_inode_item_recover.c | 59 ++++++++++++++++++++++++---------
3 files changed, 47 insertions(+), 16 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 336ef843f2fe..71311c4fc2ed 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -479,7 +479,7 @@ xfs_dinode_verify_fork(
return NULL;
}
-static xfs_failaddr_t
+xfs_failaddr_t
xfs_dinode_verify_forkoff(
struct xfs_dinode *dip,
struct xfs_mount *mp)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index f3624532b023..d120675f6c07 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -27,6 +27,8 @@ int xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
xfs_failaddr_t xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
struct xfs_dinode *dip);
+xfs_failaddr_t xfs_dinode_verify_forkoff(struct xfs_dinode *dip,
+ struct xfs_mount *mp);
xfs_failaddr_t xfs_dinode_verify_metadir(struct xfs_mount *mp,
struct xfs_dinode *dip, uint16_t mode, uint16_t flags,
uint64_t flags2);
diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
index 169a8fe3bf0a..8ebc8eaf9e03 100644
--- a/fs/xfs/xfs_inode_item_recover.c
+++ b/fs/xfs/xfs_inode_item_recover.c
@@ -366,6 +366,13 @@ xlog_recover_inode_commit_pass2(
error = -EFSCORRUPTED;
goto out_release;
}
+ /* ri_buf[1] may be absent or shorter than a log dinode */
+ if (XFS_IS_CORRUPT(mp, item->ri_cnt < 2) ||
+ XFS_IS_CORRUPT(mp,
+ item->ri_buf[1].iov_len < xfs_log_dinode_size(mp))) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
ldip = item->ri_buf[1].iov_base;
if (XFS_IS_CORRUPT(mp, ldip->di_magic != XFS_DINODE_MAGIC)) {
xfs_alert(mp,
@@ -374,6 +381,11 @@ xlog_recover_inode_commit_pass2(
error = -EFSCORRUPTED;
goto out_release;
}
+ /* the size gate and fork offsets assume the mount's inode version */
+ if (XFS_IS_CORRUPT(mp, !xfs_dinode_good_version(mp, ldip->di_version))) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
/*
* If the inode has an LSN in it, recover the inode only if the on-disk
@@ -462,15 +474,6 @@ xlog_recover_inode_commit_pass2(
if (error)
goto out_release;
- if (unlikely(ldip->di_forkoff > mp->m_sb.sb_inodesize)) {
- XFS_CORRUPTION_ERROR("Bad log dinode fork offset",
- XFS_ERRLEVEL_LOW, mp, ldip, sizeof(*ldip));
- xfs_alert(mp,
- "Bad inode 0x%llx, di_forkoff 0x%x",
- in_f->ilf_ino, ldip->di_forkoff);
- error = -EFSCORRUPTED;
- goto out_release;
- }
isize = xfs_log_dinode_size(mp);
if (unlikely(item->ri_buf[1].iov_len > isize)) {
XFS_CORRUPTION_ERROR("Bad log dinode size", XFS_ERRLEVEL_LOW,
@@ -495,6 +498,30 @@ xlog_recover_inode_commit_pass2(
xfs_log_dinode_to_disk(ldip, dip, current_lsn);
fields = in_f->ilf_fields;
+
+ /* forks are otherwise ASSERT-only; check di_forkoff and presence */
+ if (fields & (XFS_ILOG_DFORK | XFS_ILOG_AFORK)) {
+ if (XFS_IS_CORRUPT(mp, xfs_dinode_verify_forkoff(dip, mp) != NULL)) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
+ }
+ if (fields & XFS_ILOG_DFORK) {
+ if (XFS_IS_CORRUPT(mp, item->ri_cnt < 3) ||
+ XFS_IS_CORRUPT(mp, item->ri_buf[2].iov_base == NULL)) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
+ }
+ if (fields & XFS_ILOG_AFORK) {
+ attr_index = (fields & XFS_ILOG_DFORK) ? 3 : 2;
+
+ if (XFS_IS_CORRUPT(mp, item->ri_cnt < attr_index + 1) ||
+ XFS_IS_CORRUPT(mp, item->ri_buf[attr_index].iov_base == NULL)) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
+ }
if (fields & XFS_ILOG_DEV)
xfs_dinode_put_rdev(dip, in_f->ilf_u.ilfu_rdev);
@@ -510,6 +537,10 @@ xlog_recover_inode_commit_pass2(
switch (fields & XFS_ILOG_DFORK) {
case XFS_ILOG_DDATA:
case XFS_ILOG_DEXT:
+ if (XFS_IS_CORRUPT(mp, len > XFS_DFORK_DSIZE(dip, mp))) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
memcpy(XFS_DFORK_DPTR(dip), src, len);
break;
@@ -533,11 +564,6 @@ xlog_recover_inode_commit_pass2(
* transaction.
*/
if (in_f->ilf_fields & XFS_ILOG_AFORK) {
- if (in_f->ilf_fields & XFS_ILOG_DFORK) {
- attr_index = 3;
- } else {
- attr_index = 2;
- }
len = item->ri_buf[attr_index].iov_len;
src = item->ri_buf[attr_index].iov_base;
ASSERT(len == xlog_calc_iovec_len(in_f->ilf_asize));
@@ -546,7 +572,10 @@ xlog_recover_inode_commit_pass2(
case XFS_ILOG_ADATA:
case XFS_ILOG_AEXT:
dest = XFS_DFORK_APTR(dip);
- ASSERT(len <= XFS_DFORK_ASIZE(dip, mp));
+ if (XFS_IS_CORRUPT(mp, len > XFS_DFORK_ASIZE(dip, mp))) {
+ error = -EFSCORRUPTED;
+ goto out_release;
+ }
memcpy(dest, src, len);
break;
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: validate inode log item regions during recovery
2026-07-16 17:48 [PATCH] xfs: validate inode log item regions during recovery Weiming Shi
@ 2026-07-16 22:05 ` Dave Chinner
2026-07-17 3:20 ` Weiming Shi
0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2026-07-16 22:05 UTC (permalink / raw)
To: Weiming Shi
Cc: Carlos Maiolino, Darrick J . Wong, linux-xfs, linux-kernel, xmei5
On Thu, Jul 16, 2026 at 10:48:12AM -0700, Weiming Shi wrote:
> xlog_recover_inode_commit_pass2() replays an XFS_LI_INODE item straight
> out of the log: it dereferences ri_buf[1] as the log dinode and copies the
> data and attr forks from ri_buf[2]/ri_buf[attr_index], but never checks
> that those regions were logged or that the dinode geometry agrees with the
> mount. ri_buf is sized by the item's ilf_size, bounded only to
> [1, XLOG_MAX_REGIONS_IN_ITEM]; the number of regions actually filled in
> (ri_cnt) and the fork copies are guarded only by ASSERT()s, which compile
> out in production.
>
> A crafted image mounted for log recovery can thus hit a NULL dereference of
> an absent region, an out-of-bounds read from a di_version or di_forkoff
> that disagrees with the mount, or an overflow of the inode fork by an
> oversized local or extent region. An item declaring a second region that
> is never logged faults on the log dinode:
>
> KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> RIP: xlog_recover_inode_commit_pass2 (fs/xfs/xfs_inode_item_recover.c:370)
> Call Trace:
> xlog_recover_items_pass2 (fs/xfs/xfs_log_recover.c:2011)
> xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2078)
> xlog_recovery_process_trans (fs/xfs/xfs_log_recover.c:2328)
> xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2502)
> xlog_recover (fs/xfs/xfs_log_recover.c:3486)
> xfs_log_mount (fs/xfs/xfs_log.c:667)
> xfs_mountfs (fs/xfs/xfs_mount.c:1039)
> xfs_fs_fill_super (fs/xfs/xfs_super.c:1965)
> get_tree_bdev_flags (fs/super.c:1680)
> vfs_get_tree (fs/super.c:1803)
> __x64_sys_mount (fs/namespace.c:4433)
>
> Validate the regions before use: require ri_cnt to cover the regions
> ilf_fields implies, the log dinode to be present and at least a log dinode
> in size, and di_version to match the mount. Replace the di_forkoff check,
> which can never fire (di_forkoff is a u8 of 8-byte units, sb_inodesize a
> byte count of at least 256), with xfs_dinode_verify_forkoff() (exported
> here), and bound the local and extent fork copies against the destination
> fork. The btree-root formats are logged in their larger in-core form and
> converted on the way in, so they are not bounded by the on-disk fork size.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/libxfs/xfs_inode_buf.h | 2 ++
> fs/xfs/xfs_inode_item_recover.c | 59 ++++++++++++++++++++++++---------
> 3 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 336ef843f2fe..71311c4fc2ed 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -479,7 +479,7 @@ xfs_dinode_verify_fork(
> return NULL;
> }
>
> -static xfs_failaddr_t
> +xfs_failaddr_t
> xfs_dinode_verify_forkoff(
> struct xfs_dinode *dip,
> struct xfs_mount *mp)
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index f3624532b023..d120675f6c07 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -27,6 +27,8 @@ int xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
>
> xfs_failaddr_t xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
> struct xfs_dinode *dip);
> +xfs_failaddr_t xfs_dinode_verify_forkoff(struct xfs_dinode *dip,
> + struct xfs_mount *mp);
> xfs_failaddr_t xfs_dinode_verify_metadir(struct xfs_mount *mp,
> struct xfs_dinode *dip, uint16_t mode, uint16_t flags,
> uint64_t flags2);
> diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> index 169a8fe3bf0a..8ebc8eaf9e03 100644
> --- a/fs/xfs/xfs_inode_item_recover.c
> +++ b/fs/xfs/xfs_inode_item_recover.c
> @@ -366,6 +366,13 @@ xlog_recover_inode_commit_pass2(
> error = -EFSCORRUPTED;
> goto out_release;
> }
> + /* ri_buf[1] may be absent or shorter than a log dinode */
> + if (XFS_IS_CORRUPT(mp, item->ri_cnt < 2) ||
> + XFS_IS_CORRUPT(mp,
> + item->ri_buf[1].iov_len < xfs_log_dinode_size(mp))) {
> + error = -EFSCORRUPTED;
> + goto out_release;
> + }
I'm going to push back on this sort of piece-meal validation - this
is not the place to be doing random "is the log item valid" checks.
All it does is make the code harder for humans to read and
understand.
It is inconsistent, it is not applied to all log items, the checks
are randomly distributed through the decoding and replay logic, and
it's impossible to tell what checks might still be missing.
We have a solid metadata verifier architecture for a reason: it
centralises all the metadata verification for an object in a central
place, and it always gets run when the object is first read from
disk before we try to do anything with it. The verification is in
one place, and it is easy to audit to determine what parts of the
structure we aren't actually verifying.
That means when we actually got to do an operation, we already know
the structures are valid and do not contain corruptions. Issues like
the log format structure not having enough regions is not an inode
log format problem - it is a generic log format structure problem.
Addressing these issues piece meal in each log item decode in pass 2 -
after we've already parsed them in pass 1 and for readahead purposes
- is too late and too random.
Use your AI tokens to do something properly useful - add an object
verifier layer to the journal recovery code that robustly verifies
objects before we perform decoding. We already have type abstraction
for items in log recovery (i.e. struct xlog_recover_item_ops) and we
already have two passes over the journal (XLOG_RECOVER_PASS1/2), and
pass 1 is used to set up state needed for the actual recovery in
pass 2. IOWs, we should be using pass 1 as a full object verifier
pass in addition to checking CRC.
This would allows a clean separation between validation and decoding
of log items and their formatted structures, and allow the recovery
logic to be simplified because it is no longer mixing recovery with
validation.
This will allow us to add full, robust validation for all log items,
and do it in a way that is easy to audit and identify parts of
structres taht we haven't verified or are unable to verify easily.
This is the sort of thing you should be using the power of the LLM
agents to do - it's a waste of tokens to be trying to find and fix
issues hidden in the code. Build the infrastructure that makes
verification -robust-, and that will almost entirely eliminate this
class of bug from log recovery.
Use the power of the LLMs to address the underlying problem, don't
waste time and tokens getting it to apply bandaids that don't
actually help address the underlying issue.
-Dave.
--
Dave Chinner
dgc@kernel.org
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: validate inode log item regions during recovery
2026-07-16 22:05 ` Dave Chinner
@ 2026-07-17 3:20 ` Weiming Shi
0 siblings, 0 replies; 3+ messages in thread
From: Weiming Shi @ 2026-07-17 3:20 UTC (permalink / raw)
To: Dave Chinner
Cc: Carlos Maiolino, Darrick J . Wong, linux-xfs, linux-kernel, xmei5
Dave Chinner <dgc@kernel.org> 于2026年7月17日周五 06:05写道:
>
> On Thu, Jul 16, 2026 at 10:48:12AM -0700, Weiming Shi wrote:
> > xlog_recover_inode_commit_pass2() replays an XFS_LI_INODE item straight
> > out of the log: it dereferences ri_buf[1] as the log dinode and copies the
> > data and attr forks from ri_buf[2]/ri_buf[attr_index], but never checks
> > that those regions were logged or that the dinode geometry agrees with the
> > mount. ri_buf is sized by the item's ilf_size, bounded only to
> > [1, XLOG_MAX_REGIONS_IN_ITEM]; the number of regions actually filled in
> > (ri_cnt) and the fork copies are guarded only by ASSERT()s, which compile
> > out in production.
> >
> > A crafted image mounted for log recovery can thus hit a NULL dereference of
> > an absent region, an out-of-bounds read from a di_version or di_forkoff
> > that disagrees with the mount, or an overflow of the inode fork by an
> > oversized local or extent region. An item declaring a second region that
> > is never logged faults on the log dinode:
> >
> > KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
> > RIP: xlog_recover_inode_commit_pass2 (fs/xfs/xfs_inode_item_recover.c:370)
> > Call Trace:
> > xlog_recover_items_pass2 (fs/xfs/xfs_log_recover.c:2011)
> > xlog_recover_commit_trans (fs/xfs/xfs_log_recover.c:2078)
> > xlog_recovery_process_trans (fs/xfs/xfs_log_recover.c:2328)
> > xlog_recover_process_data (fs/xfs/xfs_log_recover.c:2502)
> > xlog_recover (fs/xfs/xfs_log_recover.c:3486)
> > xfs_log_mount (fs/xfs/xfs_log.c:667)
> > xfs_mountfs (fs/xfs/xfs_mount.c:1039)
> > xfs_fs_fill_super (fs/xfs/xfs_super.c:1965)
> > get_tree_bdev_flags (fs/super.c:1680)
> > vfs_get_tree (fs/super.c:1803)
> > __x64_sys_mount (fs/namespace.c:4433)
> >
> > Validate the regions before use: require ri_cnt to cover the regions
> > ilf_fields implies, the log dinode to be present and at least a log dinode
> > in size, and di_version to match the mount. Replace the di_forkoff check,
> > which can never fire (di_forkoff is a u8 of 8-byte units, sb_inodesize a
> > byte count of at least 256), with xfs_dinode_verify_forkoff() (exported
> > here), and bound the local and extent fork copies against the destination
> > fork. The btree-root formats are logged in their larger in-core form and
> > converted on the way in, so they are not bounded by the on-disk fork size.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> > ---
> > fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> > fs/xfs/libxfs/xfs_inode_buf.h | 2 ++
> > fs/xfs/xfs_inode_item_recover.c | 59 ++++++++++++++++++++++++---------
> > 3 files changed, 47 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 336ef843f2fe..71311c4fc2ed 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -479,7 +479,7 @@ xfs_dinode_verify_fork(
> > return NULL;
> > }
> >
> > -static xfs_failaddr_t
> > +xfs_failaddr_t
> > xfs_dinode_verify_forkoff(
> > struct xfs_dinode *dip,
> > struct xfs_mount *mp)
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> > index f3624532b023..d120675f6c07 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.h
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> > @@ -27,6 +27,8 @@ int xfs_inode_from_disk(struct xfs_inode *ip, struct xfs_dinode *from);
> >
> > xfs_failaddr_t xfs_dinode_verify(struct xfs_mount *mp, xfs_ino_t ino,
> > struct xfs_dinode *dip);
> > +xfs_failaddr_t xfs_dinode_verify_forkoff(struct xfs_dinode *dip,
> > + struct xfs_mount *mp);
> > xfs_failaddr_t xfs_dinode_verify_metadir(struct xfs_mount *mp,
> > struct xfs_dinode *dip, uint16_t mode, uint16_t flags,
> > uint64_t flags2);
> > diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> > index 169a8fe3bf0a..8ebc8eaf9e03 100644
> > --- a/fs/xfs/xfs_inode_item_recover.c
> > +++ b/fs/xfs/xfs_inode_item_recover.c
> > @@ -366,6 +366,13 @@ xlog_recover_inode_commit_pass2(
> > error = -EFSCORRUPTED;
> > goto out_release;
> > }
> > + /* ri_buf[1] may be absent or shorter than a log dinode */
> > + if (XFS_IS_CORRUPT(mp, item->ri_cnt < 2) ||
> > + XFS_IS_CORRUPT(mp,
> > + item->ri_buf[1].iov_len < xfs_log_dinode_size(mp))) {
> > + error = -EFSCORRUPTED;
> > + goto out_release;
> > + }
>
> I'm going to push back on this sort of piece-meal validation - this
> is not the place to be doing random "is the log item valid" checks.
> All it does is make the code harder for humans to read and
> understand.
>
> It is inconsistent, it is not applied to all log items, the checks
> are randomly distributed through the decoding and replay logic, and
> it's impossible to tell what checks might still be missing.
>
> We have a solid metadata verifier architecture for a reason: it
> centralises all the metadata verification for an object in a central
> place, and it always gets run when the object is first read from
> disk before we try to do anything with it. The verification is in
> one place, and it is easy to audit to determine what parts of the
> structure we aren't actually verifying.
>
> That means when we actually got to do an operation, we already know
> the structures are valid and do not contain corruptions. Issues like
> the log format structure not having enough regions is not an inode
> log format problem - it is a generic log format structure problem.
>
> Addressing these issues piece meal in each log item decode in pass 2 -
> after we've already parsed them in pass 1 and for readahead purposes
> - is too late and too random.
>
> Use your AI tokens to do something properly useful - add an object
> verifier layer to the journal recovery code that robustly verifies
> objects before we perform decoding. We already have type abstraction
> for items in log recovery (i.e. struct xlog_recover_item_ops) and we
> already have two passes over the journal (XLOG_RECOVER_PASS1/2), and
> pass 1 is used to set up state needed for the actual recovery in
> pass 2. IOWs, we should be using pass 1 as a full object verifier
> pass in addition to checking CRC.
>
> This would allows a clean separation between validation and decoding
> of log items and their formatted structures, and allow the recovery
> logic to be simplified because it is no longer mixing recovery with
> validation.
>
> This will allow us to add full, robust validation for all log items,
> and do it in a way that is easy to audit and identify parts of
> structres taht we haven't verified or are unable to verify easily.
>
> This is the sort of thing you should be using the power of the LLM
> agents to do - it's a waste of tokens to be trying to find and fix
> issues hidden in the code. Build the infrastructure that makes
> verification -robust-, and that will almost entirely eliminate this
> class of bug from log recovery.
>
> Use the power of the LLMs to address the underlying problem, don't
> waste time and tokens getting it to apply bandaids that don't
> actually help address the underlying issue.
>
> -Dave.
> --
> Dave Chinner
> dgc@kernel.org
Hi Dave,
Thanks for the review. I'll drop the per-item checks and rework it
into the pass1 object verifier
pass you described, keeping the validation separate from the decode
and replay code.
Thanks,
Weiming Shi
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-07-17 3:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-16 17:48 [PATCH] xfs: validate inode log item regions during recovery Weiming Shi
2026-07-16 22:05 ` Dave Chinner
2026-07-17 3:20 ` Weiming Shi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.