xfs_metadump: agcount*agblocks overflow

Found another potential overflow in xfs_metadump,
similar to those just fixed in repair.

Signed-off-by Eric Sandeen <[email protected]>

--

db/metadump.c +0 -0

index 19aed4f..ef6e571 100644

Quoted 19 lines · Eric Sandeen

Found another potential overflow in xfs_metadump,
similar to those just fixed in repair.

Signed-off-by Eric Sandeen <[email protected]>

--

db/metadump.c +2 -1

index 19aed4f..ef6e571 100644
@@ -222,7 +222,8 @@ valid_bno(
 		return 1;
 	if (agno == (mp->m_sb.sb_agcount - 1) && agbno > 0 &&
 			agbno <= (mp->m_sb.sb_dblocks -
-			 (mp->m_sb.sb_agcount - 1) * mp->m_sb.sb_agblocks))
+			 (xfs_drfsbno_t)(mp->m_sb.sb_agcount - 1) *
+			 mp->m_sb.sb_agblocks))
 		return 1;
 

db/metadump.c valid_bno( · line 229

Quoted 1 line

db/metadump.c +0 -0

 	return 0;

I have a really hard time reading the function (both before and after your patch). It's a real mess and no wonder we have these overflow problems here. What about the following instead:

static int
valid_bno(
xfs_agnumber_t agno,
xfs_agblock_t agbno)

{
	xfs_agnumber_t		last_agno = mp->m_sb.sb_agcount - 1;
	xfs_drfsbno_t		nblocks;

	/*
	 * The first block in every AG contains a backups superblock,
	 * and is copied separately, and we can skip it early as an
	 * optimization.
	 */
	if (agbno == 0)
		return 0;

	/*
	 * An invalid AG number is never okay.
	 */
	if (agno > last_agno)
		return 0;

	if (agno == last_agno) {
		nblocks = mp->m_sb.sb_dblocks -
			((xfs_drfsbno_t)mp->m_sb.sb_agblocks *
			 (mp->m_sb.sb_agcount - 1));
	} else {
		nblocks = mp->m_sb.sb_agblocks);
	}
	
	if (agbno > nblocks)
		return 0;
	return 1;
}

and with that form I wonder if we don't still have an off-by-one in the last if clause - shouldn't the agblocks be the count of blocks while agbno is an indes and thus 0-based?

Btw, do you have a testcase for this?