Re: xfs_metadump segmentation fault on large fs - xfsprogs 6.1

[Dave Chinner <david@fromorbit.com>]

On Sun, Sep 28, 2025 at 08:11:05AM +0200, Carlos Maiolino wrote:
> On Sun, Sep 28, 2025 at 09:22:57AM +1000, Dave Chinner wrote:
> > On Fri, Sep 26, 2025 at 03:45:17PM +0200, Carlos Maiolino wrote:
> > > On Fri, Sep 26, 2025 at 07:39:18PM +1000, Dave Chinner wrote:
> > > > So there must be a bounds checking bug in process_exinode():
> > > >
> > > > static int
> > > > process_exinode(
> > > >         struct xfs_dinode       *dip,
> > > >         int                     whichfork)
> > > > {
> > > >         xfs_extnum_t            max_nex = xfs_iext_max_nextents(
> > > >                         xfs_dinode_has_large_extent_counts(dip), whichfork);
> > > >         xfs_extnum_t            nex = xfs_dfork_nextents(dip, whichfork);
> > > >         int                     used = nex * sizeof(struct xfs_bmbt_rec);
> > > >
> > > >         if (nex > max_nex || used > XFS_DFORK_SIZE(dip, mp, whichfork)) {
> > > >                 if (metadump.show_warnings)
> > > >                         print_warning("bad number of extents %llu in inode %lld",
> > > >                                 (unsigned long long)nex,
> > > >                                 (long long)metadump.cur_ino);
> > > >                 return 1;
> > > >         }
> > > >
> > > > Can you spot it?
> > > >
> > > > Hint: ((2^28 + 1) * 2^4) - 1 as an int is?
> > >
> > > Perhaps the patch below will suffice?
> > >
> > > diff --git a/db/metadump.c b/db/metadump.c
> > > index 34f2d61700fe..1dd38ab84ade 100644
> > > --- a/db/metadump.c
> > > +++ b/db/metadump.c
> > > @@ -2395,7 +2395,7 @@ process_btinode(
> > >
> > >  static int
> > >  process_exinode(
> > > -	struct xfs_dinode 	*dip,
> > > +	struct xfs_dinode	*dip,
> > >  	int			whichfork)
> > >  {
> > >  	xfs_extnum_t		max_nex = xfs_iext_max_nextents(
> > > @@ -2403,7 +2403,13 @@ process_exinode(
> > >  	xfs_extnum_t		nex = xfs_dfork_nextents(dip, whichfork);
> > >  	int			used = nex * sizeof(struct xfs_bmbt_rec);
> > >
> > > -	if (nex > max_nex || used > XFS_DFORK_SIZE(dip, mp, whichfork)) {
> > > +	/*
> > > +	 * We need to check for overflow of used counter.
> > > +	 * If the inode extent count is corrupted, we risk having a
> > > +	 * big enough number of extents to overflow it.
> > > +	 */
> > > +	if (used < nex || nex > max_nex ||
> > > +	    used > XFS_DFORK_SIZE(dip, mp, whichfork)) {
> > >  		if (metadump.show_warnings)
> > >  			print_warning("bad number of extents %llu in inode %lld",
> > >  				(unsigned long long)nex,
> > >
> > 
> > That fixes this specific problem, but now it will reject valid
> > inodes with valid but large extent counts.
> > 
> > What type does XFS_SB_FEAT_INCOMPAT_NREXT64 require for extent
> > count calculations?  i.e. what's the size of xfs_extnum_t?
> 
> I thought about extending it to 64bit, but honestly thought it was not
> necessary here as I thought the number of extents in an inode before it
> was converted to btree format wouldn't exceed a 32-bit counter.

The filesystem is corrupt so the normal rules of sanity don't apply.
The extent count could be anything, and we can't assume that it fits
in a 32 bit value, nor that any unchecked calculation based on the
value fits in 32 bits.

Mixing integer types like this always leads to bugs. It's bad
practice because everyone who looks at the code has to think about
type conversion rules (which no-one ever remembers or gets right) to
determine if the code is correct or not. Nobody catches stuff
like this during review and the compiler is no help, either.

> That's a
> trivial change for the patch, but still I think the overflow check
> should still be there as even for a 64bit counter we could have enough
> garbage to overflow it. Does it make sense to you?

Yes, we need to check for overflow, but IMO, the best way to do
these checks is to use the same type (and hence unsigned 64 bit
math) throughout. This requires much less metnal gymnastics to
determine that it is obviously correct:

....
	xfs_extnum_t		used = nex * sizeof(struct xfs_bmbt_rec);

	// number of extents clearly bad
	if (nex > max_nex)
		goto warn;

	// catch extent array size overflow
	if (used < nex)
		goto warn;

	// extent array should fit in the inode fork
	if (used > XFS_DFORK_SIZE(dip, mp, whichfork))
		goto warn;

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