On Fri, 14 Mar 2025 at 17:15, Kees Cook <kees@xxxxxxxxxx> wrote:
>
> Introduce type-aware kmalloc-family helpers to replace the common
> idioms for single, array, and flexible object allocations:
>
> kmalloc_obj(ptr, gfp);
> [ ... ]
Honestly, I absolutely hate this.
Don't do this. It's a huge mistake.
Yes, it's a really easy and convenient interface to use.
And it's a ABSOLUTELY HORRENDOUSLY BAD interface to actually then *maintain*.
Why? Because it's simply visually and syntactically entirely wrong.
It's much much too easy to miss that there's an assignment there,
because the assignment is hidden inside that macro that visually looks
like a function call.
So when you scan the code, the data flow becomes very hard to see.
So no. A hard NAK on this. It's wrong, it's bad, and it's crap.
Maintaining code is AT LEAST as important as writing it, and arguably
much more so. And making code and data flow visually clear is
important, and this is actively detrimental to that.
So I understand why you want to do this, but no, this is absolutely
not the way to do it.
It needs at a minimum some way to make it very very visually clear
that this is an assignment to 'ptr', and honestly, I do not see how to
do that cleanly.
Alternatively, this might be acceptable if the syntax makes mistakes
much harder to do. So for example, if it wasn't just an assignment,
but also declared the 'ptr' variable, maybe it would become much safer
simply because it would make the compiler warn about mis-uses.
Using visual cues (something that makes it look like it's not a
regular function call) might also help. The traditional C way is
obviously to use ALL_CAPS() names, which is how we visually show "this
is a macro that doesn't necessarily work like the normal stuff". Some
variation of that might help the fundamental issue with your
horrendous thing.
But something very serious needs to be done before this is acceptable.
Because no, the advantage of writing
kmalloc_obj(ptr, gfp);
instead of
ptr = kmalloc(sizeof(*ptr), gfp);
is absolutely NOT worth the horrendous disadvantages of that
disgusting and wrong syntax. You saved a handful of characters, and
made the code faster to write, at the cost of making the result be
much worse.
My suggestion would be to look at some bigger pattern, maybe including
that declaration. To take a real example matching that kind of
pattern, we have
struct mod_unload_taint *mod_taint;
...
mod_taint = kmalloc(sizeof(*mod_taint), GFP_KERNEL);
and maybe those *two* lines could be combined into something saner like
ALLOC(mod_taint, struct mod_unload_taint, GFP_KERNEL);
which would stand out visually (which is admittedly very different
from being "pretty" ;), and would also save you some typing because it
also gets rid of the declaration.
We allow declarations in the middle of code these days because we
needed it for the guarding macros, so this would be a new kind of
interface that dos that.
And no, I'm not married to that disgusting "ALLOC()" thing. I'm
throwing it out not as TheSOlution(tm), but as a "this interface
absolutely needs clarity and must stand out syntactically both to
humans and the compiler".
Linus
