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
