From: Kees Cook <keescook@chromium.org>
To: Jonathan Corbet <corbet@lwn.net>
Cc: "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Stephen Rothwell <sfr@canb.auug.org.au>
Subject: Re: [PATCH] docs: Introduce deprecated APIs list
Date: Wed, 17 Oct 2018 15:51:22 -0700 [thread overview]
Message-ID: <CAGXu5j+CP9aqGNxO9HvbMgR9rJHx1d1=5vS7id8S-bUWRASw_g@mail.gmail.com> (raw)
In-Reply-To: <20181017065011.40ad8b24@lwn.net>
On Wed, Oct 17, 2018 at 5:50 AM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Tue, 16 Oct 2018 19:17:08 -0700
> Kees Cook <keescook@chromium.org> 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@chromium.org>
>> ---
>> 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
next prev parent reply other threads:[~2018-10-17 22:51 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-17 2:17 [PATCH] docs: Introduce deprecated APIs list Kees Cook
2018-10-17 3:25 ` Randy Dunlap
2018-10-17 10:00 ` Jani Nikula
2018-10-17 17:08 ` Randy Dunlap
2018-10-17 22:37 ` Kees Cook
2018-10-17 22:41 ` Randy Dunlap
2018-10-17 23:34 ` Randy Dunlap
2018-10-17 12:50 ` Jonathan Corbet
2018-10-17 22:51 ` Kees Cook [this message]
2018-10-21 15:11 ` Miguel Ojeda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGXu5j+CP9aqGNxO9HvbMgR9rJHx1d1=5vS7id8S-bUWRASw_g@mail.gmail.com' \
--to=keescook@chromium.org \
--cc=corbet@lwn.net \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sfr@canb.auug.org.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).