Re: [PATCH] xfs_db: make sure agblocks is valid to prevent corruption

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 22, 2024 at 05:49:12PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 21, 2024 at 06:44:12PM +0800, liuhuan01@kylinos.cn wrote:
> > From: liuh <liuhuan01@kylinos.cn>
> > 
> > Recently, I was testing xfstests. When I run xfs/350 case, it always generate coredump during the process.
> > 	xfs_db -c "sb 0" -c "p agblocks" /dev/loop1
> > 
> > System will generate signal SIGFPE corrupt the process. And the stack as follow:
> > corrupt at: (*bpp)->b_pag = xfs_perag_get(btp->bt_mount, xfs_daddr_to_agno(btp->bt_mount, blkno)); in function libxfs_getbuf_flags
> > 	#0  libxfs_getbuf_flags
> > 	#1  libxfs_getbuf_flags
> > 	#2  libxfs_buf_read_map
> > 	#3  libxfs_buf_read
> > 	#4  libxfs_mount
> > 	#5  init
> > 	#6  main
> > 
> > The coredump was caused by the corrupt superblock metadata: (mp)->m_sb.sb_agblocks, it was 0.
> > In this case, user cannot run in expert mode also.
> > 
> > Never check (mp)->m_sb.sb_agblocks before use it cause this issue.
> > Make sure (mp)->m_sb.sb_agblocks > 0 before libxfs_mount to prevent corruption and leave a message.
> > 
> > Signed-off-by: liuh <liuhuan01@kylinos.cn>
> > ---
> >  db/init.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/db/init.c b/db/init.c
> > index cea25ae5..2d3295ba 100644
> > --- a/db/init.c
> > +++ b/db/init.c
> > @@ -129,6 +129,13 @@ init(
> >  		}
> >  	}
> >  
> > +	if (unlikely(sbp->sb_agblocks == 0)) {
> > +		fprintf(stderr,
> > +			_("%s: device %s agblocks unexpected\n"),
> > +			progname, x.data.name);
> > +		exit(1);
> 
> What if we set sb_agblocks to 1 and let the debugger continue?

Yeah, I'd prefer that xfs_db will operate on a corrupt filesystem and
maybe crash unexpectedly than to refuse to allow any diagnosis of
the corrupt filesystem.

xfs_db is a debug and forensic analysis tool. Having it crash
because it didn't handle some corruption entirely corectly isn't
something that we should be particularly worried about...

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