RE: [PATCH 02/32] Introduce flexible array struct memcpy() helpers

RE: [PATCH 02/32] Introduce flexible array struct memcpy() helpers

[David Laight]

From: Johannes Berg
> Sent: 05 May 2022 21:13
> On Thu, 2022-05-05 at 13:08 -0700, Keith Packard wrote:
> 
> 
> > I bet you've already considered the simpler form:
> >
> >         struct something *instance = mem_to_flex_dup(byte_array, count, GFP_KERNEL);
> >         if (IS_ERR(instance))
> >             return PTR_ERR(instance);
> >
> 
> Sadly, this doesn't work in any way because mem_to_flex_dup() needs to
> know at least the type, hence passing 'instance', which is simpler than
> passing 'struct something'.

You can use:
         struct something *instance;
         mem_to_flex_dup(instance, byte_array, count, GFP_KERNEL);
         if (IS_ERR(instance))
             return PTR_ERR(instance);
and have mem_to_flex_dup() (which must be a #define) update 'instance'.
(You can require &instance - and just precede all the uses with
an extra '*' to make it more obvious the variable is updated.
But there is little point requiring it be NULL.)

If you really want to define the variable mid-block you can use:
         mem_to_flex_dup(struct something *, instance, byte_array, count, GFP_KERNEL);

but I really hate having declarations anywhere other than the top of
a function because it makes them hard for the 'mk1 eyeball' to spot.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 19/32] afs: Use mem_to_flex_dup() with struct afs_acl

[David Howells]

Kees Cook <keescook@chromium.org> wrote:

>  struct afs_acl {
> -	u32	size;
> -	u8	data[];
> +	DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size);
> +	DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
>  };

Oof...  That's really quite unpleasant syntax.  Is it not possible to have
mem_to_flex_dup() and friends work without that?  You are telling them the
fields they have to fill in.

> +	struct afs_acl *acl = NULL;
>  
> -	acl = kmalloc(sizeof(*acl) + size, GFP_KERNEL);
> -	if (!acl) {
> +	if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL)) {

Please don't do that.  Either do:

	acl = mem_to_flex_dup(buffer, size, GFP_KERNEL);
	if (!acl)

or:

	acl = mem_to_flex_dup(buffer, size, GFP_KERNEL);
	if (IS_ERR(acl))

Please especially don't make it that an apparent 'true' return indicates an
error.  If you absolutely must return the acl pointer through the argument
list (presumably because it's actually a macro), make it return false on
failure:

	if (!mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL))

or return and explicitly check for an error code:

	if (mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL) < 0)

or:

	ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL);
	if (ret < 0)

(or use != 0 rather than < 0)

David

Re: [PATCH 19/32] afs: Use mem_to_flex_dup() with struct afs_acl

[Kees Cook]

On Thu, May 12, 2022 at 10:41:05PM +0100, David Howells wrote:
> 
> Kees Cook <keescook@chromium.org> wrote:
> 
> >  struct afs_acl {
> > -	u32	size;
> > -	u8	data[];
> > +	DECLARE_FLEX_ARRAY_ELEMENTS_COUNT(u32, size);
> > +	DECLARE_FLEX_ARRAY_ELEMENTS(u8, data);
> >  };
> 
> Oof...  That's really quite unpleasant syntax.  Is it not possible to have
> mem_to_flex_dup() and friends work without that?  You are telling them the
> fields they have to fill in.

Other threads discussed this too. I'm hoping to have something more
flexible (pardon the pun) in v2.

> [...]
> or:
> 
> 	ret = mem_to_flex_dup(&acl, buffer, size, GFP_KERNEL);
> 	if (ret < 0)
> 
> (or use != 0 rather than < 0)

Sure, I can make the tests more explicit. The kerndoc, etc all shows it's
using < 0 for errors.

-- 
Kees Cook

Re: [PATCH 00/32] Introduce flexible array struct memcpy() helpers

[David Howells]

Kees Cook <keescook@chromium.org> wrote:

> I'm happy to also point out that the conversions (patches 5+) are actually
> a net reduction in lines of code:
>  49 files changed, 154 insertions(+), 244 deletions(-)

That doesn't mean that it's actually code that's clearer to read.  I would say
that it's actually less clear.  In a bunch of places, you've done something
like:

-	e = kmalloc(...);
-	if (!e)
+	if (__mem_to_flex_dup(&e, ...))

The problem is that, to me at least, it looks like:

-	e = kmalloc(...);
-	if (kmalloc failed)
+	if (__mem_to_flex_dup(&e, ...) succeeded)

David