Re: kalloc_objs() may not be as safe as it seems

[Alejandro Colomar <alx@kernel.org>]

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

On 2026-03-16T19:33:37+0100, Alejandro Colomar wrote:
> 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))};            \

I meant the following:

		(typeas(T) *){reallocarray(NULL, n, sizeof(T))};

I had the issue while pasting and adapting for simplicity.  The original
code is correct, of course.


> 	})
> 
> 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>



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

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