Re: [PATCH] docs: Introduce deprecated APIs list

[Kees Cook]

On Wed, Oct 17, 2018 at 5:50 AM, Jonathan Corbet <corbet@xxxxxxx> wrote:
> On Tue, 16 Oct 2018 19:17:08 -0700
> Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> As discussed in the "API replacement/deprecation" thread[1], this
>> makes an effort to document what things shouldn't get (re)added to the
>> kernel, by introducing Documentation/process/deprecated.rst. It also
>> adds the overflow kerndoc to ReST output, and tweaks the struct_size()
>> documentation to parse correctly.
>
> Seems like a good idea overall...a couple of quick comments
>
>> [1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2018-September/005282.html
>>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> ---
>>  Documentation/driver-api/basics.rst  |  3 +
>>  Documentation/process/deprecated.rst | 99 ++++++++++++++++++++++++++++
>>  Documentation/process/index.rst      |  1 +
>
> I wonder if "process" is the right place, or core-api?  I guess we have a
> lot of similar stuff in process now.

Totally up to you. It seemed better suited for "process" (here's
things NOT to do) than "core-api" (here's things to use).

>
> [...]
>
>> +open-coded arithmetic in allocator arguments
>> +--------------------------------------------
>> +Dynamic size calculations (especially multiplication)
>> +should not be performed in memory allocator (or similar)
>> +function arguments.
>> +
>> +For example, do not use ``rows * cols`` as an argument, as in:
>> +``kmalloc(rows * cols, GFP_KERNEL)``.
>> +Instead, the 2-factor form of the allocator should be used:
>> +``kmalloc_array(rows, cols, GFP_KERNEL)``.
>> +If no 2-factor form is available, the saturate-on-overflow helpers should
>> +be used:
>> +``vmalloc(array_size(rows, cols))``.
>> +
>> +See :c:func:`array_size`, :c:func:`array3_size`, and :c:func:`struct_size`,
>> +for more details as well as the related :c:func:`check_add_overflow` and
>> +:c:func:`check_mul_overflow` family of functions.
>
> I think this should say *why* developers are being told not to do it.  Does
> this advice hold even in cases where the dimensions are known, under the
> kernel's control, and guaranteed not to overflow even on Alan's port to
> eight-bit CPUs?

I will attempt to explain this better. When all factors are constants,
the compiler will warn if there is an overflow. If, however, anything
is a dynamic size, we run the risk of overflow. It's not true in all
cases (e.g. u8 var * sizeof()) but it's more robust to globally avoid
it. (What happens when that u8 becomes a u64 at some future time?)

> To me it's also a bit confusing to present the arguments to kmalloc_array()
> as "rows" and "cols" when they are really "n" and "size".

That's true, though I used rows * cols as an example because people
aren't always thinking about their multiplications as having n-many
sizes. e.g. "I just want a full screen of pixels." Let me see if I can
adjust it...

Thanks for the feedback!

-Kees

-- 
Kees Cook
Pixel Security