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 12:17:40PM -0700, Linus Torvalds wrote:
> On Fri, Sep 10, 2021 at 11:43 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > 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'". :|
> 
> Oh, I had entirely missed that historical subtlety of __malloc.
> 
> Yeah, that would have been absolutely horrible. But it's not actually
> really true.
> 
> It seems that the gcc people actually realized the problem, and fixed
> the documentation:
> 
>   "Attribute malloc indicates that a function is malloc-like, i.e.,
> that the pointer P returned by the function cannot alias any other
> pointer valid when the function returns, and moreover no pointers to
> valid objects occur in any storage addressed by P. In addition, the
> GCC predicts that a function with the attribute returns non-null in
> most cases"
> 
> IOW, it is purely about aliasing guarantees. Basically the guarantee
> is that the memory that a "malloc" function returns can not alias
> (directly or indirectly) any other allocations.
> 
> See
> 
>     https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes
> 
> So I think it's ok, and your reaction was entirely correct, but came
> from looking at old documentation.

Okay, sounds good. The other reason for having them be separate is that
some of our allocators are implicitly sized. (i.e. kmem_cache_alloc()),
so there isn't actually a "size" argument to give. I suppose some kind
of VARARGS macro magic could be used to make __malloc() be valid, but
I don't like that in the face of future changes where people just don't
include the argument by accident.

How about the other way around, where __malloc is included in
__alloc_size()? Then the implicitly-sized allocators are left unchanged
with __malloc.

For the mechanical part of this, I'm left needing an answer to "what's
the style guide for this?" in the face of these longer definitions,
especially in the face of potential future trailing attributes.

e.g. all on one line would be 119 characters, way past even the updated
100 character limit:

__must_check static inline void *krealloc_array(void *p, size_t new_n, size_t new_size, gfp_t flags) __alloc_size(2, 3)
{
	...
}

Maybe this? I find it weird still:

__must_check static inline void *krealloc_array(void *p, size_t new_n,
						size_t new_size, gfp_t flags)
						__alloc_size(2, 3)
{
	...
}

-- 
Kees Cook