[PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[Eric Sandeen]

We validate commandline options for stripe unit and stripe width, and
if a device returns nonsensical values via libblkid, the superbock write
verifier will eventually catch it and fail (noisily and cryptically) but
it seems a bit cleaner to just do a basic sanity check on the numbers
as soon as we get them from blkid, and if they're bogus, ignore them from
the start.

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

diff --git a/libfrog/topology.c b/libfrog/topology.c
index b1b470c9..38ed03b7 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -213,6 +213,19 @@ static void blkid_get_topology(
 	val = blkid_topology_get_optimal_io_size(tp);
 	*swidth = val;
 
+        /*
+	 * Occasionally, firmware is broken and returns optimal < minimum,
+	 * or optimal which is not a multiple of minimum.
+	 * In that case just give up and set both to zero, we can't trust
+	 * information from this device. Similar to xfs_validate_sb_common().
+	 */
+        if (*sunit) {
+                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {
+                        *sunit = 0;
+                        *swidth = 0;
+                }
+        }
+
 	/*
 	 * If the reported values are the same as the physical sector size
 	 * do not bother to report anything.  It will only cause warnings

Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[Darrick J. Wong]

On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
> We validate commandline options for stripe unit and stripe width, and
> if a device returns nonsensical values via libblkid, the superbock write
> verifier will eventually catch it and fail (noisily and cryptically) but
> it seems a bit cleaner to just do a basic sanity check on the numbers
> as soon as we get them from blkid, and if they're bogus, ignore them from
> the start.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..38ed03b7 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -213,6 +213,19 @@ static void blkid_get_topology(
>  	val = blkid_topology_get_optimal_io_size(tp);
>  	*swidth = val;
>  
> +        /*
> +	 * Occasionally, firmware is broken and returns optimal < minimum,
> +	 * or optimal which is not a multiple of minimum.
> +	 * In that case just give up and set both to zero, we can't trust
> +	 * information from this device. Similar to xfs_validate_sb_common().
> +	 */
> +        if (*sunit) {
> +                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {

I feel like we're copypasting this sunit/swidth checking logic all over
xfsprogs and yet we're still losing the stripe unit validation whackamole
game.

In the end, we want to check more or less the same things for each pair
of stripe unit and stripe width:

 * 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

All four of these rules apply to the blkid_get_toplogy answers for the
data device, the log device, and the realtime device; and any mkfs CLI
overrides of those values.

IOWs, is there some way to refactor those four rules into a single
validation function and call that in the six(ish) places we need it?
Especially since you're the one who played the last round of whackamole,
back in May 2018. :)

--D

> +                        *sunit = 0;
> +                        *swidth = 0;
> +                }
> +        }
> +
>  	/*
>  	 * If the reported values are the same as the physical sector size
>  	 * do not bother to report anything.  It will only cause warnings
> 

Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[Eric Sandeen]

On 5/15/20 3:48 PM, Darrick J. Wong wrote:
> On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
>> We validate commandline options for stripe unit and stripe width, and
>> if a device returns nonsensical values via libblkid, the superbock write
>> verifier will eventually catch it and fail (noisily and cryptically) but
>> it seems a bit cleaner to just do a basic sanity check on the numbers
>> as soon as we get them from blkid, and if they're bogus, ignore them from
>> the start.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/libfrog/topology.c b/libfrog/topology.c
>> index b1b470c9..38ed03b7 100644
>> --- a/libfrog/topology.c
>> +++ b/libfrog/topology.c
>> @@ -213,6 +213,19 @@ static void blkid_get_topology(
>>  	val = blkid_topology_get_optimal_io_size(tp);
>>  	*swidth = val;
>>  
>> +        /*
>> +	 * Occasionally, firmware is broken and returns optimal < minimum,
>> +	 * or optimal which is not a multiple of minimum.
>> +	 * In that case just give up and set both to zero, we can't trust
>> +	 * information from this device. Similar to xfs_validate_sb_common().
>> +	 */
>> +        if (*sunit) {
>> +                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {
> 
> I feel like we're copypasting this sunit/swidth checking logic all over
> xfsprogs 

That's because we are!

> and yet we're still losing the stripe unit validation whackamole
> game.

Need moar hammers!

> In the end, we want to check more or less the same things for each pair
> of stripe unit and stripe width:
> 
>  * 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
> 
> All four of these rules apply to the blkid_get_toplogy answers for the
> data device, the log device, and the realtime device; and any mkfs CLI
> overrides of those values.
> 
> IOWs, is there some way to refactor those four rules into a single
> validation function and call that in the six(ish) places we need it?
> Especially since you're the one who played the last round of whackamole,
> back in May 2018. :)

So .... I would like to do that refactoring.  I'd also like to fix this
with some expediency, TBH...

Refactoring is going to be a little more complicated, I fear, because sanity
on "what came straight from blkid" is a little different from "what came from
cmdline" and has slightly different checks than "how does it fit into the
superblock we just read?"

This (swidth-vs-sunit-is-borken) is common enough that I wanted to just kill
it with fire, and um ... make it all better/cohesive at some later date.

I don't like arguing for expediency over beauty but well... here I am.

-Eric

> --D
> 
>> +                        *sunit = 0;
>> +                        *swidth = 0;
>> +                }
>> +        }
>> +
>>  	/*
>>  	 * If the reported values are the same as the physical sector size
>>  	 * do not bother to report anything.  It will only cause warnings
>>
> 

Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[Eric Sandeen]

On 5/15/20 3:54 PM, Eric Sandeen wrote:
> On 5/15/20 3:48 PM, Darrick J. Wong wrote:
>> On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
>>> We validate commandline options for stripe unit and stripe width, and
>>> if a device returns nonsensical values via libblkid, the superbock write
>>> verifier will eventually catch it and fail (noisily and cryptically) but
>>> it seems a bit cleaner to just do a basic sanity check on the numbers
>>> as soon as we get them from blkid, and if they're bogus, ignore them from
>>> the start.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>
>>> diff --git a/libfrog/topology.c b/libfrog/topology.c
>>> index b1b470c9..38ed03b7 100644
>>> --- a/libfrog/topology.c
>>> +++ b/libfrog/topology.c
>>> @@ -213,6 +213,19 @@ static void blkid_get_topology(
>>>  	val = blkid_topology_get_optimal_io_size(tp);
>>>  	*swidth = val;
>>>  
>>> +        /*
>>> +	 * Occasionally, firmware is broken and returns optimal < minimum,
>>> +	 * or optimal which is not a multiple of minimum.
>>> +	 * In that case just give up and set both to zero, we can't trust
>>> +	 * information from this device. Similar to xfs_validate_sb_common().
>>> +	 */
>>> +        if (*sunit) {
>>> +                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {
>>
>> I feel like we're copypasting this sunit/swidth checking logic all over
>> xfsprogs 
> 
> That's because we are!
> 
>> and yet we're still losing the stripe unit validation whackamole
>> game.
> 
> Need moar hammers!
> 
>> In the end, we want to check more or less the same things for each pair
>> of stripe unit and stripe width:
>>
>>  * 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
>>
>> All four of these rules apply to the blkid_get_toplogy answers for the
>> data device, the log device, and the realtime device; and any mkfs CLI
>> overrides of those values.
>>
>> IOWs, is there some way to refactor those four rules into a single
>> validation function and call that in the six(ish) places we need it?
>> Especially since you're the one who played the last round of whackamole,
>> back in May 2018. :)
> 
> So .... I would like to do that refactoring.  I'd also like to fix this
> with some expediency, TBH...
> 
> Refactoring is going to be a little more complicated, I fear, because sanity
> on "what came straight from blkid" is a little different from "what came from
> cmdline" and has slightly different checks than "how does it fit into the
> superblock we just read?"
> 
> This (swidth-vs-sunit-is-borken) is common enough that I wanted to just kill
> it with fire, and um ... make it all better/cohesive at some later date.
> 
> I don't like arguing for expediency over beauty but well... here I am.

A little more context, failures like this are caught today, but only
in the validators, and it's very noisy and nonobvious when it happens.
And we seem to keep finding devices that are broken like this, so would be
nice to just ignore them and move on.

-Eric

Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[Darrick J. Wong]

On Fri, May 15, 2020 at 03:54:34PM -0500, Eric Sandeen wrote:
> On 5/15/20 3:48 PM, Darrick J. Wong wrote:
> > On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
> >> We validate commandline options for stripe unit and stripe width, and
> >> if a device returns nonsensical values via libblkid, the superbock write
> >> verifier will eventually catch it and fail (noisily and cryptically) but
> >> it seems a bit cleaner to just do a basic sanity check on the numbers
> >> as soon as we get them from blkid, and if they're bogus, ignore them from
> >> the start.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/libfrog/topology.c b/libfrog/topology.c
> >> index b1b470c9..38ed03b7 100644
> >> --- a/libfrog/topology.c
> >> +++ b/libfrog/topology.c
> >> @@ -213,6 +213,19 @@ static void blkid_get_topology(
> >>  	val = blkid_topology_get_optimal_io_size(tp);
> >>  	*swidth = val;
> >>  
> >> +        /*

Tabs not spaces

> >> +	 * Occasionally, firmware is broken and returns optimal < minimum,
> >> +	 * or optimal which is not a multiple of minimum.
> >> +	 * In that case just give up and set both to zero, we can't trust
> >> +	 * information from this device. Similar to xfs_validate_sb_common().
> >> +	 */
> >> +        if (*sunit) {
> >> +                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {

Why not combine these?

if (*sunit != 0 && (*sunit > *swidth || *swidth % *sunit != 0)) {

Aside from that the code looks fine I guess...

> > I feel like we're copypasting this sunit/swidth checking logic all over
> > xfsprogs 
> 
> That's because we are!
> 
> > and yet we're still losing the stripe unit validation whackamole
> > game.
> 
> Need moar hammers!
> 
> > In the end, we want to check more or less the same things for each pair
> > of stripe unit and stripe width:
> > 
> >  * 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
> > 
> > All four of these rules apply to the blkid_get_toplogy answers for the
> > data device, the log device, and the realtime device; and any mkfs CLI
> > overrides of those values.
> > 
> > IOWs, is there some way to refactor those four rules into a single
> > validation function and call that in the six(ish) places we need it?
> > Especially since you're the one who played the last round of whackamole,
> > back in May 2018. :)
> 
> So .... I would like to do that refactoring.  I'd also like to fix this
> with some expediency, TBH...
> 
> Refactoring is going to be a little more complicated, I fear, because sanity
> on "what came straight from blkid" is a little different from "what came from
> cmdline" and has slightly different checks than "how does it fit into the
> superblock we just read?"

Admittedly I wondered if "refactor all these checks" would fall apart
because each tool has its own slightly different reporting and logging
requirements.  You could make a checker function return an enum of what
it's mad about and each caller could either have a message catalogue or
just bail depending on the circumstances, but now I've probably
overengineered the corner case catching code.

> This (swidth-vs-sunit-is-borken) is common enough that I wanted to just kill
> it with fire, and um ... make it all better/cohesive at some later date.
> 
> I don't like arguing for expediency over beauty but well... here I am.

:(

--D

> -Eric
> 
> > --D
> > 
> >> +                        *sunit = 0;
> >> +                        *swidth = 0;
> >> +                }
> >> +        }
> >> +
> >>  	/*
> >>  	 * If the reported values are the same as the physical sector size
> >>  	 * do not bother to report anything.  It will only cause warnings
> >>
> > 
> 

Re: [PATCH] mkfs.xfs: sanity check stripe geometry from blkid

[Eric Sandeen]

On 5/15/20 4:10 PM, Darrick J. Wong wrote:
> On Fri, May 15, 2020 at 03:54:34PM -0500, Eric Sandeen wrote:
>> On 5/15/20 3:48 PM, Darrick J. Wong wrote:
>>> On Fri, May 15, 2020 at 02:14:17PM -0500, Eric Sandeen wrote:
>>>> We validate commandline options for stripe unit and stripe width, and
>>>> if a device returns nonsensical values via libblkid, the superbock write
>>>> verifier will eventually catch it and fail (noisily and cryptically) but
>>>> it seems a bit cleaner to just do a basic sanity check on the numbers
>>>> as soon as we get them from blkid, and if they're bogus, ignore them from
>>>> the start.
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>
>>>> diff --git a/libfrog/topology.c b/libfrog/topology.c
>>>> index b1b470c9..38ed03b7 100644
>>>> --- a/libfrog/topology.c
>>>> +++ b/libfrog/topology.c
>>>> @@ -213,6 +213,19 @@ static void blkid_get_topology(
>>>>  	val = blkid_topology_get_optimal_io_size(tp);
>>>>  	*swidth = val;
>>>>  
>>>> +        /*
> 
> Tabs not spaces

yeah I have no idea how that happened :P

>>>> +	 * Occasionally, firmware is broken and returns optimal < minimum,
>>>> +	 * or optimal which is not a multiple of minimum.
>>>> +	 * In that case just give up and set both to zero, we can't trust
>>>> +	 * information from this device. Similar to xfs_validate_sb_common().
>>>> +	 */
>>>> +        if (*sunit) {
>>>> +                if ((*sunit > *swidth) || (*swidth % *sunit != 0)) {
> 
> Why not combine these?
> 
> if (*sunit != 0 && (*sunit > *swidth || *swidth % *sunit != 0)) {

was making it look a little like the kernel sb checks but *shrug*

> Aside from that the code looks fine I guess...
> 
>>> I feel like we're copypasting this sunit/swidth checking logic all over
>>> xfsprogs 
>>
>> That's because we are!
>>
>>> and yet we're still losing the stripe unit validation whackamole
>>> game.
>>
>> Need moar hammers!
>>
>>> In the end, we want to check more or less the same things for each pair
>>> of stripe unit and stripe width:
>>>
>>>  * 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
>>>
>>> All four of these rules apply to the blkid_get_toplogy answers for the
>>> data device, the log device, and the realtime device; and any mkfs CLI
>>> overrides of those values.
>>>
>>> IOWs, is there some way to refactor those four rules into a single
>>> validation function and call that in the six(ish) places we need it?
>>> Especially since you're the one who played the last round of whackamole,
>>> back in May 2018. :)
>>
>> So .... I would like to do that refactoring.  I'd also like to fix this
>> with some expediency, TBH...
>>
>> Refactoring is going to be a little more complicated, I fear, because sanity
>> on "what came straight from blkid" is a little different from "what came from
>> cmdline" and has slightly different checks than "how does it fit into the
>> superblock we just read?"
> 
> Admittedly I wondered if "refactor all these checks" would fall apart
> because each tool has its own slightly different reporting and logging
> requirements.  You could make a checker function return an enum of what
> it's mad about and each caller could either have a message catalogue or
> just bail depending on the circumstances, but now I've probably
> overengineered the corner case catching code.
> 
>> This (swidth-vs-sunit-is-borken) is common enough that I wanted to just kill
>> it with fire, and um ... make it all better/cohesive at some later date.
>>
>> I don't like arguing for expediency over beauty but well... here I am.
> 
> :(

Well, I guess it's not actually that urgent; we don't handle it well
today but maybe I should resist the urge to do another spot-fix that
could(?) be handled better....

i'll put this on the backburner & give it a bit more thought I guess.

Thanks,
-Eric

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

[Gao Xiang]

Currently stripe unit/width 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>
---

This patch follows Darrick's original suggestion [1], yet I'm
not sure if I'm doing the right thing or if something is still
missing (e.g the meaning of six(ish) places)... So post it
right now...

TBH, especially all these naming and the helper location (whether
in topology.c)...plus, click a dislike on calc_stripe_factors()
itself...

(Hopefully hear some advice about this... Thanks!)

[1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia

 libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 libfrog/topology.h | 15 ++++++++++++++
 mkfs/xfs_mkfs.c    | 48 ++++++++++++++++++++++----------------------
 3 files changed, 89 insertions(+), 24 deletions(-)

diff --git a/libfrog/topology.c b/libfrog/topology.c
index b1b470c9..cf56fb03 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -174,6 +174,41 @@ out:
 	return ret;
 }
 
+enum xfs_stripe_retcode
+xfs_validate_stripe_factors(
+	int	sectorsize,
+	int 	*sup,
+	int	*swp)
+{
+	int sunit = *sup, swidth = *swp;
+
+	if (sectorsize) {
+		long long	big_swidth;
+
+		if (sunit % sectorsize)
+			return XFS_STRIPE_RET_SUNIT_MISALIGN;
+
+		sunit = (int)BTOBBT(sunit);
+		big_swidth = (long long)sunit * swidth;
+
+		if (big_swidth > INT_MAX)
+			return XFS_STRIPE_RET_SWIDTH_OVERFLOW;
+		swidth = big_swidth;
+	}
+	if ((sunit && !swidth) || (!sunit && swidth))
+		return XFS_STRIPE_RET_PARTIAL_VALID;
+
+	if (sunit > swidth)
+		return XFS_STRIPE_RET_SUNIT_TOO_LARGE;
+
+	if (sunit && (swidth % sunit))
+		return XFS_STRIPE_RET_SWIDTH_MISALIGN;
+
+	*sup = sunit;
+	*swp = swidth;
+	return XFS_STRIPE_RET_OK;
+}
+
 static void blkid_get_topology(
 	const char	*device,
 	int		*sunit,
@@ -229,6 +264,21 @@ static void blkid_get_topology(
 	 */
 	*sunit = *sunit >> 9;
 	*swidth = *swidth >> 9;
+	switch (xfs_validate_stripe_factors(0, sunit, swidth)) {
+	case XFS_STRIPE_RET_OK:
+		break;
+	case XFS_STRIPE_RET_PARTIAL_VALID:
+		fprintf(stderr,
+_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"),
+				progname, BBTOB(*sunit), BBTOB(*swidth));
+	default:
+		/*
+		 * 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,
diff --git a/libfrog/topology.h b/libfrog/topology.h
index 6fde868a..e8be26b2 100644
--- a/libfrog/topology.h
+++ b/libfrog/topology.h
@@ -36,4 +36,19 @@ extern int
 check_overwrite(
 	const char	*device);
 
+enum xfs_stripe_retcode {
+	XFS_STRIPE_RET_OK = 0,
+	XFS_STRIPE_RET_SUNIT_MISALIGN,
+	XFS_STRIPE_RET_SWIDTH_OVERFLOW,
+	XFS_STRIPE_RET_PARTIAL_VALID,
+	XFS_STRIPE_RET_SUNIT_TOO_LARGE,
+	XFS_STRIPE_RET_SWIDTH_MISALIGN,
+};
+
+enum xfs_stripe_retcode
+xfs_validate_stripe_factors(
+	int	sectorsize,
+	int 	*sup,
+	int	*swp);
+
 #endif	/* __LIBFROG_TOPOLOGY_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280..a3d6032c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2255,7 +2255,6 @@ 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;
@@ -2263,6 +2262,7 @@ calc_stripe_factors(
 	int		dsw = 0;
 	int		lsu = 0;
 	bool		use_dev = false;
+	int		error;
 
 	if (cli_opt_set(&dopts, D_SUNIT))
 		dsunit = cli->dsunit;
@@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}
 
-		if (dsu % cfg->sectorsize) {
+		dsunit = dsu;
+		dswidth = dsw;
+		error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth);
+		switch(error) {
+		case XFS_STRIPE_RET_SUNIT_MISALIGN:
 			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) {
+			break;
+		case XFS_STRIPE_RET_SWIDTH_OVERFLOW:
 			fprintf(stderr,
-_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
-				big_dswidth, dsunit);
+_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"),
+				dsw, dsunit);
 			usage();
+			break;
 		}
-		dswidth = big_dswidth;
+	} else {
+		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
 	}
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
-	    (dsunit && (dswidth % dsunit != 0))) {
+	if (error == XFS_STRIPE_RET_PARTIAL_VALID ||
+	    error == XFS_STRIPE_RET_SWIDTH_MISALIGN) {
 		fprintf(stderr,
 _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
 			dswidth, dsunit);
 		usage();
 	}
 
+	if (error) {
+		fprintf(stderr,
+_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth);
+		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)) &&
 	    !dsunit && !dswidth)
@@ -2328,18 +2337,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) {
-- 
2.18.1

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

[Darrick J. Wong]

On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote:
> Currently stripe unit/width 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>
> ---
> 
> This patch follows Darrick's original suggestion [1], yet I'm
> not sure if I'm doing the right thing or if something is still
> missing (e.g the meaning of six(ish) places)... So post it
> right now...
> 
> TBH, especially all these naming and the helper location (whether
> in topology.c)...plus, click a dislike on calc_stripe_factors()
> itself...
> 
> (Hopefully hear some advice about this... Thanks!)
> 
> [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia
> 
>  libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  libfrog/topology.h | 15 ++++++++++++++
>  mkfs/xfs_mkfs.c    | 48 ++++++++++++++++++++++----------------------
>  3 files changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..cf56fb03 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -174,6 +174,41 @@ out:
>  	return ret;
>  }
>  
> +enum xfs_stripe_retcode
> +xfs_validate_stripe_factors(

libfrog functions (and enums) should be prefixed with libfrog, not xfs.

LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.}

> +	int	sectorsize,
> +	int 	*sup,

Errant space between "int" and "*sup".

> +	int	*swp)

Strange that a validator function has out parameters...

Also, uh, .... full names, please.

	int	*sunitp,
	int	*swidthp)

(I'm vaguely wondering why we use signed ints here vs. unsigned, but
that isn't critical...)

> +{
> +	int sunit = *sup, swidth = *swp;
> +
> +	if (sectorsize) {
> +		long long	big_swidth;
> +
> +		if (sunit % sectorsize)
> +			return XFS_STRIPE_RET_SUNIT_MISALIGN;
> +
> +		sunit = (int)BTOBBT(sunit);

Hmm.  On input, *sup is in units of bytes, but on output it can be in
units of 512b blocks?  That is very surprising...

> +		big_swidth = (long long)sunit * swidth;
> +
> +		if (big_swidth > INT_MAX)
> +			return XFS_STRIPE_RET_SWIDTH_OVERFLOW;
> +		swidth = big_swidth;
> +	}
> +	if ((sunit && !swidth) || (!sunit && swidth))
> +		return XFS_STRIPE_RET_PARTIAL_VALID;
> +
> +	if (sunit > swidth)
> +		return XFS_STRIPE_RET_SUNIT_TOO_LARGE;
> +
> +	if (sunit && (swidth % sunit))
> +		return XFS_STRIPE_RET_SWIDTH_MISALIGN;
> +
> +	*sup = sunit;

...especially since in the !sectorsize case we don't change it at all.

> +	*swp = swidth;
> +	return XFS_STRIPE_RET_OK;
> +}
> +
>  static void blkid_get_topology(
>  	const char	*device,
>  	int		*sunit,
> @@ -229,6 +264,21 @@ static void blkid_get_topology(
>  	 */
>  	*sunit = *sunit >> 9;
>  	*swidth = *swidth >> 9;
> +	switch (xfs_validate_stripe_factors(0, sunit, swidth)) {
> +	case XFS_STRIPE_RET_OK:
> +		break;
> +	case XFS_STRIPE_RET_PARTIAL_VALID:
> +		fprintf(stderr,
> +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"),
> +				progname, BBTOB(*sunit), BBTOB(*swidth));

Needs a "/* fallthrough */" comment here.

> +	default:

Why don't we warn about receiving garbage geometry that produces
MISALIGN or OVERFLOW?

> +		/*
> +		 * 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,
> diff --git a/libfrog/topology.h b/libfrog/topology.h
> index 6fde868a..e8be26b2 100644
> --- a/libfrog/topology.h
> +++ b/libfrog/topology.h
> @@ -36,4 +36,19 @@ extern int
>  check_overwrite(
>  	const char	*device);
>  
> +enum xfs_stripe_retcode {
> +	XFS_STRIPE_RET_OK = 0,
> +	XFS_STRIPE_RET_SUNIT_MISALIGN,
> +	XFS_STRIPE_RET_SWIDTH_OVERFLOW,
> +	XFS_STRIPE_RET_PARTIAL_VALID,
> +	XFS_STRIPE_RET_SUNIT_TOO_LARGE,
> +	XFS_STRIPE_RET_SWIDTH_MISALIGN,
> +};
> +
> +enum xfs_stripe_retcode
> +xfs_validate_stripe_factors(
> +	int	sectorsize,
> +	int 	*sup,

Errant space between "int" and "*sup".

> +	int	*swp);
> +
>  #endif	/* __LIBFROG_TOPOLOGY_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280..a3d6032c 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2255,7 +2255,6 @@ 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;
> @@ -2263,6 +2262,7 @@ calc_stripe_factors(
>  	int		dsw = 0;
>  	int		lsu = 0;
>  	bool		use_dev = false;
> +	int		error;
>  
>  	if (cli_opt_set(&dopts, D_SUNIT))
>  		dsunit = cli->dsunit;
> @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n"));
>  			usage();
>  		}
>  
> -		if (dsu % cfg->sectorsize) {
> +		dsunit = dsu;
> +		dswidth = dsw;
> +		error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth);

I thought this function returned an enum?

> +		switch(error) {
> +		case XFS_STRIPE_RET_SUNIT_MISALIGN:
>  			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) {
> +			break;
> +		case XFS_STRIPE_RET_SWIDTH_OVERFLOW:
>  			fprintf(stderr,
> -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> -				big_dswidth, dsunit);
> +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"),

Why change this message?

> +				dsw, dsunit);
>  			usage();
> +			break;
>  		}
> -		dswidth = big_dswidth;
> +	} else {
> +		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
>  	}
>  
> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> +	if (error == XFS_STRIPE_RET_PARTIAL_VALID ||
> +	    error == XFS_STRIPE_RET_SWIDTH_MISALIGN) {
>  		fprintf(stderr,
>  _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
>  			dswidth, dsunit);
>  		usage();
>  	}
>  
> +	if (error) {
> +		fprintf(stderr,
> +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth);

Invalid how?  We know the exact reason, so we should say so.

--D

> +		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)) &&
>  	    !dsunit && !dswidth)
> @@ -2328,18 +2337,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) {
> -- 
> 2.18.1
> 

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

[Gao Xiang]

Hi Darrick,

On Mon, Aug 03, 2020 at 08:26:09AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 03, 2020 at 08:50:18PM +0800, Gao Xiang wrote:
> > Currently stripe unit/width 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>
> > ---
> > 
> > This patch follows Darrick's original suggestion [1], yet I'm
> > not sure if I'm doing the right thing or if something is still
> > missing (e.g the meaning of six(ish) places)... So post it
> > right now...
> > 
> > TBH, especially all these naming and the helper location (whether
> > in topology.c)...plus, click a dislike on calc_stripe_factors()
> > itself...
> > 
> > (Hopefully hear some advice about this... Thanks!)
> > 
> > [1] https://lore.kernel.org/r/20200515204802.GO6714@magnolia
> > 
> >  libfrog/topology.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++
> >  libfrog/topology.h | 15 ++++++++++++++
> >  mkfs/xfs_mkfs.c    | 48 ++++++++++++++++++++++----------------------
> >  3 files changed, 89 insertions(+), 24 deletions(-)
> > 
> > diff --git a/libfrog/topology.c b/libfrog/topology.c
> > index b1b470c9..cf56fb03 100644
> > --- a/libfrog/topology.c
> > +++ b/libfrog/topology.c
> > @@ -174,6 +174,41 @@ out:
> >  	return ret;
> >  }
> >  
> > +enum xfs_stripe_retcode
> > +xfs_validate_stripe_factors(
> 
> libfrog functions (and enums) should be prefixed with libfrog, not xfs.
> 
> LIBFROG_STRIPEVAL_{OK,SUNIT_MISALIGN, etc.}
> 
> > +	int	sectorsize,
> > +	int 	*sup,
> 
> Errant space between "int" and "*sup".

Ack. Sorry about that.

> 
> > +	int	*swp)
> 
> Strange that a validator function has out parameters...
> 
> Also, uh, .... full names, please.

see the reasons below...

> 
> 	int	*sunitp,
> 	int	*swidthp)
> 
> (I'm vaguely wondering why we use signed ints here vs. unsigned, but
> that isn't critical...)

That is because I saw many previous "sunit/swidth" usage in the codebase
by using "int" rather than "unsigned int". I don't have much tendency
of this. (either form is ok with me since signed int is also enough here.)

> 
> > +{
> > +	int sunit = *sup, swidth = *swp;
> > +
> > +	if (sectorsize) {
> > +		long long	big_swidth;
> > +
> > +		if (sunit % sectorsize)
> > +			return XFS_STRIPE_RET_SUNIT_MISALIGN;
> > +
> > +		sunit = (int)BTOBBT(sunit);
> 
> Hmm.  On input, *sup is in units of bytes, but on output it can be in
> units of 512b blocks?  That is very surprising...

Yeah, It seems a bit weird at first. But I have no better idea
how to fulfill/wrap up "- sunit and swidth alignment wrt sector
size" check from the original thread in the validator helper.

So I finally implemented the helper in a form which accepts
either:
 [1] (sectersize != 0) dsu (in bytes) / dsw (which is multiple of dsu)

Or
 [2] (sectersize == 0) dunit / dwidth (in 512b sector size)

In [1], dsu and dsw would be turned into dunit / dwidth finally...


Yeah, that's my premature thought about this tho... hope for better
idea about this :)

> 
> > +		big_swidth = (long long)sunit * swidth;
> > +
> > +		if (big_swidth > INT_MAX)
> > +			return XFS_STRIPE_RET_SWIDTH_OVERFLOW;
> > +		swidth = big_swidth;
> > +	}
> > +	if ((sunit && !swidth) || (!sunit && swidth))
> > +		return XFS_STRIPE_RET_PARTIAL_VALID;
> > +
> > +	if (sunit > swidth)
> > +		return XFS_STRIPE_RET_SUNIT_TOO_LARGE;
> > +
> > +	if (sunit && (swidth % sunit))
> > +		return XFS_STRIPE_RET_SWIDTH_MISALIGN;
> > +
> > +	*sup = sunit;
> 
> ...especially since in the !sectorsize case we don't change it at all.

Yeah...

> 
> > +	*swp = swidth;
> > +	return XFS_STRIPE_RET_OK;
> > +}
> > +
> >  static void blkid_get_topology(
> >  	const char	*device,
> >  	int		*sunit,
> > @@ -229,6 +264,21 @@ static void blkid_get_topology(
> >  	 */
> >  	*sunit = *sunit >> 9;
> >  	*swidth = *swidth >> 9;
> > +	switch (xfs_validate_stripe_factors(0, sunit, swidth)) {
> > +	case XFS_STRIPE_RET_OK:
> > +		break;
> > +	case XFS_STRIPE_RET_PARTIAL_VALID:
> > +		fprintf(stderr,
> > +_("%s: Volume reports stripe unit of %d bytes and stripe width of %d bytes, ignoring.\n"),
> > +				progname, BBTOB(*sunit), BBTOB(*swidth));
> 
> Needs a "/* fallthrough */" comment here.

Ack.

> 
> > +	default:
> 
> Why don't we warn about receiving garbage geometry that produces
> MISALIGN or OVERFLOW?

Okay, I could add them in the next version...
Yet I still suspect my broken English works... :)

> 
> > +		/*
> > +		 * 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,
> > diff --git a/libfrog/topology.h b/libfrog/topology.h
> > index 6fde868a..e8be26b2 100644
> > --- a/libfrog/topology.h
> > +++ b/libfrog/topology.h
> > @@ -36,4 +36,19 @@ extern int
> >  check_overwrite(
> >  	const char	*device);
> >  
> > +enum xfs_stripe_retcode {
> > +	XFS_STRIPE_RET_OK = 0,
> > +	XFS_STRIPE_RET_SUNIT_MISALIGN,
> > +	XFS_STRIPE_RET_SWIDTH_OVERFLOW,
> > +	XFS_STRIPE_RET_PARTIAL_VALID,
> > +	XFS_STRIPE_RET_SUNIT_TOO_LARGE,
> > +	XFS_STRIPE_RET_SWIDTH_MISALIGN,
> > +};
> > +
> > +enum xfs_stripe_retcode
> > +xfs_validate_stripe_factors(
> > +	int	sectorsize,
> > +	int 	*sup,
> 
> Errant space between "int" and "*sup".

Sorry, copy again.

> 
> > +	int	*swp);
> > +
> >  #endif	/* __LIBFROG_TOPOLOGY_H__ */
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2e6cd280..a3d6032c 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -2255,7 +2255,6 @@ 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;
> > @@ -2263,6 +2262,7 @@ calc_stripe_factors(
> >  	int		dsw = 0;
> >  	int		lsu = 0;
> >  	bool		use_dev = false;
> > +	int		error;
> >  
> >  	if (cli_opt_set(&dopts, D_SUNIT))
> >  		dsunit = cli->dsunit;
> > @@ -2289,31 +2289,40 @@ _("both data su and data sw options must be specified\n"));
> >  			usage();
> >  		}
> >  
> > -		if (dsu % cfg->sectorsize) {
> > +		dsunit = dsu;
> > +		dswidth = dsw;
> > +		error = xfs_validate_stripe_factors(cfg->sectorsize, &dsunit, &dswidth);
> 
> I thought this function returned an enum?

okay, will update in the next version.

> 
> > +		switch(error) {
> > +		case XFS_STRIPE_RET_SUNIT_MISALIGN:
> >  			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) {
> > +			break;
> > +		case XFS_STRIPE_RET_SWIDTH_OVERFLOW:
> >  			fprintf(stderr,
> > -_("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
> > -				big_dswidth, dsunit);
> > +_("data stripe width (dsw %d) is too large of a multiple of the data stripe unit (%d)\n"),
> 
> Why change this message?

big_dswidth isn't defined here.
So I'm not sure if the original message can still be properly used here.
(I could leave it alone...)

> 
> > +				dsw, dsunit);
> >  			usage();
> > +			break;
> >  		}
> > -		dswidth = big_dswidth;
> > +	} else {
> > +		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
> >  	}
> >  
> > -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> > -	    (dsunit && (dswidth % dsunit != 0))) {
> > +	if (error == XFS_STRIPE_RET_PARTIAL_VALID ||
> > +	    error == XFS_STRIPE_RET_SWIDTH_MISALIGN) {
> >  		fprintf(stderr,
> >  _("data stripe width (%d) must be a multiple of the data stripe unit (%d)\n"),
> >  			dswidth, dsunit);
> >  		usage();
> >  	}
> >  
> > +	if (error) {
> > +		fprintf(stderr,
> > +_("invalid data stripe unit (%d), width (%d)\n"), dsunit, dswidth);
> 
> Invalid how?  We know the exact reason, so we should say so.

Okay, let me think more about some cleaner way for these message.
I feel it could be a bit messy here.

Thanks，
Gao Xiang

> 
> --D
> 
> > +		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)) &&
> >  	    !dsunit && !dswidth)
> > @@ -2328,18 +2337,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) {
> > -- 
> > 2.18.1
> > 
> 

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

[Gao Xiang]

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

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

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

[Eric Sandeen]

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
> 

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

[Gao Xiang]

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

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

[Gao Xiang]

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 v2:
 - try to cover xfs_validate_sb_common() case as well suggested by Eric,
   so move to libxfs as an attempt for review;

 - use an static inline helper in xfs_sb.h so compilers can do their
   best to eliminate unneeded branches / rearrange code according to
   each individual caller;

 - get back to use "int error" expression since it's compatible with
   "enum xfs_stripeval" and it can be laterly reused for future uses
   in these callers instead of introducing another error code variables;

just for review/comments for now, if it looks almost fine, I'd like to
go further to stage up related functions to kernel.

 libfrog/topology.c | 15 +++++++++++
 libxfs/xfs_sb.c    | 32 ++++++++++++++++--------
 libxfs/xfs_sb.h    | 54 ++++++++++++++++++++++++++++++++++++++++
 mkfs/xfs_mkfs.c    | 62 ++++++++++++++++++++++++----------------------
 po/pl.po           |  4 +--
 5 files changed, 124 insertions(+), 43 deletions(-)

diff --git a/libfrog/topology.c b/libfrog/topology.c
index b1b470c9..554afbfc 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -187,6 +187,7 @@ static void blkid_get_topology(
 	blkid_probe pr;
 	unsigned long val;
 	struct stat statbuf;
+	int error;
 
 	/* can't get topology info from a file */
 	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
@@ -230,6 +231,20 @@ static void blkid_get_topology(
 	*sunit = *sunit >> 9;
 	*swidth = *swidth >> 9;
 
+	error = xfs_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),
+			xfs_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/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
index d37d60b3..0c0f5f90 100644
--- a/libxfs/xfs_sb.c
+++ b/libxfs/xfs_sb.c
@@ -210,6 +210,15 @@ xfs_validate_sb_write(
 	return 0;
 }
 
+const char *xfs_stripeval_str[] = {
+	"OK",
+	"SUNIT_MISALIGN",
+	"SWIDTH_OVERFLOW",
+	"PARTIAL_VALID",
+	"SUNIT_TOO_LARGE",
+	"SWIDTH_MISALIGN",
+};
+
 /* Check the validity of the SB. */
 STATIC int
 xfs_validate_sb_common(
@@ -220,6 +229,8 @@ xfs_validate_sb_common(
 	struct xfs_dsb		*dsb = bp->b_addr;
 	uint32_t		agcount = 0;
 	uint32_t		rem;
+	int			sunit, swidth;
+	int			error;
 
 	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
 		xfs_warn(mp, "bad magic number");
@@ -357,21 +368,20 @@ 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)) {
+	sunit = sbp->sb_unit;
+	swidth = sbp->sb_width;
+
+	if (!sunit ^ !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;
 	}
 
+	error = xfs_validate_stripe_factors(0, &sunit, &swidth);
+	if (error) {
+		xfs_notice(mp, "SB stripe sanity check failed %s",
+				xfs_stripeval_str[error]);
+		return -EFSCORRUPTED;
+	}
 
 	if (xfs_sb_version_hascrc(&mp->m_sb) &&
 	    sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
index 92465a9a..c4524bbc 100644
--- a/libxfs/xfs_sb.h
+++ b/libxfs/xfs_sb.h
@@ -41,5 +41,59 @@ extern int	xfs_sb_read_secondary(struct xfs_mount *mp,
 extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
 				struct xfs_trans *tp, xfs_agnumber_t agno,
 				struct xfs_buf **bpp);
+enum xfs_stripeval {
+	XFS_STRIPEVAL_OK = 0,
+	XFS_STRIPEVAL_SUNIT_MISALIGN,
+	XFS_STRIPEVAL_SWIDTH_OVERFLOW,
+	XFS_STRIPEVAL_PARTIAL_VALID,
+	XFS_STRIPEVAL_SUNIT_TOO_LARGE,
+	XFS_STRIPEVAL_SWIDTH_MISALIGN,
+};
+
+extern const char *xfs_stripeval_str[];
+
+/*
+ * This accepts either
+ *  - (sectersize != 0) dsu (in bytes) / dsw (which is multiplier of dsu)
+ * or
+ *  - (sectersize == 0) sunit / swidth (in 512B sectors)
+ * and return sunit/swidth in sectors.
+ */
+static inline enum xfs_stripeval
+xfs_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 XFS_STRIPEVAL_SUNIT_MISALIGN;
+
+		sunit = (int)BTOBBT(sunit);
+		big_swidth = (long long)sunit * swidth;
+
+		if (big_swidth > INT_MAX)
+			return XFS_STRIPEVAL_SWIDTH_OVERFLOW;
+		swidth = big_swidth;
+	}
+
+	if ((sunit && !swidth) || (!sunit && swidth))
+		return XFS_STRIPEVAL_PARTIAL_VALID;
+
+	if (sunit > swidth)
+		return XFS_STRIPEVAL_SUNIT_TOO_LARGE;
+
+	if (sunit && (swidth % sunit))
+		return XFS_STRIPEVAL_SWIDTH_MISALIGN;
+
+	*sunitp = sunit;
+	*swidthp = swidth;
+	return XFS_STRIPEVAL_OK;
+}
 
 #endif	/* __XFS_SB_H__ */
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280..8fdf17d7 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2255,7 +2255,6 @@ 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;
@@ -2263,6 +2262,7 @@ calc_stripe_factors(
 	int		dsw = 0;
 	int		lsu = 0;
 	bool		use_dev = false;
+	int		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 = xfs_validate_stripe_factors(cfg->sectorsize,
+				&dsunit, &dswidth);
+	} else {
+		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
 	}
 
-	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
-	    (dsunit && (dswidth % dsunit != 0))) {
+	switch (error) {
+	case XFS_STRIPEVAL_OK:
+		break;
+	case XFS_STRIPEVAL_SUNIT_MISALIGN:
+		fprintf(stderr,
+_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
+		usage();
+		break;
+	case XFS_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 XFS_STRIPEVAL_PARTIAL_VALID:
+	case XFS_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, xfs_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
-- 
2.18.1

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

[Eric Sandeen]

On 8/6/20 8:03 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 v2:
>  - try to cover xfs_validate_sb_common() case as well suggested by Eric,
>    so move to libxfs as an attempt for review;
> 
>  - use an static inline helper in xfs_sb.h so compilers can do their
>    best to eliminate unneeded branches / rearrange code according to
>    each individual caller;
> 
>  - get back to use "int error" expression since it's compatible with
>    "enum xfs_stripeval" and it can be laterly reused for future uses
>    in these callers instead of introducing another error code variables;
> 
> just for review/comments for now, if it looks almost fine, I'd like to
> go further to stage up related functions to kernel.
> 
>  libfrog/topology.c | 15 +++++++++++
>  libxfs/xfs_sb.c    | 32 ++++++++++++++++--------
>  libxfs/xfs_sb.h    | 54 ++++++++++++++++++++++++++++++++++++++++
>  mkfs/xfs_mkfs.c    | 62 ++++++++++++++++++++++++----------------------
>  po/pl.po           |  4 +--
>  5 files changed, 124 insertions(+), 43 deletions(-)

Sorry this sat w/o review for a while... I'm going to kind of suggest a new
approach here, since this seems to have gotten rather complicated.

> diff --git a/libfrog/topology.c b/libfrog/topology.c
> index b1b470c9..554afbfc 100644
> --- a/libfrog/topology.c
> +++ b/libfrog/topology.c
> @@ -187,6 +187,7 @@ static void blkid_get_topology(
>  	blkid_probe pr;
>  	unsigned long val;
>  	struct stat statbuf;
> +	int error;
>  
>  	/* can't get topology info from a file */
>  	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode)) {
> @@ -230,6 +231,20 @@ static void blkid_get_topology(
>  	*sunit = *sunit >> 9;
>  	*swidth = *swidth >> 9;
>  
> +	error = xfs_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),
> +			xfs_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/libxfs/xfs_sb.c b/libxfs/xfs_sb.c
> index d37d60b3..0c0f5f90 100644
> --- a/libxfs/xfs_sb.c
> +++ b/libxfs/xfs_sb.c
> @@ -210,6 +210,15 @@ xfs_validate_sb_write(
>  	return 0;
>  }
>  
> +const char *xfs_stripeval_str[] = {
> +	"OK",
> +	"SUNIT_MISALIGN",
> +	"SWIDTH_OVERFLOW",
> +	"PARTIAL_VALID",
> +	"SUNIT_TOO_LARGE",
> +	"SWIDTH_MISALIGN",
> +};
> +
>  /* Check the validity of the SB. */
>  STATIC int
>  xfs_validate_sb_common(
> @@ -220,6 +229,8 @@ xfs_validate_sb_common(
>  	struct xfs_dsb		*dsb = bp->b_addr;
>  	uint32_t		agcount = 0;
>  	uint32_t		rem;
> +	int			sunit, swidth;
> +	int			error;
>  
>  	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
>  		xfs_warn(mp, "bad magic number");
> @@ -357,21 +368,20 @@ 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)) {
> +	sunit = sbp->sb_unit;
> +	swidth = sbp->sb_width;
> +
> +	if (!sunit ^ !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;
>  	}
>  
> +	error = xfs_validate_stripe_factors(0, &sunit, &swidth);
> +	if (error) {
> +		xfs_notice(mp, "SB stripe sanity check failed %s",
> +				xfs_stripeval_str[error]);
> +		return -EFSCORRUPTED;
> +	}
>  
>  	if (xfs_sb_version_hascrc(&mp->m_sb) &&
>  	    sbp->sb_blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> diff --git a/libxfs/xfs_sb.h b/libxfs/xfs_sb.h
> index 92465a9a..c4524bbc 100644
> --- a/libxfs/xfs_sb.h
> +++ b/libxfs/xfs_sb.h
> @@ -41,5 +41,59 @@ extern int	xfs_sb_read_secondary(struct xfs_mount *mp,
>  extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
>  				struct xfs_trans *tp, xfs_agnumber_t agno,
>  				struct xfs_buf **bpp);
> +enum xfs_stripeval {
> +	XFS_STRIPEVAL_OK = 0,
> +	XFS_STRIPEVAL_SUNIT_MISALIGN,
> +	XFS_STRIPEVAL_SWIDTH_OVERFLOW,
> +	XFS_STRIPEVAL_PARTIAL_VALID,
> +	XFS_STRIPEVAL_SUNIT_TOO_LARGE,
> +	XFS_STRIPEVAL_SWIDTH_MISALIGN,
> +};
> +
> +extern const char *xfs_stripeval_str[];
> +
> +/*
> + * This accepts either
> + *  - (sectersize != 0) dsu (in bytes) / dsw (which is multiplier of dsu)
> + * or
> + *  - (sectersize == 0) sunit / swidth (in 512B sectors)
> + * and return sunit/swidth in sectors.
> + */

I'm still troubled by the complicated behavior of this function, even if it's
fully described in the comment.

What if this function:

* only accepts bytes, and does not convert sectors <-> bytes
  - i.e. callers should convert to bytes first

* directly prints the error using xfs_notice/warn() and an i8n'd _("...") string
  - this gets rid of enums & cases for strings, etc
  - kernelspace may need a #define to handle _("...")

* becomes a boolean and returns true (geom ok) or false (geom bad)
  - caller can print more context if needed, i.e. "Device returned bad geometry..."

* sectorsize check could be optional if we are calling from somewhere that
  does not need to or cannot validate against sector size (?)

> +static inline enum xfs_stripeval
> +xfs_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 XFS_STRIPEVAL_SUNIT_MISALIGN;
> +
> +		sunit = (int)BTOBBT(sunit);
> +		big_swidth = (long long)sunit * swidth;
> +
> +		if (big_swidth > INT_MAX)
> +			return XFS_STRIPEVAL_SWIDTH_OVERFLOW;

I think this check can stay in mkfs.xfs, since it is the only place
that accepts an "sunit multiplier" - i.e. this could be more of a option
parse-time check than a strict geometry check.

> +		swidth = big_swidth;
> +	}
> +
> +	if ((sunit && !swidth) || (!sunit && swidth))
> +		return XFS_STRIPEVAL_PARTIAL_VALID;
> +
> +	if (sunit > swidth)
> +		return XFS_STRIPEVAL_SUNIT_TOO_LARGE;
> +
> +	if (sunit && (swidth % sunit))
> +		return XFS_STRIPEVAL_SWIDTH_MISALIGN;
> +
> +	*sunitp = sunit;
> +	*swidthp = swidth;
> +	return XFS_STRIPEVAL_OK;
> +}
>  
>  #endif	/* __XFS_SB_H__ */
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e6cd280..8fdf17d7 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2255,7 +2255,6 @@ 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;
> @@ -2263,6 +2262,7 @@ calc_stripe_factors(
>  	int		dsw = 0;
>  	int		lsu = 0;
>  	bool		use_dev = false;
> +	int		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();
> -		}
This would still move to the core validator. If some callers do not have sectorsize,
the check could be conditional.

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

I think this can stay here in mkfs, as a mkfs option validator rather than an actual
geometry validator, since it is the only placle that multiples the two.

So if we leave the multiplier checking in place above, then:

> -		dswidth = big_dswidth;
> +		dsunit = dsu;
> +		dswidth = dsw;
> +		error = xfs_validate_stripe_factors(cfg->sectorsize,
> +				&dsunit, &dswidth);

this call can go away, and we can just call xfs_validate_stripe_factors once,
with sunit and swidth in bytes and no conversion?

> +	} else {
> +		error = xfs_validate_stripe_factors(0, &dsunit, &dswidth);
>  	}


> -	if ((dsunit && !dswidth) || (!dsunit && dswidth) ||
> -	    (dsunit && (dswidth % dsunit != 0))) {
> +	switch (error) {
> +	case XFS_STRIPEVAL_OK:
> +		break;
> +	case XFS_STRIPEVAL_SUNIT_MISALIGN:
> +		fprintf(stderr,
> +_("data su must be a multiple of the sector size (%d)\n"), cfg->sectorsize);
> +		usage();
> +		break;
> +	case XFS_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 XFS_STRIPEVAL_PARTIAL_VALID:
> +	case XFS_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, xfs_stripeval_str[error]);
> +		usage();

Then this case statement all goes away, and we can just do

	if (error)
		usage();

because xfs_validate_stripe_factors already printed the inoformation strings.

Does that make sense?  Does it seem like it would work?

Thanks,
-Eric

>  	}
>  
>  	/* 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
> 

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

[Gao Xiang]

Hi Eric,

On Mon, Sep 28, 2020 at 06:18:37PM -0500, Eric Sandeen wrote:
> On 8/6/20 8:03 AM, Gao Xiang wrote:

...

> >  mkfs/xfs_mkfs.c    | 62 ++++++++++++++++++++++++----------------------
> >  po/pl.po           |  4 +--
> >  5 files changed, 124 insertions(+), 43 deletions(-)
> 
> Sorry this sat w/o review for a while... I'm going to kind of suggest a new
> approach here, since this seems to have gotten rather complicated.

Yeah, that is fine.

> 
> > +/*
> > + * This accepts either
> > + *  - (sectersize != 0) dsu (in bytes) / dsw (which is multiplier of dsu)
> > + * or
> > + *  - (sectersize == 0) sunit / swidth (in 512B sectors)
> > + * and return sunit/swidth in sectors.
> > + */
> 
> I'm still troubled by the complicated behavior of this function, even if it's
> fully described in the comment.
> 
> What if this function:
> 
> * only accepts bytes, and does not convert sectors <-> bytes
>   - i.e. callers should convert to bytes first
> 
> * directly prints the error using xfs_notice/warn() and an i8n'd _("...") string
>   - this gets rid of enums & cases for strings, etc
>   - kernelspace may need a #define to handle _("...")
> 
> * becomes a boolean and returns true (geom ok) or false (geom bad)
>   - caller can print more context if needed, i.e. "Device returned bad geometry..."
> 
> * sectorsize check could be optional if we are calling from somewhere that
>   does not need to or cannot validate against sector size (?)
> 

I'm fine with the above proposal. I'll figout out a new patch
for this later (I'm outside now, will seek time).

The reason why it used enum here was to follow Darrick's
original suggestion in
https://lore.kernel.org/linux-xfs/20200515211011.GP6714@magnolia/

For such wrappers, Dave suggested updating xfs_notice() / xfs_warn()
in libxfs/libxfs_priv.h on IRC. So I will go on in that way.

If all people agree on this approach, I'm fine with all of that.

Thanks,
Gao Xiang