[4/5,VERY,RFC] mm: kmalloc(_node): return NULL immediately for SIZE_MAX
diff mbox series

Message ID 20200120074344.504-5-dja@axtens.net
State New
Headers show
Series
  • Annotate allocation functions with alloc_size attribute
Related show

Commit Message

Daniel Axtens Jan. 20, 2020, 7:43 a.m. UTC
kmalloc is sometimes compiled with an size that at compile time may be
equal to SIZE_MAX.

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 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.

This brings kmalloc() and kmalloc_node() into line with the array functions
kmalloc_array() and kmalloc_array_node() for the overflow case. The overall
compiled size change per bloat-o-meter is in the noise (a reduction of
<0.01%).

Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 include/linux/slab.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Michal Hocko Jan. 20, 2020, 11:14 a.m. UTC | #1
On Mon 20-01-20 18:43:43, Daniel Axtens wrote:
> kmalloc is sometimes compiled with an size that at compile time may be
> equal to SIZE_MAX.
> 
> 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?

> 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
KMALLOC_MAX_CACHE_SIZE.
Daniel Axtens Jan. 20, 2020, 10:51 p.m. UTC | #2
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
today.

>> 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
> 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

> -- 
> Michal Hocko
> SUSE Labs

Patch
diff mbox series

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 03a389358562..8141c6b1882a 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -544,6 +544,9 @@  static __always_inline void *kmalloc(size_t size, gfp_t flags)
 #ifndef CONFIG_SLOB
 		unsigned int index;
 #endif
+		if (unlikely(size == SIZE_MAX))
+			return NULL;
+
 		if (size > KMALLOC_MAX_CACHE_SIZE)
 			return kmalloc_large(size, flags);
 #ifndef CONFIG_SLOB
@@ -562,6 +565,9 @@  static __always_inline void *kmalloc(size_t size, gfp_t flags)
 
 static __always_inline void *kmalloc_node(size_t size, gfp_t flags, int node)
 {
+	if (__builtin_constant_p(size) && size == SIZE_MAX)
+		return NULL;
+
 #ifndef CONFIG_SLOB
 	if (__builtin_constant_p(size) &&
 		size <= KMALLOC_MAX_CACHE_SIZE) {