Re: [PATCH v2] mkfs.xfs: introduce sunit/swidth validation helper

From: Eric Sandeen <sandeen@sandeen.net>
To: Gao Xiang <hsiangkao@redhat.com>, linux-xfs <linux-xfs@vger.kernel.org>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>,
	Eric Sandeen <sandeen@redhat.com>
Subject: Re: [PATCH v2] mkfs.xfs: introduce sunit/swidth validation helper
Date: Tue, 4 Aug 2020 12:55:43 -0700	[thread overview]
Message-ID: <879f4ac3-4c1f-1890-4be2-04ade8c1e56b@sandeen.net> (raw)
In-Reply-To: <20200804160015.17330-1-hsiangkao@redhat.com>

On 8/4/20 9:00 AM, Gao Xiang wrote:
> Currently stripe unit/swidth checking logic is all over xfsprogs.
> So, refactor the same code snippet into a single validation helper
> xfs_validate_stripe_factors(), including:
>  - integer overflows of either value
>  - sunit and swidth alignment wrt sector size
>  - if either sunit or swidth are zero, both should be zero
>  - swidth must be a multiple of sunit
> 
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v1:
>  - several update (errant space, full names...) suggested by Darrick;
>  - rearrange into a unique handler in calc_stripe_factors();
>  - add libfrog_stripeval_str[] yet I'm not sure if it needs localization;
>  - update po translalation due to (%lld type -> %d).
> 
> (I'd still like to post it in advance...)

Sorry for not commenting sooner.

I wonder - would it be possible to factor out a stripe value validation
helper from xfs_validate_sb_common() in libxfs/xfs_sb.c, so that this
could be called from userspace too?

It is a bit different because kernelspace checks against whether the
superblock has XFS_SB_VERSION_DALIGNBIT set, and that makes no sense
in i.e. blkid_get_topology.

On the other hand, the code below currently checks against sector size,
which seems to be something that kernelspace does not do currently
(but it probably could).

Doing all of this checking in a common location in libxfs for
both userspace and kernelspace seems like it would be a good goal.

Thoughts?

-Eric

>  libfrog/topology.c | 68 +++++++++++++++++++++++++++++++++++++++++
>  libfrog/topology.h | 17 +++++++++++
>  mkfs/xfs_mkfs.c    | 76 ++++++++++++++++++++++++----------------------
>  po/pl.po           |  4 +--
>  4 files changed, 126 insertions(+), 39 deletions(-)
> 
> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..1ce151fd 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -174,6 +174,59 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * This accepts either
> + *  - (sectersize != 0) dsu (in bytes) / dsw (which is mulplier of dsu)
> + * or
> + *  - (sectersize == 0) dunit / dwidth (in 512b sector size)
> + * and return sunit/swidth in sectors.
> + */
> +enum libfrog_stripeval
> +libfrog_validate_stripe_factors(
> +	int	sectorsize,
> +	int	*sunitp,
> +	int	*swidthp)
> +{
> +	int	sunit = *sunitp;
> +	int	swidth = *swidthp;
> +
> +	if (sectorsize) {
> +		long long	big_swidth;
> +
> +		if (sunit % sectorsize)
> +			return LIBFROG_STRIPEVAL_SUNIT_MISALIGN;
> +
> +		sunit = (int)BTOBBT(sunit);
> +		big_swidth = (long long)sunit * swidth;
> +
> +		if (big_swidth > INT_MAX)
> +			return LIBFROG_STRIPEVAL_SWIDTH_OVERFLOW;
> +		swidth = big_swidth;
> +	}
> +
> +	if ((sunit && !swidth) || (!sunit && swidth))
> +		return LIBFROG_STRIPEVAL_PARTIAL_VALID;
> +
> +	if (sunit > swidth)
> +		return LIBFROG_STRIPEVAL_SUNIT_TOO_LARGE;
> +
> +	if (sunit && (swidth % sunit))
> +		return LIBFROG_STRIPEVAL_SWIDTH_MISALIGN;
> +
> +	*sunitp = sunit;
> +	*swidthp = swidth;
> +	return LIBFROG_STRIPEVAL_OK;
> +}
> +
> +const char *libfrog_stripeval_str[] = {
> +	"OK",
> +	"SUNIT_MISALIGN",
> +	"SWIDTH_OVERFLOW",
> +	"PARTIAL_VALID",
> +	"SUNIT_TOO_LARGE",
> +	"SWIDTH_MISALIGN",
> +};
> +
>  static void blkid_get_topology(
>  	const char	*device,
>  	int		*sunit,
> @@ -187,6 +240,7 @@ static void blkid_get_topology(
>  	blkid_probe pr;
>  	unsigned long val;
>  	struct stat statbuf;
> +	enum libfrog_stripeval error;
>  
>  	/* can't get topology info from a file */
>  	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
> @@ -230,6 +284,20 @@ static void blkid_get_topology(
>  	*sunit = *sunit >> 9;
>  	*swidth = *swidth >> 9;
>  
> +	error = libfrog_validate_stripe_factors(0, sunit, swidth);
> +	if (error) {
> +		fprintf(stderr,
> +_("%s: Volume reports invalid sunit (%d bytes) and swidth (%d bytes) %s, ignoring.\n"),
> +			progname, BBTOB(*sunit), BBTOB(*swidth),
> +			libfrog_stripeval_str[error]);
> +		/*
> +		 * if firmware is broken, just give up and set both to zero,
> +		 * we can't trust information from this device.
> +		 */
> +		*sunit = 0;
> +		*swidth = 0;
> +	}
> +
>  	if (blkid_topology_get_alignment_offset(tp) != 0) {
>  		fprintf(stderr,
>  			_("warning: device is not properly aligned %s\n"),
> diff --git a/libfrog/topology.h b/libfrog/topology.h
> index 6fde868a..507fe121 100644
> --- a/libfrog/topology.h
> +++ b/libfrog/topology.h
> @@ -36,4 +36,21 @@ extern int
>  check_overwrite(
>  	const char	*device);
>  
> +enum libfrog_stripeval {
> +	LIBFROG_STRIPEVAL_OK = 0,
> +	LIBFROG_STRIPEVAL_SUNIT_MISALIGN,
> +	LIBFROG_STRIPEVAL_SWIDTH_OVERFLOW,
> +	LIBFROG_STRIPEVAL_PARTIAL_VALID,
> +	LIBFROG_STRIPEVAL_SUNIT_TOO_LARGE,
> +	LIBFROG_STRIPEVAL_SWIDTH_MISALIGN,
> +};
> +
> +extern const char *libfrog_stripeval_str[];
> +
> +enum libfrog_stripeval
> +libfrog_validate_stripe_factors(
> +	int	sectorsize,
> +	int	*sunitp,
> +	int	*swidthp);
> +
>  #endif	/* __LIBFROG_TOPOLOGY_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280..f7b38b36 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2255,14 +2255,14 @@ calc_stripe_factors(
>  	struct cli_params	*cli,
>  	struct fs_topology	*ft)
>  {
> -	long long int	big_dswidth;
> -	int		dsunit = 0;
> -	int		dswidth = 0;
> -	int		lsunit = 0;
> -	int		dsu = 0;
> -	int		dsw = 0;
> -	int		lsu = 0;
> -	bool		use_dev = false;
> +	int			dsunit = 0;
> +	int			dswidth = 0;
> +	int			lsunit = 0;
> +	int			dsu = 0;
> +	int			dsw = 0;
> +	int			lsu = 0;
> +	bool			use_dev = false;
> +	enum libfrog_stripeval	error;
>  
>  	if (cli_opt_set(&dopts, D_SUNIT))
>  		dsunit = cli->dsunit;
> @@ -2289,29 +2289,40 @@ _("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) {
> -			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;
> +		dsunit = dsu;
> +		dswidth = dsw;
> +		error = libfrog_validate_stripe_factors(cfg->sectorsize,
> +				&dsunit, &dswidth);
> +	} else {
> +		error = libfrog_validate_stripe_factors(0, &dsunit, &dswidth);
>  	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> +	switch (error) {
> +	case LIBFROG_STRIPEVAL_OK:
> +		break;
> +	case LIBFROG_STRIPEVAL_SUNIT_MISALIGN:
> +		fprintf(stderr,
> +_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> +		usage();
> +		break;
> +	case LIBFROG_STRIPEVAL_SWIDTH_OVERFLOW:
> +		fprintf(stderr,
> +_("data stripe width (%d) is too large of a multiple of the data stripe unit (%d)\n"),
> +			dsw, dsunit);
> +		usage();
> +		break;
> +	case LIBFROG_STRIPEVAL_PARTIAL_VALID:
> +	case LIBFROG_STRIPEVAL_SWIDTH_MISALIGN:
>  		fprintf(stderr,
>  _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  			dswidth, dsunit);
>  		usage();
> +		break;
> +	default:
> +		fprintf(stderr,
> +_("invalid data stripe unit (%d), width (%d) %s\n"),
> +			dsunit, dswidth, libfrog_stripeval_str[error]);
> +		usage();
>  	}
>  
>  	/* If sunit & swidth were manually specified as 0, same as noalign */
> @@ -2328,18 +2339,9 @@ _("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) {
> -			fprintf(stderr,
> -_("%s: Volume reports stripe unit of %d bytes and stripe width of 0, ignoring.\n"),
> -				progname, BBTOB(ft->dsunit));
> -			ft->dsunit = 0;
> -			ft->dswidth = 0;
> -		} else {
> -			dsunit = ft->dsunit;
> -			dswidth = ft->dswidth;
> -			use_dev = true;
> -		}
> +		dsunit = ft->dsunit;
> +		dswidth = ft->dswidth;
> +		use_dev = true;
>  	} else {
>  		/* check and warn if user-specified alignment is sub-optimal */
>  		if (ft->dsunit && ft->dsunit != dsunit) {
> diff --git a/po/pl.po b/po/pl.po
> index 87109f6b..02d2258f 100644
> --- a/po/pl.po
> +++ b/po/pl.po
> @@ -9085,10 +9085,10 @@ msgstr "su danych musi być wielokrotnością rozmiaru sektora (%d)\n"
>  #: .././mkfs/xfs_mkfs.c:2267
>  #, c-format
>  msgid ""
> -"data stripe width (%lld) is too large of a multiple of the data stripe unit "
> +"data stripe width (%d) is too large of a multiple of the data stripe unit "
>  "(%d)\n"
>  msgstr ""
> -"szerokość pasa danych (%lld) jest zbyt dużą wielokrotnością jednostki pasa "
> +"szerokość pasa danych (%d) jest zbyt dużą wielokrotnością jednostki pasa "
>  "danych (%d)\n"
>  
>  #: .././mkfs/xfs_mkfs.c:2276
> 