Message-ID: <ddf093db-b0a8-4e44-9d81-1e4840967557@suse.cz>
Date: Fri, 9 Aug 2024 10:59:52 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Kees Cook <kees@...nel.org>
Cc: Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
 David Rientjes <rientjes@...gle.com>, Joonsoo Kim <iamjoonsoo.kim@....com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>,
 "Gustavo A . R . Silva" <gustavoars@...nel.org>,
 Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>,
 Jann Horn <jannh@...gle.com>, Przemek Kitszel
 <przemyslaw.kitszel@...el.com>, Marco Elver <elver@...gle.com>,
 linux-mm@...ck.org, Nathan Chancellor <nathan@...nel.org>,
 Nick Desaulniers <ndesaulniers@...gle.com>, linux-kernel@...r.kernel.org,
 llvm@...ts.linux.dev, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] slab: Introduce kmalloc_obj() and family

On 8/8/24 01:54, Kees Cook wrote:
> Introduce type-aware kmalloc-family helpers to replace the common
> idioms for single, array, and flexible object allocations:
> 
> 	ptr = kmalloc(sizeof(*ptr), gfp);
> 	ptr = kcalloc(count, sizeof(*ptr), gfp);
> 	ptr = kmalloc_array(count, sizeof(*ptr), gfp);
> 	ptr = kcalloc(count, sizeof(*ptr), gfp);
> 	ptr = kmalloc(struct_size(ptr, flex_member, count), gfp);
> 
> These become, respectively:
> 
> 	kmalloc_obj(p, gfp);
> 	kzalloc_obj(p, count, gfp);
> 	kmalloc_obj(p, count, gfp);
> 	kzalloc_obj(p, count, gfp);
> 	kmalloc_obj(p, flex_member, count, gfp);

So I'm not a huge fan in hiding the assignment, but I understand there's
value in having guaranteed the target of the assignment is really the same
thing as the one used for sizeof() etc.

But returning size seems awkward, it would be IMHO less confusing if it
still returned the object pointer, that could be then also assigned
elsewhere if needed, tested for NULL and ZERO_SIZE_PTR (now it's both 0?).

I'm also not sure that having it all called kmalloc_obj() with 3 variants of
how many parameters it takes is such a win? e.g. kmalloc_obj(),
kcalloc_obj() and kcalloc_obj_flex() would be more obvious?

> These each return the size of the allocation, so that other common
> idioms can be converted easily as well. For example:
> 
> 	info->size = struct_size(ptr, flex_member, count);
> 	ptr = kmalloc(info->size, gfp);
> 
> becomes:
> 
> 	info->size = kmalloc_obj(ptr, flex_member, count, gfp);

How about instead taking an &info->size parameter that assigns size to it,
so the ptr can be still returned but we also can record the size?

Also the last time David asked for documentation, you say you would try, but
there's nothing new here? Dunno if the kerneldocs are feasible but there's
at least Documentation/core-api/memory-allocation.rst ...

Thanks,
Vlastimil

> Internal introspection of allocated type also becomes possible, allowing
> for alignment-aware choices and future hardening work. For example,
> adding __alignof(*ptr) as an argument to the internal allocators so that
> appropriate/efficient alignment choices can be made, or being able to
> correctly choose per-allocation offset randomization within a bucket
> that does not break alignment requirements.
> 
> Additionally, once __builtin_get_counted_by() is added by GCC[1] and
> Clang[2], it will be possible to automatically set the counted member of
> a struct with a counted_by FAM, further eliminating open-coded redundant
> initializations, and can internally check for "too large" allocations
> based on the type size of the counter variable:
> 
> 	if (count > type_max(ptr->flex_count))
> 		fail...;
> 	info->size = struct_size(ptr, flex_member, count);
> 	ptr = kmalloc(info->size, gfp);
> 	ptr->flex_count = count;
> 
> becomes (i.e. unchanged from earlier example):
> 
> 	info->size = kmalloc_obj(ptr, flex_member, count, gfp);
> 
> Replacing all existing simple code patterns found via Coccinelle[3]
> shows what could be replaced immediately (saving roughly 1,500 lines):
> 
>  7040 files changed, 14128 insertions(+), 15557 deletions(-)
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116016 [1]
> Link: https://github.com/llvm/llvm-project/issues/99774 [2]
> Link: https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/kmalloc_obj-assign-size.cocci [3]
> Signed-off-by: Kees Cook <kees@...nel.org>
> ---
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: Christoph Lameter <cl@...ux.com>
> Cc: Pekka Enberg <penberg@...nel.org>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Roman Gushchin <roman.gushchin@...ux.dev>
> Cc: Hyeonggon Yoo <42.hyeyoo@...il.com>
> Cc: Gustavo A. R. Silva <gustavoars@...nel.org>
> Cc: Bill Wendling <morbo@...gle.com>
> Cc: Justin Stitt <justinstitt@...gle.com>
> Cc: Jann Horn <jannh@...gle.com>
> Cc: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> Cc: Marco Elver <elver@...gle.com>
> Cc: linux-mm@...ck.org
> ---
>  include/linux/slab.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index eb2bf4629157..46801c28908e 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -686,6 +686,44 @@ static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f
>  }
>  #define kmalloc(...)				alloc_hooks(kmalloc_noprof(__VA_ARGS__))
>  
> +#define __alloc_obj3(ALLOC, P, COUNT, FLAGS)			\
> +({								\
> +	size_t __obj_size = size_mul(sizeof(*P), COUNT);	\
> +	void *__obj_ptr;					\
> +	(P) = __obj_ptr = ALLOC(__obj_size, FLAGS);		\
> +	if (!__obj_ptr)						\
> +		__obj_size = 0;					\
> +	__obj_size;						\
> +})
> +
> +#define __alloc_obj2(ALLOC, P, FLAGS)	__alloc_obj3(ALLOC, P, 1, FLAGS)
> +
> +#define __alloc_obj4(ALLOC, P, FAM, COUNT, FLAGS)		\
> +({								\
> +	size_t __obj_size = struct_size(P, FAM, COUNT);		\
> +	void *__obj_ptr;					\
> +	(P) = __obj_ptr = ALLOC(__obj_size, FLAGS);		\
> +	if (!__obj_ptr)						\
> +		__obj_size = 0;					\
> +	__obj_size;						\
> +})
> +
> +#define kmalloc_obj(...)					\
> +	CONCATENATE(__alloc_obj,				\
> +		    COUNT_ARGS(__VA_ARGS__))(kmalloc, __VA_ARGS__)
> +
> +#define kzalloc_obj(...)					\
> +	CONCATENATE(__alloc_obj,				\
> +		    COUNT_ARGS(__VA_ARGS__))(kzalloc, __VA_ARGS__)
> +
> +#define kvmalloc_obj(...)					\
> +	CONCATENATE(__alloc_obj,				\
> +		    COUNT_ARGS(__VA_ARGS__))(kvmalloc, __VA_ARGS__)
> +
> +#define kvzalloc_obj(...)					\
> +	CONCATENATE(__alloc_obj,				\
> +		    COUNT_ARGS(__VA_ARGS__))(kvzalloc, __VA_ARGS__)
> +
>  #define kmem_buckets_alloc(_b, _size, _flags)	\
>  	alloc_hooks(__kmalloc_node_noprof(PASS_BUCKET_PARAMS(_size, _b), _flags, NUMA_NO_NODE))
>  

Powered by blists - more mailing lists