[PATCH] Grow: fix resize of array component size to > 32bits

[PATCH] Grow: fix resize of array component size to > 32bits

[Justin Maggard]

If the request --size to --grow an array to is larger
than 32bits, then mdadm may make the wrong choice and
use ioctl instead of setting component_size via sysfs
and the change is ignored.

Instead of using casts to check for a 32-bit overflow,
just check for set bits outside of INT32_MAX.
---
 Grow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Grow.c b/Grow.c
index a9c8589..a614102 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1818,7 +1818,7 @@ int Grow_reshape(char *devname, int fd,
 		if (s->size == MAX_SIZE)
 			s->size = 0;
 		array.size = s->size;
-		if (array.size != (signed)s->size) {
+		if (s->size & ~INT32_MAX) {
 			/* got truncated to 32bit, write to
 			 * component_size instead
 			 */
-- 
1.9.1

Re: [PATCH] Grow: fix resize of array component size to > 32bits

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

On Fri, 24 Oct 2014 17:55:02 -0700 Justin Maggard <jmaggard10@gmail.com>
wrote:

> If the request --size to --grow an array to is larger
> than 32bits, then mdadm may make the wrong choice and
> use ioctl instead of setting component_size via sysfs
> and the change is ignored.

Can you explain exactly why the  current code is not sufficient?  When does
it fail?
If you include the explanation in a re-submission of the patch, and I am
convinced, then I will gladly apply your patch.

Thanks,
NeilBrown

> 
> Instead of using casts to check for a 32-bit overflow,
> just check for set bits outside of INT32_MAX.
> ---
>  Grow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Grow.c b/Grow.c
> index a9c8589..a614102 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1818,7 +1818,7 @@ int Grow_reshape(char *devname, int fd,
>  		if (s->size == MAX_SIZE)
>  			s->size = 0;
>  		array.size = s->size;
> -		if (array.size != (signed)s->size) {
> +		if (s->size & ~INT32_MAX) {
>  			/* got truncated to 32bit, write to
>  			 * component_size instead
>  			 */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Grow: fix resize of array component size to > 32bits

[Justin Maggard]

On Tue, Oct 28, 2014 at 3:19 PM, NeilBrown <neilb@suse.de> wrote:
> On Fri, 24 Oct 2014 17:55:02 -0700 Justin Maggard <jmaggard10@gmail.com>
> wrote:
>
>> If the request --size to --grow an array to is larger
>> than 32bits, then mdadm may make the wrong choice and
>> use ioctl instead of setting component_size via sysfs
>> and the change is ignored.
>
> Can you explain exactly why the  current code is not sufficient?  When does
> it fail?
> If you include the explanation in a re-submission of the patch, and I am
> convinced, then I will gladly apply your patch.
>

To be honest, I can't figure out how the current comparison would have
ever worked at all. To illustrate, here's a simple test program:
-----
$ ./test 0xfffffffff
cast comparison did not detect truncation
bit comparison detected truncation

$ cat test.c
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

int main(int argc, char **argv)
{
unsigned long long ssize = strtoull(argv[1], NULL, 0);
int asize;

asize = ssize;
if (asize != (signed)ssize)
printf("cast comparison detected truncation\n");
else
printf("cast comparison did not detect truncation \n");
if (ssize & ~INT32_MAX)
printf("bit comparison detected truncation\n");
else
printf("bit comparison did not detect truncation \n");

return 0;
}
-----

I plugged lots of numbers in there, and I was never able to get the
current cast comparison to see a difference.

I ran into the issue by trying to grow the component size of a RAID
array from 1TB to 3TB, and it wouldn't work if I specified the size;
only using "max" worked.

I'm happy to re-submit if you'd like; I just thought it was a pretty
straightforward bug.  I guess what I'm saying is, I don't understand
why there *should* be a difference between assigning a unsigned long
long to an int variable, and casting that unsigned long long to a
signed type.  But

-Justin

> Thanks,
> NeilBrown
>
>>
>> Instead of using casts to check for a 32-bit overflow,
>> just check for set bits outside of INT32_MAX.
>> ---
>>  Grow.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Grow.c b/Grow.c
>> index a9c8589..a614102 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1818,7 +1818,7 @@ int Grow_reshape(char *devname, int fd,
>>               if (s->size == MAX_SIZE)
>>                       s->size = 0;
>>               array.size = s->size;
>> -             if (array.size != (signed)s->size) {
>> +             if (s->size & ~INT32_MAX) {
>>                       /* got truncated to 32bit, write to
>>                        * component_size instead
>>                        */
>

Re: [PATCH] Grow: fix resize of array component size to > 32bits

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3219 bytes --]

On Tue, 28 Oct 2014 16:47:06 -0700 Justin Maggard <jmaggard10@gmail.com>
wrote:

> On Tue, Oct 28, 2014 at 3:19 PM, NeilBrown <neilb@suse.de> wrote:
> > On Fri, 24 Oct 2014 17:55:02 -0700 Justin Maggard <jmaggard10@gmail.com>
> > wrote:
> >
> >> If the request --size to --grow an array to is larger
> >> than 32bits, then mdadm may make the wrong choice and
> >> use ioctl instead of setting component_size via sysfs
> >> and the change is ignored.
> >
> > Can you explain exactly why the  current code is not sufficient?  When does
> > it fail?
> > If you include the explanation in a re-submission of the patch, and I am
> > convinced, then I will gladly apply your patch.
> >
> 
> To be honest, I can't figure out how the current comparison would have
> ever worked at all. To illustrate, here's a simple test program:
> -----
> $ ./test 0xfffffffff
> cast comparison did not detect truncation
> bit comparison detected truncation
> 
> $ cat test.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <stdint.h>
> 
> int main(int argc, char **argv)
> {
> unsigned long long ssize = strtoull(argv[1], NULL, 0);
> int asize;
> 
> asize = ssize;
> if (asize != (signed)ssize)
> printf("cast comparison detected truncation\n");
> else
> printf("cast comparison did not detect truncation \n");
> if (ssize & ~INT32_MAX)
> printf("bit comparison detected truncation\n");
> else
> printf("bit comparison did not detect truncation \n");
> 
> return 0;
> }
> -----
> 
> I plugged lots of numbers in there, and I was never able to get the
> current cast comparison to see a difference.
> 
> I ran into the issue by trying to grow the component size of a RAID
> array from 1TB to 3TB, and it wouldn't work if I specified the size;
> only using "max" worked.
> 
> I'm happy to re-submit if you'd like; I just thought it was a pretty
> straightforward bug.  I guess what I'm saying is, I don't understand
> why there *should* be a difference between assigning a unsigned long
> long to an int variable, and casting that unsigned long long to a
> signed type.  But
> 
> -Justin
> 
> > Thanks,
> > NeilBrown
> >
> >>
> >> Instead of using casts to check for a 32-bit overflow,
> >> just check for set bits outside of INT32_MAX.
> >> ---
> >>  Grow.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/Grow.c b/Grow.c
> >> index a9c8589..a614102 100644
> >> --- a/Grow.c
> >> +++ b/Grow.c
> >> @@ -1818,7 +1818,7 @@ int Grow_reshape(char *devname, int fd,
> >>               if (s->size == MAX_SIZE)
> >>                       s->size = 0;
> >>               array.size = s->size;
> >> -             if (array.size != (signed)s->size) {
> >> +             if (s->size & ~INT32_MAX) {
> >>                       /* got truncated to 32bit, write to
> >>                        * component_size instead
> >>                        */
> >


Thanks.
Looks like I broke it in July.

http://git.neil.brown.name/?p=mdadm.git;a=commitdiff;h=4e9a3dd16d656b269f5602624ac4f7109a571368

I probably should have made it
    if (s->size != (signed long long)s->size)

I've applied your patch.

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 828 bytes --]