Re: [patch 9/9] mm/vmalloc: add __alloc_size attributes for better bounds checking

[Kees Cook <keescook@chromium.org>]

On Fri, Sep 10, 2021 at 10:23:48AM -0700, Linus Torvalds wrote:
> On Thu, Sep 9, 2021 at 8:10 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > +__alloc_size(1)
> >  extern void *vmalloc(unsigned long size);
> [...]
> 
> All of these are added in the wrong place - inconsistent with the very
> compiler documentation the patches add.
> 
> The function attributes are generally added _after_ the function,
> although admittedly we've been quite confused here before.
> 
> But the very compiler documentation you point to in the patch that
> adds these macros gives that as the examples both for gcc and clang:
> 
> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alloc_005fsize-function-attribute
> + * clang: https://clang.llvm.org/docs/AttributeReference.html#alloc-size
> 
> and honestly I think that is the preferred format because this is
> about the *function*, not about the return type.
> 
> Do both placements work? Yes.
> 
> Have we been confused about this before? Yes. I note that our __printf
> attributes in particular have been added in odd places. And our
> existing __malloc annotations seem to correct in <linux/slab.h> and
> <linux/device.h> but then randomly applied in some other places.
> 
> I really think it's pointlessly stupid and hard to read/grep for to
> make it be a separate line before the whole thing.

This was bike-shed on the list, and this result seemed to be consensus,
but I kind of dislike all the options. Either things are on separate
lines or they're trailing attributes that get really long, etc. Ugh.

I'm happy to clean all of it up into whatever form can be agreed on for
the "correct" placement.

> I also think it should have taken over the "__malloc" name that is
> almost unused right now. Because why would you ever have
> __alloc_size() without having __malloc().

I had originally set out to do that, but the problem with merging with
__malloc is the bit in the docs about "and that the memory has undefined
content". So we can't do that for kmalloc() in the face of GFP_ZERO, as
well as a bunch of other helpers. I always get suspicious about "this
will improve optimization because we depend on claiming something is
'undefined'". :|

-Kees

-- 
Kees Cook