Re: [PATCH 1/2] xfs: inode recovery does not validate the recovered inode

[Dave Chinner <david@fromorbit.com>]

On Fri, Nov 10, 2023 at 11:27:52AM -0800, Darrick J. Wong wrote:
> On Fri, Nov 10, 2023 at 03:33:13PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Discovered when trying to track down a weird recovery corruption
> > issue that wasn't detected at recovery time.
> > 
> > The specific corruption was a zero extent count field when big
> > extent counts are in use, and it turns out the dinode verifier
> > doesn't detect that specific corruption case, either. So fix it too.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_inode_buf.c   |  3 +++
> >  fs/xfs/xfs_inode_item_recover.c | 14 +++++++++++++-
> >  2 files changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index a35781577cad..0f970a0b3382 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -508,6 +508,9 @@ xfs_dinode_verify(
> >  	if (mode && nextents + naextents > nblocks)
> >  		return __this_address;
> >  
> > +	if (nextents + naextents == 0 && nblocks != 0)
> > +		return __this_address;
> > +
> >  	if (S_ISDIR(mode) && nextents > mp->m_dir_geo->max_extents)
> >  		return __this_address;
> >  
> > diff --git a/fs/xfs/xfs_inode_item_recover.c b/fs/xfs/xfs_inode_item_recover.c
> > index 6b09e2bf2d74..f4c31c2b60d5 100644
> > --- a/fs/xfs/xfs_inode_item_recover.c
> > +++ b/fs/xfs/xfs_inode_item_recover.c
> > @@ -286,6 +286,7 @@ xlog_recover_inode_commit_pass2(
> >  	struct xfs_log_dinode		*ldip;
> >  	uint				isize;
> >  	int				need_free = 0;
> > +	xfs_failaddr_t			fa;
> >  
> >  	if (item->ri_buf[0].i_len == sizeof(struct xfs_inode_log_format)) {
> >  		in_f = item->ri_buf[0].i_addr;
> > @@ -529,8 +530,19 @@ xlog_recover_inode_commit_pass2(
> >  	    (dip->di_mode != 0))
> >  		error = xfs_recover_inode_owner_change(mp, dip, in_f,
> >  						       buffer_list);
> > -	/* re-generate the checksum. */
> > +	/* re-generate the checksum and validate the recovered inode. */
> >  	xfs_dinode_calc_crc(log->l_mp, dip);
> > +	fa = xfs_dinode_verify(log->l_mp, in_f->ilf_ino, dip);
> > +	if (fa) {
> 
> Does xlog_recover_dquot_commit_pass2 need to call xfs_dquot_verify as
> well?

Maybe - I haven't looked closely at that, and it depends what the
dquot buffer verifier does. If it's similar to the inode cluster
buffer verifier (i.e. only checks for dquots, doesn't verify the
dquots) then it should do the same thing. I don't have time to do
this right now because I'm OOO for the next week, so maybe you could
check this and send a patch for it?

> This patch looks good though,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com