On Thu, Feb 18, 2021 at 10:41:59AM +0800, Gao Xiang wrote:

Hi Eric,

On Mon, Feb 15, 2021 at 07:04:25PM -0600, Eric Sandeen wrote:

quoted

On 10/12/20 11:06 PM, Gao Xiang wrote:

quoted

Check stripe numbers in calc_stripe_factors() by using
xfs_validate_stripe_geometry().

Signed-off-by: Gao Xiang <redacted>

Hm, unless I have made a mistake, this seems to allow an invalid
stripe specification.

Without this patch, this fails:

# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0
data su must be a multiple of the sector size (512)

With the patch:

# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0
meta-data=/dev/loop0             isize=512    agcount=8, agsize=32768 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=0
data     =                       bsize=4096   blocks=262144, imaxpct=25
         =                       sunit=1      swidth=1 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=2560, version=2
         =                       sectsz=512   sunit=1 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
Discarding blocks...Done.

When you are back from holiday, can you check? No big rush.

I'm back from holiday today. I think the problem is in
"if (dsu || dsw) {" it turns into "dsunit  = (int)BTOBBT(dsu);" anyway,
and then if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit),
					     BBTOB(dswidth), cfg->sectorsize, false))

so dsu isn't checked with sectorsize in advance before it turns into BB.

the fix seems simple though,
1) turn dsunit and dswidth into bytes rather than BB, but I have no idea the range of
   these 2 varibles, since I saw "if (big_dswidth > INT_MAX) {" but the big_dswidth
   was also in BB as well, if we turn these into bytes, and such range cannot be
   guarunteed...
2) recover the previous code snippet and check dsu in advance:
		if (dsu % cfg->sectorsize) {
			fprintf(stderr,
_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
			usage();
		}

btw, do we have some range test about these variables? I could rearrange the code
snippet, but I'm not sure if it could introduce some new potential regression as well...

Thanks,
Gao Xiang

Or how about applying the following incremental patch, although the maximum dswidth
would be smaller I think, but considering libxfs_validate_stripe_geometry() accepts
dswidth in 64-bit bytes as well. I think that would be fine. Does that make sense?

I've confirmed "# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0" now report:
stripe unit (4097) must be a multiple of the sector size (512)

and xfs/191-input-validation passes now...

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f152d5c7..80405790 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c

@@ -2361,20 +2361,24 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}
 
-		dsunit  = (int)BTOBBT(dsu);
-		big_dswidth = (long long int)dsunit * dsw;
+		big_dswidth = (long long int)dsu * dsw;
 		if (big_dswidth > INT_MAX) {
 			fprintf(stderr,
 _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
 				big_dswidth, dsunit);
 			usage();
 		}
-		dswidth = big_dswidth;
-	}
 
-	if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth),
-					     cfg->sectorsize, false))
+		if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth,
+						     cfg->sectorsize, false))
+			usage();
+
+		dsunit = BTOBBT(dsu);
+		dswidth = BTOBBT(big_dswidth);
+	} else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit),
+			BBTOB(dswidth), cfg->sectorsize, false)) {
 		usage();
+	}
 
 	/* If sunit & swidth were manually specified as 0, same as noalign */
 	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&

-- 
2.27.0