| Message ID | 20240807235433.work.317-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | slab: Introduce kmalloc_obj() and family | expand |
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)) >
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
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))
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_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(+)