Re: [PATCH] slab: Introduce kmalloc_obj() and family

Re: [PATCH] slab: Introduce kmalloc_obj() and family

[Vlastimil Babka]

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@kernel.org>
> ---
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Roman Gushchin <roman.gushchin@linux.dev>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> Cc: Bill Wendling <morbo@google.com>
> Cc: Justin Stitt <justinstitt@google.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Cc: Marco Elver <elver@google.com>
> Cc: linux-mm@kvack.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))
>  

Re: [PATCH] slab: Introduce kmalloc_obj() and family

[Kees Cook]

On Fri, Aug 09, 2024 at 10:59:52AM +0200, Vlastimil Babka wrote:
> 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?).

Ah, I made this changed based on requests in earlier threads. But yes,
the ambiguity with ZERO_SIZE_PTR does make me uncomfortable too. I think
I can make the size an optional argument when splitting the functions as
you request below...

> 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?

I liked it because it's always doing the same thing: allocating a
structure. But yes, I do see that it's a bit weird. Since "kcalloc" means
"zero it also", how about the naming just uses pluralization instead?

	kmalloc_obj(p, gfp);
	kmalloc_objs(p, count, gfp);
	kmalloc_flex(p, flex_member, count, gfp);

Does that looks okay?

> > 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?

If we wanted size output, we could add an optional final argument:

	kmalloc_obj(p, gfp, &size);
	kmalloc_objs(p, count, gfp, &size);
	kmalloc_flex(p, flex_member, count, gfp, &size);

And as far as solving the concern of "hiding the assignment", what about
trying to "show" that "p" is being assigned by requiring that it also
use "&"? For example:

	kmalloc_obj(&p, gfp);
	kmalloc_objs(&p, count, gfp);
	kmalloc_flex(&p, flex_member, count, gfp);

> 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 ...

Whoops, yes. I totally missed adding those. I will add that once I have
some direction on the above design ideas.

Thanks for looking at this!

-Kees

-- 
Kees Cook

[PATCH] slab: Introduce kmalloc_obj() and family

[Kees Cook]

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

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

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_set_counted_by() (or equivalent) is added
by GCC and Clang, it will be possible to automatically set the counted
member of a struct with a counted_by FAM, further eliminating open-coded
redundant initializations:

	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 via Coccinelle[1] shows what
could be replaced immediately (saving roughly 1,500 lines):

 7040 files changed, 14128 insertions(+), 15557 deletions(-)

Link: https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/kmalloc_obj-assign-size.cocci [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Christoph Lameter <cl@linux.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Cc: Bill Wendling <morbo@google.com>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Jann Horn <jannh@google.com>
Cc: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: Marco Elver <elver@google.com>
Cc: linux-mm@kvack.org
---
 include/linux/slab.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 7247e217e21b..3817554f2d51 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -665,6 +665,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__)
+
 static __always_inline __alloc_size(1) void *kmalloc_node_noprof(size_t size, gfp_t flags, int node)
 {
 	if (__builtin_constant_p(size) && size) {
-- 
2.34.1

Re: [PATCH] slab: Introduce kmalloc_obj() and family

[David Rientjes]

On Fri, 19 Jul 2024, Kees Cook wrote:

> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 7247e217e21b..3817554f2d51 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -665,6 +665,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__)
> +
>  static __always_inline __alloc_size(1) void *kmalloc_node_noprof(size_t size, gfp_t flags, int node)
>  {
>  	if (__builtin_constant_p(size) && size) {

I'm supportive of this especially because it will pave a pathway toward 
future hardening work.  Request: could we get an addition to 
Documentation/ that explains how common idioms today can be converted to 
these new macros for future users?  The above makes sense only when 
accompanied by your commit description :)

Re: [PATCH] slab: Introduce kmalloc_obj() and family

[Kees Cook]

On Fri, Jul 19, 2024 at 08:50:41PM -0700, David Rientjes wrote:
> On Fri, 19 Jul 2024, Kees Cook wrote:
> 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index 7247e217e21b..3817554f2d51 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -665,6 +665,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__)
> > +
> >  static __always_inline __alloc_size(1) void *kmalloc_node_noprof(size_t size, gfp_t flags, int node)
> >  {
> >  	if (__builtin_constant_p(size) && size) {
> 
> I'm supportive of this especially because it will pave a pathway toward 
> future hardening work.  Request: could we get an addition to 

Thanks!

> Documentation/ that explains how common idioms today can be converted to 
> these new macros for future users?  The above makes sense only when 
> accompanied by your commit description :)

Oh, yes. Very good point! I will figure out a place to add this. I'm not
sure if kerndoc would be best here.

-- 
Kees Cook