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

[Kees Cook <kees@kernel.org>]

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