kalloc_objs() may not be as safe as it seems

[Alejandro Colomar <alx@kernel.org>]

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

Hi Kees,

I just learnt about kalloc_objs() et al. from
<https://lwn.net/Articles/1062856/>.

    ptr = kmalloc_obj(*ptr);
    ptr = kmalloc_objs(*ptr, n);

This resembles a lot the macro we have in shadow-utils, malloc_T(),
which would be used as (to resemble the above):

    ptr = malloc_T(1, typeof(*ptr));  // But we'd really pass the type
    ptr = malloc_T(n, typeof(*ptr));  // But we'd really pass the type

But I've noticed some design mistakes that make it not as safe as it
seems.

Default arguments
~~~~~~~~~~~~~~~~~

I tend to think it's simpler to have a single API that works for both
1 element and multiple elements.  The special-casing of 1 element seems
unnecessary, and having a literal 1 works just fine.

I think the combination of having the macros be variadic (for gfp) with
having two very similar APIs that differ in number of arguments, and
all those arguments being integer types, is prone to errors.  Consider
the case where one would accidentally write

	ptr = kmalloc_obj(*ptr, n);  // Bogus
instead of
	ptr = kmalloc_objs(*ptr, n);

The compiler wouldn't realize at all.  That's a strong argument in
favour of having default arguments be required to be explicit, with an
empty argument:

	ptr = kmalloc_obj(*ptr,);
	ptr = kmalloc_objs(*ptr, n,);

I know you (and Linus too, FWIW) have previously claimed that it looks
weird to the eye.  But I'm pretty sure you could get used to it.  That's
certainly going to be safer.

With mandatory empty arguments, the compiler would easily distinguish
mistakes like the one above.

Type safety
~~~~~~~~~~~

Apart from the issues with the above, the ability to pass a variable
instead of a type name is also a bad choice.  In shadow-utils, we
require a type name, and a variable is rejected.  We implement that with
the typeas() macro:

	#define typeas(T)  typeof((T){0})

This macro works exactly like typeof(), but it requires that the input
is also a type.  Passing a variable is a syntax error.  We implement
malloc_T() with it:

	// malloc_T - malloc type-safe
	#define malloc_T_(n, T)                                       \
	({                                                            \
		(typeas(T) *){reallocarray(n, sizeof(T))};            \
	})

which is used as (taking some arbitrary examples from shadow-utils):

	lp = xmalloc_T(1, struct link_name);
	targs = xmalloc_T(n_args + 3, char *);

Some reasons for passing a type name instead of a variable are:

-  It allows grepping for all allocations of a given type.
-  It adds readability.  It's similar to declaring variables with some
   explicit type, vs. using 'auto' (__auto_type) everywhere.

But there's also a safety aspect.  Consider we want to allocate an array
of 42 ints.  And consider the programmer accidentally swaps arguments.

	int *p = malloc_T(int, 42);  // syntax error
	int *p = malloc_T(42, int);
vs
	int *p = kmalloc_objs(*p, 42);
	int *p = kmalloc_objs(42, *p);  // Bogus

The latter is dereferencing an uninitialized pointer.  If for some
reason the pointer had a value before this call, you'd be allocating as
many elements as *p says, which would be bogus, and since typeof(42) is
the same as typeof(*p), the return type would be valid, so this would
still compile.


Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]