Re: [PATCH V2] xfs: allocate xattr buffer on demand

[Dave Chinner <david@fromorbit.com>]

On Sat, Aug 10, 2019 at 11:45:35PM +0200, Holger Hoffstätte wrote:
> 
> Hi Dave -
> 
> great patch but I found something that seems off in xfs_attr3_leaf_getvalue:

It's not a "great patch" if there's something wrong with it. :/

> > @@ -2378,31 +2403,23 @@ xfs_attr3_leaf_getvalue((..snip..)
> > +	if (args->flags & ATTR_KERNOVAL) {
> >   		args->valuelen = args->rmtvaluelen;
> > +		return 0;
> >   	}
> > -	return 0;
> > +	return xfs_attr_copy_value(args, NULL, args->rmtvaluelen);
> 
> With gcc9 I get:
> 
>   CC      fs/xfs/libxfs/xfs_attr_leaf.o
> In function 'xfs_attr_copy_value',
>     inlined from 'xfs_attr3_leaf_getvalue' at fs/xfs/libxfs/xfs_attr_leaf.c:2425:9:
> fs/xfs/libxfs/xfs_attr_leaf.c:421:2: warning: argument 2 null where non-null expected [-Wnonnull]
>   421 |  memcpy(args->value, value, valuelen);
>       |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from ./arch/x86/include/asm/string.h:5,
>                  from ./include/linux/string.h:20,
>                  from ./include/linux/uuid.h:12,
>                  from ./fs/xfs/xfs_linux.h:10,
>                  from ./fs/xfs/xfs.h:22,
>                  from fs/xfs/libxfs/xfs_attr_leaf.c:7:
> fs/xfs/libxfs/xfs_attr_leaf.c: In function 'xfs_attr3_leaf_getvalue':
> ./arch/x86/include/asm/string_64.h:14:14: note: in a call to function 'memcpy' declared here
>    14 | extern void *memcpy(void *to, const void *from, size_t len);
>       |              ^~~~~~
> 
> and sure enough, the NULL "value" arg is and passed as-is to memcpy in
> xfs_attr_copy_value.

"sure enough", eh?

> Maybe you meant to sanitize the value when it's NULL?

Nope - look at the code:

        args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen);
>>>>    args->rmtblkno = be32_to_cpu(name_rmt->valueblk);
        args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount,
                                               args->rmtvaluelen);
        if (args->flags & ATTR_KERNOVAL) {
                args->valuelen = args->rmtvaluelen;
                return 0;
        }
        return xfs_attr_copy_value(args, NULL, args->rmtvaluelen);
}

And the relevant code in xfs_attr_copy_value() does:

        /* remote block xattr requires IO for copy-in */
>>>>    if (args->rmtblkno)
>>>>            return xfs_attr_rmtval_get(args);

        memcpy(args->value, value, valuelen);
        return 0;
}

The memcpy() is never reached in this case. Hence the compiler
warning is a false positive and the code is not going to crash here.

Regardless, I'm going to have to change the code because I doubt gcc
will ever be smart enough to understand the code flow as it stands.
We have to do this every so often to avoid false positive
uninitialised variable warnings, so it's not like working around
compiler issues is something new.

I'll post an updated version tomorrow....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com