linux-kernel - Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 May 2017 13:18:11 +0300
From:   Alexey Dobriyan <adobriyan@...il.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Joe Perches <joe@...ches.com>,
        Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred
 allocation form

On Tue, May 23, 2017 at 1:22 AM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <joe@...ches.com> wrote:
>
>> On Tue, 2017-05-23 at 00:38 +0300, Alexey Dobriyan wrote:

>> > -The preferred form for passing a size of a struct is the following:
>> > -
>> > -.. code-block:: c
>> > -
>> > -   p = kmalloc(sizeof(*p), ...);
>> > -
>> > -The alternative form where struct name is spelled out hurts readability and
>> > -introduces an opportunity for a bug when the pointer variable type is changed
>> > -but the corresponding sizeof that is passed to a memory allocator is not.
>> > -
>> >  Casting the return value which is a void pointer is redundant. The conversion
>> >  from void pointer to any other pointer type is guaranteed by the C programming
>> >  language.
>>
>> Thanks.  I agree with this deletion.
>
> I don't.  Every damn time I see a p = kmalloc(sizeof struct foo) I have
> to hunt around to check the type of p.

Maybe you should stop doing it. It single digit number of issues per years(!).
People simply don't make that kind of mistake (or at least very rarely).

The problem is that split is 2:1 (really it is 60:40 or less because
CodingStyle and
checkpatch.pl favour one style).

Proper fix is to introduce typed allocation macros with the following
signatures:

T* lmalloc(T, gfp);
T* lmalloc2(n1, T, gfp); //kmalloc_array
T* lmalloc3(n1, n2, T, gfp); //kmalloc_array 3D
...
T* lzalloc(T, gfp);
T* lzalloc2(n1, gfp); //kcalloc

T* lmalloc_priv(T, len_priv, gfp);
T* lmalloc_priv2(T, n1, Tpriv, gfp);
...
T* lmalloc2_priv2(n1, T, npriv, Tpriv);

etc

This covers 99.9% of allocations leaving kmalloc() as "legacy" API for
odd cases.

It has consistent and orthogonal naming:

  l -- Linux
  [v] -- vmalloc, optional
  [mz] -- regular or zeroed allocation
  alloc
  [234] -- number of dimensions.
  _priv[2] -- additionally allocate private area for "ribbon" arrays

OpenBSD added reallocarray() for 2D arrays, What they are going to do
with 3D arrays?

You'd write

    struct foo *x;
    x = lmalloc(struct foo, GFP_KERNEL);
    ...

One style is automatically enforced and typechecking is in place.
It is less typing in case of "sizeof(struct foo)" and slightly more on average
in case of  "sizeof(*foo)". Overall it will be less typing.

For array allocations you'd write:

  struct foo **x;
  x = lmalloc2(n, struct foo, GFP_KERNEL);

so that again style is enforced and there is no bikeschedding which argument
should go first: number of elements or sizeof).

Integer overflows checks are easyly added.

Currently once in a while someone rediscovers that piece of CodingStyle and
start sending trivial patches ad infinitum.