On Tue, Aug 27, 2024 at 11:32:14PM +0200, Vlastimil Babka wrote: > +Cc Linus > > On 8/23/24 01:13, 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 = kzalloc(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(ptr, gfp); > > kzalloc_obj(ptr, gfp); > > kmalloc_objs(ptr, count, gfp); > > kzalloc_objs(ptr, count, gfp); > > kmalloc_flex(ptr, flex_member, count, gfp); > > This is indeed better than the previous version. The hidden assignment to > ptr seems still very counter-intuitive, but if it's the only way to do those > validations, the question is then just whether it's worth the getting used > to it, or not. We could make the syntax require "&ptr"? As for alternatives, one thing I investigated for a while that made several compiler people unhappy was to introduce an builtin named something like __builtin_lvalue() which could be used on the RHS of an assignment to discover the lvalue in some way. Then we could, for example, add alignment discovery like so: #define kmalloc(_size, _gfp) \ __kmalloc(_size, __alignof(typeof(__builtin_lvalue())), _gfp) or do the FAM struct allocations: #define kmalloc_flex(_member, _count, _gfp) \ __kmalloc(sizeof(*typeof(__builtin_lvalue())) + sizeof(*__builtin_lvalue()->_member) * (_count), gfp) Compiler folks seems very unhappy with this, though. As I can recognize it creates problems for stuff like: return kmalloc(...) Of course the proposed macros still have the above problem, and both to use a temporary variable to deal with it. So, really it's a question of "how best to introspect the lvalue?" > [...] > > by GCC[1] and Clang[2]. The internal use of __flex_count() allows for > > automatically setting the counter member of a struct's flexible array > > But if it's a to-be-implemented feature, perhaps it would be too early to > include it here? Were you able to even test that part right now? There are RFC patches for both GCC and Clang that I tested against. However, yes, it is still pretty early. I just wanted to show that it can work, etc. (i.e. not propose a macro with no "real" benefit over the existing assignments). > [...] > > 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(-) > > Since that could be feasible to apply only if Linus ran that directly > himself, including him now. Because doing it any other way would leave us > semi-converted forever and not bring the full benefits? Right -- I'd want to do a mass conversion and follow it up with any remaining ones. There are a lot in the style of "return k*alloc(...)" for example. > [...] > > +#define kvmalloc_obj(P, FLAGS) \ > > + __alloc_objs(kvmalloc, P, 1, FLAGS, NULL) > > Wonder if there is really a single struct (not array) with no flex array > that could need kvmalloc? :) Ah, yes, Good point. I was going for "full" macro coverage. :P Thanks for looking at this! -Kees -- Kees Cook
