Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Daniel Axtens <dja@axtens.net>
To:     Michal Hocko <mhocko@kernel.org>
Cc:     kernel-hardening@lists.openwall.com, linux-mm@kvack.org,
        keescook@chromium.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org
Subject: Re: [PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX
In-Reply-To: <20200120111411.GX18451@dhcp22.suse.cz>
References: <20200120074344.504-1-dja@axtens.net> <20200120074344.504-5-dja@axtens.net> <20200120111411.GX18451@dhcp22.suse.cz>
Date:   Tue, 21 Jan 2020 09:51:52 +1100
Message-ID: <87pnfdkagn.fsf@dja-thinkpad.axtens.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Michal,

>> For example, struct_size(struct, array member, array elements) returns t=
he
>> size of a structure that has an array as the last element, containing a
>> given number of elements, or SIZE_MAX on overflow.
>>=20
>> However, struct_size operates in (arguably) unintuitive ways at compile =
time.
>> Consider the following snippet:
>>=20
>> struct foo {
>> 	int a;
>> 	int b[0];
>> };
>>=20
>> struct foo *alloc_foo(int elems)
>> {
>> 	struct foo *result;
>> 	size_t size =3D struct_size(result, b, elems);
>> 	if (__builtin_constant_p(size)) {
>> 		BUILD_BUG_ON(size =3D=3D SIZE_MAX);
>> 	}
>> 	result =3D kmalloc(size, GFP_KERNEL);
>> 	return result;
>> }
>>=20
>> I expected that size would only be constant if alloc_foo() was called
>> within that translation unit with a constant number of elements, and the
>> compiler had decided to inline it. I'd therefore expect that 'size' is o=
nly
>> SIZE_MAX if the constant provided was a huge number.
>>=20
>> However, instead, this function hits the BUILD_BUG_ON, even if never
>> called.
>>=20
>> include/linux/compiler.h:394:38: error: call to =E2=80=98__compiletime_a=
ssert_32=E2=80=99 declared with attribute error: BUILD_BUG_ON failed: size =
=3D=3D SIZE_MAX
>
> This sounds more like a bug to me. Have you tried to talk to compiler
> guys?

You're now the second person to suggest this to me, so I will do that
today.

>> This is with gcc 9.2.1, and I've also observed it with an gcc 8 series
>> compiler.
>>=20
>> My best explanation of this is:
>>=20
>>  - elems is a signed int, so a small negative number will become a very
>>    large unsigned number when cast to a size_t, leading to overflow.
>>=20
>>  - Then, the only way in which size can be a constant is if we hit the
>>    overflow case, in which 'size' will be 'SIZE_MAX'.
>>=20
>>  - So the compiler takes that value into the body of the if statement and
>>    blows up.
>>=20
>> But I could be totally wrong.
>>=20
>> Anyway, this is relevant to slab.h because kmalloc() and kmalloc_node()
>> check if the supplied size is a constant and take a faster path if so. A
>> number of callers of those functions use struct_size to determine the si=
ze
>> of a memory allocation. Therefore, at compile time, those functions will=
 go
>> down the constant path, specialising for the overflow case.
>>=20
>> When my next patch is applied, gcc will then throw a warning any time
>> kmalloc_large could be called with a SIZE_MAX size, as gcc deems SIZE_MAX
>> to be too big an allocation.
>>=20
>> So, make functions that check __builtin_constant_p check also against
>> SIZE_MAX in the constant path, and immediately return NULL if we hit it.
>
> I am not sure I am happy about an additional conditional path in the hot
> path of the allocator. Especially when we already have a check for
> KMALLOC_MAX_CACHE_SIZE.

It is guarded by __builtin_constant_p in both cases, so it should not
cause an additional runtime branch. But I'll check in with our friendly
local compiler folks and see where that leads first.

Regards,
Daniel

> --=20
> Michal Hocko
> SUSE Labs