[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <202408121113.D638922E4@keescook>
Date: Mon, 12 Aug 2024 11:22:00 -0700
From: Kees Cook <kees@...nel.org>
To: Vlastimil Babka <vbabka@...e.cz>
Cc: Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>,
David Rientjes <rientjes@...gle.com>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Hyeonggon Yoo <42.hyeyoo@...il.com>,
"Gustavo A . R . Silva" <gustavoars@...nel.org>,
Bill Wendling <morbo@...gle.com>,
Justin Stitt <justinstitt@...gle.com>, Jann Horn <jannh@...gle.com>,
Przemek Kitszel <przemyslaw.kitszel@...el.com>,
Marco Elver <elver@...gle.com>, linux-mm@...ck.org,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH] slab: Introduce kmalloc_obj() and family
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
Powered by blists - more mailing lists