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

[Gao Xiang <hsiangkao@redhat.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8\"", Size: 10345 bytes --]

Hi Eric,

On Tue, Aug 04, 2020 at 12:55:43PM -0700, Eric Sandeen wrote:
> 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.

Ah, sorry for not noticing that code snippet.

I think dalign check could be outside this helper since it doesn't
need to be considered for the other userspace callers (e.g. if considering
passing in it, it seems needing 2 extra arguments (has_dalign_check and
isdalign and it seems uncessary)?

maybe such condition can be simplified in a line in the
xfs_validate_sb_common() as
	if (!sbp->sb_unit ^ !xfs_sb_version_hasdalign(sbp)) {
		...
		return -FSCURRUPTTED;
	}


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

It seems that it doesn't matter since we could pass (sectorsize == 0)
and use sunit/swidth rather than the specfic dsu/dsw argument approach
to skip the related check.

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

I will try to fold xfs_validate_sb_common() case (in xfsprogs first
for review), the prefix should be xfs_ then?

Thanks,
Gao Xiang

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