Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 21 Jan 2020 09:51:52 +1100
From: Daniel Axtens <>
To: Michal Hocko <>
Subject: Re: [PATCH 4/5] [VERY RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX

Hi Michal,

>> For example, struct_size(struct, array member, array elements) returns the
>> size of a structure that has an array as the last element, containing a
>> given number of elements, or SIZE_MAX on overflow.
>> However, struct_size operates in (arguably) unintuitive ways at compile time.
>> Consider the following snippet:
>> struct foo {
>> 	int a;
>> 	int b[0];
>> };
>> struct foo *alloc_foo(int elems)
>> {
>> 	struct foo *result;
>> 	size_t size = struct_size(result, b, elems);
>> 	if (__builtin_constant_p(size)) {
>> 		BUILD_BUG_ON(size == SIZE_MAX);
>> 	}
>> 	result = kmalloc(size, GFP_KERNEL);
>> 	return result;
>> }
>> 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 only
>> SIZE_MAX if the constant provided was a huge number.
>> However, instead, this function hits the BUILD_BUG_ON, even if never
>> called.
>> include/linux/compiler.h:394:38: error: call to ‘__compiletime_assert_32’ declared with attribute error: BUILD_BUG_ON failed: size == 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

>> This is with gcc 9.2.1, and I've also observed it with an gcc 8 series
>> compiler.
>> My best explanation of this is:
>>  - 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.
>>  - 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'.
>>  - So the compiler takes that value into the body of the if statement and
>>    blows up.
>> But I could be totally wrong.
>> 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 size
>> of a memory allocation. Therefore, at compile time, those functions will go
>> down the constant path, specialising for the overflow case.
>> 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.
>> 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

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.


> -- 
> Michal Hocko
> SUSE Labs

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.