| Message ID | 20200120074344.504-5-dja@axtens.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Annotate allocation functions with alloc_size attribute | expand |
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.
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
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) {
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(+)