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 01:58:17PM -0700, Nick Desaulniers wrote:
> On Fri, Sep 10, 2021 at 1:47 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Sep 10, 2021 at 01:16:00PM -0700, Linus Torvalds wrote:
> > > So to a close approximation
> > >
> > >  - "storage class" goes first, so "static inline" etc.
> > >
> > >  - return type next (including attributes directly related to the
> > > returned value - like "__must_check")
> > >
> > >  - then function name and argument declaration
> > >
> > >  - and finally the "function argument type attributes" at the end.
> >
> > I'm going to eventually forget this thread, so I want to get it into
> > our coding style so I can find it again more easily. :) How does this
> > look?
> >
> > diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> > index 42969ab37b34..3c72f0232f02 100644
> > --- a/Documentation/process/coding-style.rst
> > +++ b/Documentation/process/coding-style.rst
> > @@ -487,6 +487,29 @@ because it is a simple way to add valuable information for the reader.
> >  Do not use the ``extern`` keyword with function prototypes as this makes
> >  lines longer and isn't strictly necessary.
> >
> > +.. code-block:: c
> > +
> > +       static __always_inline __must_check void *action(enum magic value,
> > +                                                        size_t size, u8 count,
> > +                                                        char *buffer)
> > +                                                       __alloc_size(2, 3)
> > +       {
> > +               ...
> > +       }
> > +
> > +When writing a function prototype, keep the order of elements regular. The
> > +desired order is "storage class", "return type attributes", "return
> > +type", name, arguments (as described earlier), followed by "function
> > +argument attributes". In the ``action`` function example above, ``static
> > +__always_inline`` is the "storage class" (even though ``__always_inline``
> > +is an attribute, it is treated like ``inline``). ``__must_check`` is
> 
> eh...mentioning inline as though it was a storage class doesn't seem
> precise, but I think this is a good start. Thanks Kees.

Well, hm, it's kinda like that? "where does it go?" "*everywhere*" :P
In looking at this a little longer, I do wonder about section attributes,
though. __cold is a hint, but ends up being a section attribute. And
section attributes appear to be used in the storage class (i.e.
"noinstr"). We treat "how the function should behave" attributes as
storage classes too, though (e.g. "notrace"). Is that right?

> 
> Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> Worst case, consider "inlining related attributes like __always_inline
> and noinline should follow the storage class (static, extern).
> 
> > +a "return type attribute" (describing ``void *``). ``void *`` is the
> > +"return type". ``action`` is the function name, followed by the function
> > +arguments. Finally ``__alloc_size(2,3)`` is an "function argument attribute",
> > +describing things about the function arguments. Some attributes, like
> > +``__malloc``, describe the behavior of the function more than they
> > +describe the function return type, and are more appropriately included
> > +in the "function argument attributes".
> >
> >  7) Centralized exiting of functions
> >  -----------------------------------
> >
> > --
> > Kees Cook
> 
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers

-- 
Kees Cook