Re: [PATCH] xfs: validate inode log item regions during recovery

[Weiming Shi]

Dave Chinner <dgc@xxxxxxxxxx> 于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@xxxxxxx>
> > Assisted-by: Claude:claude-opus-4-8
> > Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx

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