[PATCH v6 0/3] xfsprogs: consolidate stripe validation

[PATCH v6 0/3] xfsprogs: consolidate stripe validation

[Gao Xiang]

v5: https://lore.kernel.org/r/20201009052421.3328-1-hsiangkao@redhat.com

Hi,

This is another approach suggested by Eric in the reply of v3
(if I understand correctly), which also attempts to use
i18n-enabled xfsprogs xfs_notice() to error out sanity check
failure suggested by Dave on IRC.

kernel side of [PATCH 2/3]
https://lore.kernel.org/r/20201013034853.28236-1-hsiangkao@redhat.com

Changes since v5:
 - rename the helper to xfs_validate_stripe_geometry() (Brian);
 - drop a new added trailing newline in xfs_sb.c (Brian);
 - add "bool silent" argument to avoid too many error messages (Brian).

Thanks,
Gao Xiang

Gao Xiang (3):
  libxfs: allow i18n to xfs printk
  xfs: introduce xfs_validate_stripe_geometry()
  mkfs: make use of xfs_validate_stripe_geometry()

 libxfs/libxfs_api_defs.h |  1 +
 libxfs/libxfs_priv.h     |  8 ++---
 libxfs/xfs_sb.c          | 70 +++++++++++++++++++++++++++++++++-------
 libxfs/xfs_sb.h          |  3 ++
 mkfs/xfs_mkfs.c          | 23 ++++---------
 5 files changed, 74 insertions(+), 31 deletions(-)

-- 
2.18.1

[PATCH v6 1/3] libxfs: allow i18n to xfs printk

[Gao Xiang]

In preparation to a common stripe validation helper, allow i18n to
xfs_{notice,warn,err,alert} so xfsprogs can share code with kernel.

Suggested-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 libxfs/libxfs_priv.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 4356fa43..bd724c32 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -121,10 +121,10 @@ extern char    *progname;
 extern void cmn_err(int, char *, ...);
 enum ce { CE_DEBUG, CE_CONT, CE_NOTE, CE_WARN, CE_ALERT, CE_PANIC };
 
-#define xfs_notice(mp,fmt,args...)		cmn_err(CE_NOTE,fmt, ## args)
-#define xfs_warn(mp,fmt,args...)		cmn_err(CE_WARN,fmt, ## args)
-#define xfs_err(mp,fmt,args...)			cmn_err(CE_ALERT,fmt, ## args)
-#define xfs_alert(mp,fmt,args...)		cmn_err(CE_ALERT,fmt, ## args)
+#define xfs_notice(mp,fmt,args...)	cmn_err(CE_NOTE, _(fmt), ## args)
+#define xfs_warn(mp,fmt,args...)	cmn_err(CE_WARN, _(fmt), ## args)
+#define xfs_err(mp,fmt,args...)		cmn_err(CE_ALERT, _(fmt), ## args)
+#define xfs_alert(mp,fmt,args...)	cmn_err(CE_ALERT, _(fmt), ## args)
 
 #define xfs_buf_ioerror_alert(bp,f)	((void) 0);
 
-- 
2.18.1

[PATCH v6 2/3] xfs: introduce xfs_validate_stripe_geometry()

[Gao Xiang]

Introduce a common helper to consolidate stripe validation process.
Also make kernel code xfs_validate_sb_common() use it first.

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 libxfs/xfs_sb.c | 70 +++++++++++++++++++++++++++++++++++++++++--------
 libxfs/xfs_sb.h |  3 +++
 2 files changed, 62 insertions(+), 11 deletions(-)

diff --git a/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index 302eea16..9c1481d8 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -357,21 +357,18 @@ xfs_validate_sb_common(
 		}
 	}
 
-	if (sbp->sb_unit) {
-		if (!xfs_sb_version_hasdalign(sbp) ||
-		    sbp->sb_unit > sbp->sb_width ||
-		    (sbp->sb_width % sbp->sb_unit) != 0) {
-			xfs_notice(mp, "SB stripe unit sanity check failed");
-			return -EFSCORRUPTED;
-		}
-	} else if (xfs_sb_version_hasdalign(sbp)) {
+	/*
+	 * Either (sb_unit and !hasdalign) or (!sb_unit and hasdalign)
+	 * would imply the image is corrupted.
+	 */
+	if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
 		xfs_notice(mp, "SB stripe alignment sanity check failed");
 		return -EFSCORRUPTED;
-	} else if (sbp->sb_width) {
-		xfs_notice(mp, "SB stripe width sanity check failed");
-		return -EFSCORRUPTED;
 	}
 
+	if (!xfs_validate_stripe_geometry(mp, XFS_FSB_TO_B(mp, sbp->sb_unit),
+			XFS_FSB_TO_B(mp, sbp->sb_width), 0, false))
+		return -EFSCORRUPTED;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
@@ -1208,3 +1205,54 @@ xfs_sb_get_secondary(
 	*bpp = bp;
 	return 0;
 }
+
+/*
+ * sunit, swidth, sectorsize(optional with 0) should be all in bytes,
+ * so users won't be confused by values in error messages.
+ */
+bool
+xfs_validate_stripe_geometry(
+	struct xfs_mount	*mp,
+	__s64			sunit,
+	__s64			swidth,
+	int			sectorsize,
+	bool			silent)
+{
+	if (sectorsize && sunit % sectorsize) {
+		if (!silent)
+			xfs_notice(mp,
+"stripe unit (%lld) must be a multiple of the sector size (%d)",
+				   sunit, sectorsize);
+		return false;
+	}
+
+	if (sunit && !swidth) {
+		if (!silent)
+			xfs_notice(mp,
+"invalid stripe unit (%lld) and stripe width of 0", sunit);
+		return false;
+	}
+
+	if (!sunit && swidth) {
+		if (!silent)
+			xfs_notice(mp,
+"invalid stripe width (%lld) and stripe unit of 0", swidth);
+		return false;
+	}
+
+	if (sunit > swidth) {
+		if (!silent)
+			xfs_notice(mp,
+"stripe unit (%lld) is larger than the stripe width (%lld)", sunit, swidth);
+		return false;
+	}
+
+	if (sunit && (swidth % sunit)) {
+		if (!silent)
+			xfs_notice(mp,
+"stripe width (%lld) must be a multiple of the stripe unit (%lld)",
+				   swidth, sunit);
+		return false;
+	}
+	return true;
+}
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index 92465a9a..f79f9dc6 100644
--- a/libxfs/xfs_sb.h
+++ b/libxfs/xfs_sb.h
@@ -42,4 +42,7 @@ extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
 				struct xfs_trans *tp, xfs_agnumber_t agno,
 				struct xfs_buf **bpp);
 
+extern bool	xfs_validate_stripe_geometry(struct xfs_mount *mp,
+		__s64 sunit, __s64 swidth, int sectorsize, bool silent);
+
 #endif	/* __XFS_SB_H__ */
-- 
2.18.1

[PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Gao Xiang]

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

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
 libxfs/libxfs_api_defs.h |  1 +
 mkfs/xfs_mkfs.c          | 23 +++++++----------------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index e7e42e93..306d0deb 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -188,6 +188,7 @@
 #define xfs_trans_roll_inode		libxfs_trans_roll_inode
 #define xfs_trans_roll			libxfs_trans_roll
 
+#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
 #define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_agino		libxfs_verify_agino
 #define xfs_verify_cksum		libxfs_verify_cksum
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 8fe149d7..aec40c1f 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2305,12 +2305,6 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}
 
-		if (dsu % cfg->sectorsize) {
-			fprintf(stderr,
-_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
-			usage();
-		}
-
 		dsunit  = (int)BTOBBT(dsu);
 		big_dswidth = (long long int)dsunit * dsw;
 		if (big_dswidth > INT_MAX) {
@@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit (
 		dswidth = big_dswidth;
 	}
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
-	    (dsunit && (dswidth % dsunit != 0))) {
-		fprintf(stderr,
-_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
-			dswidth, dsunit);
+	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)) &&
@@ -2344,11 +2334,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
 
 	/* if no stripe config set, use the device default */
 	if (!dsunit) {
-		/* Ignore nonsense from device.  XXX add more validation */
-		if (ft->dsunit && ft->dswidth == 0) {
+		/* Ignore nonsense from device report. */
+		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
+				BBTOB(ft->dswidth), 0, true)) {
 			fprintf(stderr,
-_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
-				progname, BBTOB(ft->dsunit));
+_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
+				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
 			ft->dsunit = 0;
 			ft->dswidth = 0;
 		} else {
-- 
2.18.1

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Brian Foster]

On Tue, Oct 13, 2020 at 12:06:27PM +0800, Gao Xiang wrote:
> Check stripe numbers in calc_stripe_factors() by using
> xfs_validate_stripe_geometry().
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  libxfs/libxfs_api_defs.h |  1 +
>  mkfs/xfs_mkfs.c          | 23 +++++++----------------
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index e7e42e93..306d0deb 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -188,6 +188,7 @@
>  #define xfs_trans_roll_inode		libxfs_trans_roll_inode
>  #define xfs_trans_roll			libxfs_trans_roll
>  
> +#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
>  #define xfs_verify_agbno		libxfs_verify_agbno
>  #define xfs_verify_agino		libxfs_verify_agino
>  #define xfs_verify_cksum		libxfs_verify_cksum
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 8fe149d7..aec40c1f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2305,12 +2305,6 @@ _("both data su and data sw options must be specified\n"));
>  			usage();
>  		}
>  
> -		if (dsu % cfg->sectorsize) {
> -			fprintf(stderr,
> -_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> -			usage();
> -		}
> -
>  		dsunit  = (int)BTOBBT(dsu);
>  		big_dswidth = (long long int)dsunit * dsw;
>  		if (big_dswidth > INT_MAX) {
> @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit (
>  		dswidth = big_dswidth;
>  	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> -		fprintf(stderr,
> -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> -			dswidth, dsunit);
> +	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)) &&
> @@ -2344,11 +2334,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  
>  	/* if no stripe config set, use the device default */
>  	if (!dsunit) {
> -		/* Ignore nonsense from device.  XXX add more validation */
> -		if (ft->dsunit && ft->dswidth == 0) {
> +		/* Ignore nonsense from device report. */
> +		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
> +				BBTOB(ft->dswidth), 0, true)) {
>  			fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> -				progname, BBTOB(ft->dsunit));
> +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
>  			ft->dsunit = 0;
>  			ft->dswidth = 0;
>  		} else {
> -- 
> 2.18.1
> 

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Darrick J. Wong]

On Tue, Oct 13, 2020 at 12:06:27PM +0800, Gao Xiang wrote:
> Check stripe numbers in calc_stripe_factors() by using
> xfs_validate_stripe_geometry().
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  libxfs/libxfs_api_defs.h |  1 +
>  mkfs/xfs_mkfs.c          | 23 +++++++----------------
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index e7e42e93..306d0deb 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -188,6 +188,7 @@
>  #define xfs_trans_roll_inode		libxfs_trans_roll_inode
>  #define xfs_trans_roll			libxfs_trans_roll
>  
> +#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
>  #define xfs_verify_agbno		libxfs_verify_agbno
>  #define xfs_verify_agino		libxfs_verify_agino
>  #define xfs_verify_cksum		libxfs_verify_cksum
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 8fe149d7..aec40c1f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2305,12 +2305,6 @@ _("both data su and data sw options must be specified\n"));
>  			usage();
>  		}
>  
> -		if (dsu % cfg->sectorsize) {
> -			fprintf(stderr,
> -_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> -			usage();
> -		}
> -
>  		dsunit  = (int)BTOBBT(dsu);
>  		big_dswidth = (long long int)dsunit * dsw;
>  		if (big_dswidth > INT_MAX) {
> @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit (
>  		dswidth = big_dswidth;
>  	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> -		fprintf(stderr,
> -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> -			dswidth, dsunit);
> +	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)) &&
> @@ -2344,11 +2334,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  
>  	/* if no stripe config set, use the device default */
>  	if (!dsunit) {
> -		/* Ignore nonsense from device.  XXX add more validation */
> -		if (ft->dsunit && ft->dswidth == 0) {
> +		/* Ignore nonsense from device report. */
> +		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
> +				BBTOB(ft->dswidth), 0, true)) {
>  			fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> -				progname, BBTOB(ft->dsunit));
> +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
>  			ft->dsunit = 0;
>  			ft->dswidth = 0;
>  		} else {
> -- 
> 2.18.1
> 

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Eric Sandeen]

On 10/12/20 11:06 PM, Gao Xiang wrote:
> Check stripe numbers in calc_stripe_factors() by using
> xfs_validate_stripe_geometry().
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

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.

Thanks,
-Eric

> ---
>  libxfs/libxfs_api_defs.h |  1 +
>  mkfs/xfs_mkfs.c          | 23 +++++++----------------
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index e7e42e93..306d0deb 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -188,6 +188,7 @@
>  #define xfs_trans_roll_inode		libxfs_trans_roll_inode
>  #define xfs_trans_roll			libxfs_trans_roll
>  
> +#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
>  #define xfs_verify_agbno		libxfs_verify_agbno
>  #define xfs_verify_agino		libxfs_verify_agino
>  #define xfs_verify_cksum		libxfs_verify_cksum
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 8fe149d7..aec40c1f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2305,12 +2305,6 @@ _("both data su and data sw options must be specified\n"));
>  			usage();
>  		}
>  
> -		if (dsu % cfg->sectorsize) {
> -			fprintf(stderr,
> -_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> -			usage();
> -		}
> -
>  		dsunit  = (int)BTOBBT(dsu);
>  		big_dswidth = (long long int)dsunit * dsw;
>  		if (big_dswidth > INT_MAX) {
> @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit (
>  		dswidth = big_dswidth;
>  	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> -		fprintf(stderr,
> -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> -			dswidth, dsunit);
> +	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)) &&
> @@ -2344,11 +2334,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  
>  	/* if no stripe config set, use the device default */
>  	if (!dsunit) {
> -		/* Ignore nonsense from device.  XXX add more validation */
> -		if (ft->dsunit && ft->dswidth == 0) {
> +		/* Ignore nonsense from device report. */
> +		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
> +				BBTOB(ft->dswidth), 0, true)) {
>  			fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> -				progname, BBTOB(ft->dsunit));
> +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
>  			ft->dsunit = 0;
>  			ft->dswidth = 0;
>  		} else {
> 

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Gao Xiang]

Hi Eric,

On Mon, Feb 15, 2021 at 07:04:25PM -0600, Eric Sandeen wrote:
> On 10/12/20 11:06 PM, Gao Xiang wrote:
> > Check stripe numbers in calc_stripe_factors() by using
> > xfs_validate_stripe_geometry().
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> 
> 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

> 
> Thanks,
> -Eric
> 
> > ---
> >  libxfs/libxfs_api_defs.h |  1 +
> >  mkfs/xfs_mkfs.c          | 23 +++++++----------------
> >  2 files changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> > index e7e42e93..306d0deb 100644
> > --- a/libxfs/libxfs_api_defs.h
> > +++ b/libxfs/libxfs_api_defs.h
> > @@ -188,6 +188,7 @@
> >  #define xfs_trans_roll_inode		libxfs_trans_roll_inode
> >  #define xfs_trans_roll			libxfs_trans_roll
> >  
> > +#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
> >  #define xfs_verify_agbno		libxfs_verify_agbno
> >  #define xfs_verify_agino		libxfs_verify_agino
> >  #define xfs_verify_cksum		libxfs_verify_cksum
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 8fe149d7..aec40c1f 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -2305,12 +2305,6 @@ _("both data su and data sw options must be specified\n"));
> >  			usage();
> >  		}
> >  
> > -		if (dsu % cfg->sectorsize) {
> > -			fprintf(stderr,
> > -_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> > -			usage();
> > -		}
> > -
> >  		dsunit  = (int)BTOBBT(dsu);
> >  		big_dswidth = (long long int)dsunit * dsw;
> >  		if (big_dswidth > INT_MAX) {
> > @@ -2322,13 +2316,9 @@ _("data stripe width (%lld) is too large of a multiple of the data stripe unit (
> >  		dswidth = big_dswidth;
> >  	}
> >  
> > -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> > -	    (dsunit && (dswidth % dsunit != 0))) {
> > -		fprintf(stderr,
> > -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> > -			dswidth, dsunit);
> > +	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)) &&
> > @@ -2344,11 +2334,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> >  
> >  	/* if no stripe config set, use the device default */
> >  	if (!dsunit) {
> > -		/* Ignore nonsense from device.  XXX add more validation */
> > -		if (ft->dsunit && ft->dswidth == 0) {
> > +		/* Ignore nonsense from device report. */
> > +		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
> > +				BBTOB(ft->dswidth), 0, true)) {
> >  			fprintf(stderr,
> > -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> > -				progname, BBTOB(ft->dsunit));
> > +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> > +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
> >  			ft->dsunit = 0;
> >  			ft->dswidth = 0;
> >  		} else {
> > 
> 

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Gao Xiang]

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:
> > On 10/12/20 11:06 PM, Gao Xiang wrote:
> > > Check stripe numbers in calc_stripe_factors() by using
> > > xfs_validate_stripe_geometry().
> > > 
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > 
> > 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

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Eric Sandeen]

On 2/17/21 11:24 PM, Gao Xiang wrote:
> 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:
>>> On 10/12/20 11:06 PM, Gao Xiang wrote:
>>>> Check stripe numbers in calc_stripe_factors() by using
>>>> xfs_validate_stripe_geometry().
>>>>
>>>> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
>>>
>>> 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();
>> 		}

since we have this check already in xfs_validate_stripe_geometry, it seems best to
keep using it there, and not copy it ... which I think you accomplish below.

>> 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();
>  		}

Just thinking through this... I think this is the right idea.

> -		dsunit  = (int)BTOBBT(dsu);
> -		big_dswidth = (long long int)dsunit * dsw;
> +		big_dswidth = (long long int)dsu * dsw;

dsu is in bytes; this would mean big_dswidth is now also in bytes...
the original goal here, I think, is to not overflow the 32-bit superblock value
for dswidth.

>  		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);

so this used to test big_dswidth in BB (sectors); but now it tests in bytes.

Perhaps this should change to check and report sectors again:

  		if (BTOBBT(big_dswidth) > INT_MAX) {
  			fprintf(stderr,
  _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
  				BTOBBT(big_dswidth), dsunit);

I think the goal is to not overflow the 32-bit on-disk values, which would be
easy to do with "dsw" specified as a /multiplier/ of "dsu"

So I think that if we keep range checking the value in BB units, it will be
OK.

>  			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();
> +	}
Otherwise this looks reasonable to me; now it's basically:

1) If we got geometry in bytes, validate them directly
2) If we got geometry in BB, convert to bytes, and validate
3) If we got no geometry, validate the device-reported defaults

Thanks,
-Eric

>  	/* If sunit & swidth were manually specified as 0, same as noalign */
>  	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
> 

Re: [PATCH v6 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Gao Xiang]

On Thu, Feb 18, 2021 at 10:38:17AM -0600, Eric Sandeen wrote:
> On 2/17/21 11:24 PM, Gao Xiang wrote:
> 

...

> since we have this check already in xfs_validate_stripe_geometry, it seems best to
> keep using it there, and not copy it ... which I think you accomplish below.
> 
> >> 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();
> >  		}
> 
> Just thinking through this... I think this is the right idea.
> 
> > -		dsunit  = (int)BTOBBT(dsu);
> > -		big_dswidth = (long long int)dsunit * dsw;
> > +		big_dswidth = (long long int)dsu * dsw;
> 
> dsu is in bytes; this would mean big_dswidth is now also in bytes...
> the original goal here, I think, is to not overflow the 32-bit superblock value
> for dswidth.

Yeah, agreed. Thanks for catching this.

> 
> >  		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);
> 
> so this used to test big_dswidth in BB (sectors); but now it tests in bytes.
> 
> Perhaps this should change to check and report sectors again:
> 
>   		if (BTOBBT(big_dswidth) > INT_MAX) {
>   			fprintf(stderr,
>   _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
>   				BTOBBT(big_dswidth), dsunit);
> 
> I think the goal is to not overflow the 32-bit on-disk values, which would be
> easy to do with "dsw" specified as a /multiplier/ of "dsu"
> 
> So I think that if we keep range checking the value in BB units, it will be
> OK.
> 
> >  			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();
> > +	}
> Otherwise this looks reasonable to me; now it's basically:
> 
> 1) If we got geometry in bytes, validate them directly
> 2) If we got geometry in BB, convert to bytes, and validate
> 3) If we got no geometry, validate the device-reported defaults
> 

Ok, let me send the next version.

Thanks,
Gao Xiang

> Thanks,
> -Eric
> 
> >  	/* If sunit & swidth were manually specified as 0, same as noalign */
> >  	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&
> > 
> 

[PATCH v7 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Gao Xiang]

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

Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v6:
 - fix dsu round-down issue (the related print message has also
   been turned into bytes to avoid round-down issue);
 - rebase on for-next.

 libxfs/libxfs_api_defs.h |  1 +
 mkfs/xfs_mkfs.c          | 35 +++++++++++++++--------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 9a00ce66..e4192e1b 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -192,6 +192,7 @@
 #define xfs_trans_roll			libxfs_trans_roll
 #define xfs_trim_extent			libxfs_trim_extent
 
+#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
 #define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_agino		libxfs_verify_agino
 #define xfs_verify_cksum		libxfs_verify_cksum
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d72d21ef..dcdd5262 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2361,28 +2361,22 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}
 
-		if (dsu % cfg->sectorsize) {
+		big_dswidth = (long long int)dsu * dsw;
+		if (BTOBBT(big_dswidth) > INT_MAX) {
 			fprintf(stderr,
-_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
+_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
+				big_dswidth, dsu);
 			usage();
 		}
 
-		dsunit  = (int)BTOBBT(dsu);
-		big_dswidth = (long long int)dsunit * 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);
+		if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth,
+						     cfg->sectorsize, false))
 			usage();
-		}
-		dswidth = big_dswidth;
-	}
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
-	    (dsunit && (dswidth % dsunit != 0))) {
-		fprintf(stderr,
-_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
-			dswidth, dsunit);
+		dsunit = BTOBBT(dsu);
+		dswidth = BTOBBT(big_dswidth);
+	} else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit),
+			BBTOB(dswidth), cfg->sectorsize, false)) {
 		usage();
 	}
 
@@ -2400,11 +2394,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
 
 	/* if no stripe config set, use the device default */
 	if (!dsunit) {
-		/* Ignore nonsense from device.  XXX add more validation */
-		if (ft->dsunit && ft->dswidth == 0) {
+		/* Ignore nonsense from device report. */
+		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
+				BBTOB(ft->dswidth), 0, true)) {
 			fprintf(stderr,
-_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
-				progname, BBTOB(ft->dsunit));
+_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
+				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
 			ft->dsunit = 0;
 			ft->dswidth = 0;
 		} else {
-- 
2.27.0

Re: [PATCH v7 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Eric Sandeen]

On 2/18/21 7:37 PM, Gao Xiang wrote:
> Check stripe numbers in calc_stripe_factors() by using
> xfs_validate_stripe_geometry().
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

I think this is good to go now, thank you.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

-Eric

> ---
> changes since v6:
>  - fix dsu round-down issue (the related print message has also
>    been turned into bytes to avoid round-down issue);
>  - rebase on for-next.
> 
>  libxfs/libxfs_api_defs.h |  1 +
>  mkfs/xfs_mkfs.c          | 35 +++++++++++++++--------------------
>  2 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 9a00ce66..e4192e1b 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -192,6 +192,7 @@
>  #define xfs_trans_roll			libxfs_trans_roll
>  #define xfs_trim_extent			libxfs_trim_extent
>  
> +#define xfs_validate_stripe_geometry	libxfs_validate_stripe_geometry
>  #define xfs_verify_agbno		libxfs_verify_agbno
>  #define xfs_verify_agino		libxfs_verify_agino
>  #define xfs_verify_cksum		libxfs_verify_cksum
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index d72d21ef..dcdd5262 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2361,28 +2361,22 @@ _("both data su and data sw options must be specified\n"));
>  			usage();
>  		}
>  
> -		if (dsu % cfg->sectorsize) {
> +		big_dswidth = (long long int)dsu * dsw;
> +		if (BTOBBT(big_dswidth) > INT_MAX) {
>  			fprintf(stderr,
> -_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> +_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> +				big_dswidth, dsu);
>  			usage();
>  		}
>  
> -		dsunit  = (int)BTOBBT(dsu);
> -		big_dswidth = (long long int)dsunit * 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);
> +		if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth,
> +						     cfg->sectorsize, false))
>  			usage();
> -		}
> -		dswidth = big_dswidth;
> -	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> -		fprintf(stderr,
> -_("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> -			dswidth, dsunit);
> +		dsunit = BTOBBT(dsu);
> +		dswidth = BTOBBT(big_dswidth);
> +	} else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit),
> +			BBTOB(dswidth), cfg->sectorsize, false)) {
>  		usage();
>  	}
>  
> @@ -2400,11 +2394,12 @@ _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  
>  	/* if no stripe config set, use the device default */
>  	if (!dsunit) {
> -		/* Ignore nonsense from device.  XXX add more validation */
> -		if (ft->dsunit && ft->dswidth == 0) {
> +		/* Ignore nonsense from device report. */
> +		if (!libxfs_validate_stripe_geometry(NULL, BBTOB(ft->dsunit),
> +				BBTOB(ft->dswidth), 0, true)) {
>  			fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> -				progname, BBTOB(ft->dsunit));
> +_("%s: Volume reports invalid stripe unit (%d) and stripe width (%d), ignoring.\n"),
> +				progname, BBTOB(ft->dsunit), BBTOB(ft->dswidth));
>  			ft->dsunit = 0;
>  			ft->dswidth = 0;
>  		} else {
> 

Re: [PATCH v7 3/3] mkfs: make use of xfs_validate_stripe_geometry()

[Gao Xiang]

On Tue, Feb 23, 2021 at 06:10:45PM -0600, Eric Sandeen wrote:
> On 2/18/21 7:37 PM, Gao Xiang wrote:
> > Check stripe numbers in calc_stripe_factors() by using
> > xfs_validate_stripe_geometry().
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> 
> I think this is good to go now, thank you.
>

Sorry for my careless mistake at that time and
thanks for your review!

Thanks,
Gao Xiang