Re: kalloc_objs() may not be as safe as it seems

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

Hi Kees,

On 2026-03-19T13:12:41-0700, Kees Cook wrote:
> On Wed, Mar 18, 2026 at 05:33:35PM +0100, Alejandro Colomar wrote:
> > [...]
> > After sleeping, I had some idea.
> > 
> > We could have coccinelle add typeof() around the first parameter when
> > it's an expression (not a type).  Then, we could enforce that the first
> > parameter is a type name.
> > 
> > That is:
> > 
> > 	 p = kmalloc_objs(int, 42);  // ok
> > 	-q = kmalloc_objs(*p, 7);
> > 	+q = kmalloc_objs(typeof(*p), 7);
> > 
> > I expect this would be doable with coccinelle.
> > 
> > Then, new code would be required to pass a type name.  And people could
> > slowly replace the existing typeof() calls at their own pace.
> > 
> > What do you think?
> 
> Well, it'd serve as a visual indicator, but it's redundant (typeof() is
> already used internally).

It is redundant now.  But my idea would be two steps:

1)  Add the redundant typeof() in the first parameter, when it's not a
    type already.

2)  Change the kmalloc_objs() implementation so that it doesn't do
    typeof() internally.  This would make step 1 non-redundant.

> Given it would only be a potential for
> confusion on integral types, I'm less convinced this needs solving.

It's directly adding safety for integral types only, yes.

But it's also improving readability.  There's no precedent of macros
that take a variable just for its type, and it feels a bit awkward to
read the current one.  By doing typeof(), people can relate better why
it takes that parameter.

If I read "q = kmalloc_objs(*q, 7)", the first thing I wonder is: why is
this macro reading *q??  If I read "q = kmalloc_objs(typeof(*p), 7)",
then all's fine in my brain.

> For completeness, though, this Coccinelle script:
> 
> // Options: --include-headers-for-types --all-includes --include-headers --keep-comments
> virtual patch
> 
> @type_not_var depends on patch@
> type TYPE;
> TYPE *VAR;
> identifier ALLOC =
> {kmalloc_obj,kzalloc_obj,kmalloc_objs,kzalloc_objs,kmalloc_flex,kzalloc_flex};
> @@
> 
>         VAR = ALLOC(
> -                   *VAR
> +                   TYPE
>                         , ...)
> 
> Produces:
> 
>  6007 files changed, 12430 insertions(+), 11767 deletions(-)
> 
> Which is a lot of churn...

Yeah, it would be a lot of churn if we were at rest.  But since the dust
has not yet settled, it might be doable.


Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]