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

[NeilBrown <neilb@suse.de>]

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