| Message ID | 20210511173448.GA54466@hyeyoo (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time | expand |
sorry for being annoying. I noticed that I sent patch in wrong format (and wrong subject.) so sent patch v3 again in right format. so the bot can pick up the patch. please let me know if I can help something!
On 5/11/21 7:34 PM, Hyeonggon Yoo wrote: > currently when size is not supported by kmalloc_index, compiler will > generate a run-time BUG() while compile-time error is also possible, > and better. so changed BUG to BUILD_BUG_ON_MSG to make compile-time > check possible. > > also removed code that allocates more than 32MB because current > implementation supports only up to 32MB. > > Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks! > --- > include/linux/slab.h | 7 +++++-- > mm/slab_common.c | 7 +++---- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 6d454886bcaf..df1937309df2 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -345,6 +345,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) > * 1 = 65 .. 96 bytes > * 2 = 129 .. 192 bytes > * n = 2^(n-1)+1 .. 2^n > + * > + * Note: there's no need to optimize kmalloc_index because it's evaluated > + * in compile-time. > */ > static __always_inline unsigned int kmalloc_index(size_t size) > { > @@ -381,8 +384,8 @@ static __always_inline unsigned int kmalloc_index(size_t size) > if (size <= 8 * 1024 * 1024) return 23; > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > - if (size <= 64 * 1024 * 1024) return 26; > - BUG(); > + > + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > > /* Will never be reached. Needed because the compiler may complain */ > return -1; > diff --git a/mm/slab_common.c b/mm/slab_common.c > index fe8b68482670..97664bbe8147 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1192,8 +1192,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) > > /* > * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. > - * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is > - * kmalloc-67108864. > + * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is > + * kmalloc-32M. > */ > const struct kmalloc_info_struct kmalloc_info[] __initconst = { > INIT_KMALLOC_INFO(0, 0), > @@ -1221,8 +1221,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = { > INIT_KMALLOC_INFO(4194304, 4M), > INIT_KMALLOC_INFO(8388608, 8M), > INIT_KMALLOC_INFO(16777216, 16M), > - INIT_KMALLOC_INFO(33554432, 32M), > - INIT_KMALLOC_INFO(67108864, 64M) > + INIT_KMALLOC_INFO(33554432, 32M) > }; > > /* >
On Wed, 12 May 2021 02:34:48 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > currently when size is not supported by kmalloc_index, compiler will > generate a run-time BUG() while compile-time error is also possible, > and better. so changed BUG to BUILD_BUG_ON_MSG to make compile-time > check possible. > > also removed code that allocates more than 32MB because current > implementation supports only up to 32MB. > This explodes in mysterious ways. The patch as I have it is appended, for reference. gcc-10.3.0 allmodconfig. mm/kfence/kfence_test.c: In function 'test_free_bulk': mm/kfence/kfence_test.c:519:16: warning: unused variable 'size' [-Wunused-variable] 519 | const size_t size = setup_test_cache(test, 8 + prandom_u32_max(300), 0, | ^~~~ In file included from <command-line>: In function 'kmalloc_index', inlined from 'test_alloc' at mm/kfence/kfence_test.c:270:82: ././include/linux/compiler_types.h:328:38: error: call to '__compiletime_assert_922' declared with attribute error: unexpected size in kmalloc_index() 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ ././include/linux/compiler_types.h:309:4: note: in definition of macro '__compiletime_assert' 309 | prefix ## suffix(); \ | ^~~~~~ ././include/linux/compiler_types.h:328:2: note: in expansion of macro '_compiletime_assert' 328 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ ./include/linux/slab.h:389:2: note: in expansion of macro 'BUILD_BUG_ON_MSG' 389 | BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); | ^~~~~~~~~~~~~~~~ make[2]: *** [mm/kfence/kfence_test.o] Error 1 make[1]: *** [mm/kfence] Error 2 make: *** [mm] Error 2 This patch suppresses the error: --- a/mm/kfence/kfence_test.c~a +++ a/mm/kfence/kfence_test.c @@ -318,13 +318,13 @@ static void test_out_of_bounds_read(stru /* Test both sides. */ - buf = test_alloc(test, size, GFP_KERNEL, ALLOCATE_LEFT); + buf = test_alloc(test, 32, GFP_KERNEL, ALLOCATE_LEFT); expect.addr = buf - 1; READ_ONCE(*expect.addr); KUNIT_EXPECT_TRUE(test, report_matches(&expect)); test_free(buf); - buf = test_alloc(test, size, GFP_KERNEL, ALLOCATE_RIGHT); + buf = test_alloc(test, 32, GFP_KERNEL, ALLOCATE_RIGHT); expect.addr = buf + size; READ_ONCE(*expect.addr); KUNIT_EXPECT_TRUE(test, report_matches(&expect)); @@ -519,11 +519,11 @@ static void test_free_bulk(struct kunit const size_t size = setup_test_cache(test, 8 + prandom_u32_max(300), 0, (iter & 1) ? ctor_set_x : NULL); void *objects[] = { - test_alloc(test, size, GFP_KERNEL, ALLOCATE_RIGHT), - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), - test_alloc(test, size, GFP_KERNEL, ALLOCATE_LEFT), - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_RIGHT), + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_LEFT), + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), }; kmem_cache_free_bulk(test_cache, ARRAY_SIZE(objects), objects); Is gcc-10.3.0 simply confused? test_out_of_bounds_read() is clearly calling kmalloc_index(32) which is OK. Anyway, I'll drop this patch for now so I can compile a kernel! From: Hyeonggon Yoo <42.hyeyoo@gmail.com> Subject: mm, slub: change run-time assertion in kmalloc_index() to compile-time Currently when size is not supported by kmalloc_index, compiler will generate a run-time BUG() while compile-time error is also possible, and better. So change BUG to BUILD_BUG_ON_MSG to make compile-time check possible. Also remove code that allocates more than 32MB because current implementation supports only up to 32MB. Link: https://lkml.kernel.org/r/20210511173448.GA54466@hyeyoo Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- include/linux/slab.h | 7 +++++-- mm/slab_common.c | 7 +++---- 2 files changed, 8 insertions(+), 6 deletions(-) --- a/include/linux/slab.h~mm-slub-change-run-time-assertion-in-kmalloc_index-to-compile-time +++ a/include/linux/slab.h @@ -346,6 +346,9 @@ static __always_inline enum kmalloc_cach * 1 = 65 .. 96 bytes * 2 = 129 .. 192 bytes * n = 2^(n-1)+1 .. 2^n + * + * Note: there's no need to optimize kmalloc_index because it's evaluated + * in compile-time. */ static __always_inline unsigned int kmalloc_index(size_t size) { @@ -382,8 +385,8 @@ static __always_inline unsigned int kmal if (size <= 8 * 1024 * 1024) return 23; if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - if (size <= 64 * 1024 * 1024) return 26; - BUG(); + + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); /* Will never be reached. Needed because the compiler may complain */ return -1; --- a/mm/slab_common.c~mm-slub-change-run-time-assertion-in-kmalloc_index-to-compile-time +++ a/mm/slab_common.c @@ -755,8 +755,8 @@ struct kmem_cache *kmalloc_slab(size_t s /* * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. - * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is - * kmalloc-67108864. + * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is + * kmalloc-32M. */ const struct kmalloc_info_struct kmalloc_info[] __initconst = { INIT_KMALLOC_INFO(0, 0), @@ -784,8 +784,7 @@ const struct kmalloc_info_struct kmalloc INIT_KMALLOC_INFO(4194304, 4M), INIT_KMALLOC_INFO(8388608, 8M), INIT_KMALLOC_INFO(16777216, 16M), - INIT_KMALLOC_INFO(33554432, 32M), - INIT_KMALLOC_INFO(67108864, 64M) + INIT_KMALLOC_INFO(33554432, 32M) }; /*
On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: > This explodes in mysterious ways. The patch as I have it is appended, > for reference. > > gcc-10.3.0 allmodconfig. > > This patch suppresses the error: > > --- a/mm/kfence/kfence_test.c~a > +++ a/mm/kfence/kfence_test.c > @@ -318,13 +318,13 @@ static void test_out_of_bounds_read(stru > > /* Test both sides. */ > > - buf = test_alloc(test, size, GFP_KERNEL, ALLOCATE_LEFT); > + buf = test_alloc(test, 32, GFP_KERNEL, ALLOCATE_LEFT); > expect.addr = buf - 1; > READ_ONCE(*expect.addr); > KUNIT_EXPECT_TRUE(test, report_matches(&expect)); > test_free(buf); > > - buf = test_alloc(test, size, GFP_KERNEL, ALLOCATE_RIGHT); > + buf = test_alloc(test, 32, GFP_KERNEL, ALLOCATE_RIGHT); > expect.addr = buf + size; > READ_ONCE(*expect.addr); > KUNIT_EXPECT_TRUE(test, report_matches(&expect)); > @@ -519,11 +519,11 @@ static void test_free_bulk(struct kunit > const size_t size = setup_test_cache(test, 8 + prandom_u32_max(300), 0, > (iter & 1) ? ctor_set_x : NULL); > void *objects[] = { > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_RIGHT), > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_LEFT), > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_RIGHT), > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_LEFT), > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), > }; > > kmem_cache_free_bulk(test_cache, ARRAY_SIZE(objects), objects); > > > Is gcc-10.3.0 simply confused? test_out_of_bounds_read() is clearly > calling kmalloc_index(32) which is OK. > > Anyway, I'll drop this patch for now so I can compile a kernel! > The error messages isn't so clear to me. but one problem I can see is in kfence_test.c, there are many places that are using size which is not constant. in kmalloc if size is not constant, it calls dummy function __kmalloc which does not make use of size.
On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: > > This explodes in mysterious ways. The patch as I have it is appended, > > for reference. > > > > gcc-10.3.0 allmodconfig. > > > > This patch suppresses the error: > > > > --- a/mm/kfence/kfence_test.c~a > > +++ a/mm/kfence/kfence_test.c > > @@ -318,13 +318,13 @@ static void test_out_of_bounds_read(stru > > > > /* Test both sides. */ > > > > - buf = test_alloc(test, size, GFP_KERNEL, ALLOCATE_LEFT); > > + buf = test_alloc(test, 32, GFP_KERNEL, ALLOCATE_LEFT); > > expect.addr = buf - 1; > > READ_ONCE(*expect.addr); > > KUNIT_EXPECT_TRUE(test, report_matches(&expect)); > > test_free(buf); > > > > - buf = test_alloc(test, size, GFP_KERNEL, ALLOCATE_RIGHT); > > + buf = test_alloc(test, 32, GFP_KERNEL, ALLOCATE_RIGHT); > > expect.addr = buf + size; > > READ_ONCE(*expect.addr); > > KUNIT_EXPECT_TRUE(test, report_matches(&expect)); > > @@ -519,11 +519,11 @@ static void test_free_bulk(struct kunit > > const size_t size = setup_test_cache(test, 8 + prandom_u32_max(300), 0, > > (iter & 1) ? ctor_set_x : NULL); > > void *objects[] = { > > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_RIGHT), > > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), > > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_LEFT), > > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), > > - test_alloc(test, size, GFP_KERNEL, ALLOCATE_NONE), > > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_RIGHT), > > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), > > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_LEFT), > > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), > > + test_alloc(test, 32, GFP_KERNEL, ALLOCATE_NONE), > > }; > > > > kmem_cache_free_bulk(test_cache, ARRAY_SIZE(objects), objects); > > > > > > Is gcc-10.3.0 simply confused? test_out_of_bounds_read() is clearly > > calling kmalloc_index(32) which is OK. > > > > Anyway, I'll drop this patch for now so I can compile a kernel! > > > > The error messages isn't so clear to me. > but one problem I can see is in kfence_test.c, there are many places that > are using size which is not constant. Ah, yes, of course, your patch changes kmalloc_index() to require that it always is called with a constant `size'. kfence_test doesn't do that. kfence is being a bit naughty here - the other kmalloc_index() callers only comple up the call after verifying that `size' is a compile-time constant. Would something like this work? include/linux/slab.h | 12 ++++++++---- mm/kfence/kfence_test.c | 4 ++-- 2 files changed, 10 insertions(+), 6 deletions(-) --- a/include/linux/slab.h~b +++ a/include/linux/slab.h @@ -374,7 +374,8 @@ static __always_inline enum kmalloc_cach * Note: there's no need to optimize kmalloc_index because it's evaluated * in compile-time. */ -static __always_inline unsigned int kmalloc_index(size_t size) +static __always_inline unsigned int kmalloc_index(size_t size, + bool size_is_constant) { if (!size) return 0; @@ -410,7 +411,10 @@ static __always_inline unsigned int kmal if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); + if (size_is_constant) + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); + else + BUG(); /* Will never be reached. Needed because the compiler may complain */ return -1; @@ -575,7 +579,7 @@ static __always_inline void *kmalloc(siz if (size > KMALLOC_MAX_CACHE_SIZE) return kmalloc_large(size, flags); #ifndef CONFIG_SLOB - index = kmalloc_index(size); + index = kmalloc_index(size, true); if (!index) return ZERO_SIZE_PTR; @@ -593,7 +597,7 @@ static __always_inline void *kmalloc_nod #ifndef CONFIG_SLOB if (__builtin_constant_p(size) && size <= KMALLOC_MAX_CACHE_SIZE) { - unsigned int i = kmalloc_index(size); + unsigned int i = kmalloc_index(size, true); if (!i) return ZERO_SIZE_PTR; --- a/mm/kfence/kfence_test.c~b +++ a/mm/kfence/kfence_test.c @@ -197,7 +197,7 @@ static void test_cache_destroy(void) static inline size_t kmalloc_cache_alignment(size_t size) { - return kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size)]->align; + return kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size, false)]->align; } /* Must always inline to match stack trace against caller. */ @@ -267,7 +267,7 @@ static void *test_alloc(struct kunit *te if (is_kfence_address(alloc)) { struct page *page = virt_to_head_page(alloc); - struct kmem_cache *s = test_cache ?: kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size)]; + struct kmem_cache *s = test_cache ?: kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size, false)]; /* * Verify that various helpers return the right values
On Wed, May 12, 2021 at 08:40:24PM -0700, Andrew Morton wrote: > On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > > On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: > > > This explodes in mysterious ways. The patch as I have it is appended, > > > for reference. > > > > > > gcc-10.3.0 allmodconfig. > > > > > > This patch suppresses the error: > > Ah, yes, of course, your patch changes kmalloc_index() to require that > it always is called with a constant `size'. kfence_test doesn't do > that. > > kfence is being a bit naughty here - the other kmalloc_index() callers > only comple up the call after verifying that `size' is a compile-time > constant. > > Would something like this work? > include/linux/slab.h | 12 ++++++++---- > mm/kfence/kfence_test.c | 4 ++-- > 2 files changed, 10 insertions(+), 6 deletions(-) > > --- a/include/linux/slab.h~b > +++ a/include/linux/slab.h > @@ -374,7 +374,8 @@ static __always_inline enum kmalloc_cach > * Note: there's no need to optimize kmalloc_index because it's evaluated > * in compile-time. > */ > -static __always_inline unsigned int kmalloc_index(size_t size) > +static __always_inline unsigned int kmalloc_index(size_t size, > + bool size_is_constant) > { > if (!size) > return 0; > @@ -410,7 +411,10 @@ static __always_inline unsigned int kmal > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > > - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > + if (size_is_constant) > + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > + else > + BUG(); kfence is randomly generating size. because kfence is using non-constant size, we should do run-time assertion or compile-time assertion depending on situation. I think we can use __builtin_constant_p here. we don't need to modify kmalloc_index's prototype. so what about this? if you think it makes sense, I'll send patch v4. I used KMALLOC_MAX_CACHE_SIZE to assure it's safe size. it's safer than putting BUILD_BUG_ON_MSG(1, ...) to below if statements because KMALLOC_MAX_CACHE_SIZE can be less than 32MB. --- include/linux/slab.h.orig 2021-05-12 17:56:54.504738768 +0900 +++ include/linux/slab.h 2021-05-13 15:06:25.724565850 +0900 @@ -346,9 +346,18 @@ static __always_inline enum kmalloc_cach * 1 = 65 .. 96 bytes * 2 = 129 .. 192 bytes * n = 2^(n-1)+1 .. 2^n + * + * Note: there's no need to optimize kmalloc_index because it's evaluated + * in compile-time. */ static __always_inline unsigned int kmalloc_index(size_t size) { + if (__builtin_constant_p(size)) { + BUILD_BUG_ON_MSG(size > KMALLOC_MAX_CACHE_SIZE , "unexpected size in kmalloc_index()"); + } else if (size > KMALLOC_MAX_CACHE_SIZE) { + BUG(); + } + if (!size) return 0; @@ -382,8 +391,6 @@ static __always_inline unsigned int kmal if (size <= 8 * 1024 * 1024) return 23; if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - if (size <= 64 * 1024 * 1024) return 26; - BUG(); /* Will never be reached. Needed because the compiler may complain */ return -1;
On Thu, 13 May 2021 at 08:28, Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > On Wed, May 12, 2021 at 08:40:24PM -0700, Andrew Morton wrote: > > On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > > On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: > > > > This explodes in mysterious ways. The patch as I have it is appended, > > > > for reference. > > > > > > > > gcc-10.3.0 allmodconfig. > > > > > > > > This patch suppresses the error: > > > > Ah, yes, of course, your patch changes kmalloc_index() to require that > > it always is called with a constant `size'. kfence_test doesn't do > > that. > > > > kfence is being a bit naughty here - the other kmalloc_index() callers > > only comple up the call after verifying that `size' is a compile-time > > constant. > > > > Would something like this work? > > include/linux/slab.h | 12 ++++++++---- > > mm/kfence/kfence_test.c | 4 ++-- > > 2 files changed, 10 insertions(+), 6 deletions(-) > > > > --- a/include/linux/slab.h~b > > +++ a/include/linux/slab.h > > @@ -374,7 +374,8 @@ static __always_inline enum kmalloc_cach > > * Note: there's no need to optimize kmalloc_index because it's evaluated > > * in compile-time. > > */ > > -static __always_inline unsigned int kmalloc_index(size_t size) > > +static __always_inline unsigned int kmalloc_index(size_t size, > > + bool size_is_constant) > > { > > if (!size) > > return 0; > > @@ -410,7 +411,10 @@ static __always_inline unsigned int kmal > > if (size <= 16 * 1024 * 1024) return 24; > > if (size <= 32 * 1024 * 1024) return 25; > > > > - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > > + if (size_is_constant) > > + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > > + else > > + BUG(); > > > kfence is randomly generating size. because kfence is using non-constant > size, we should do run-time assertion or compile-time assertion depending > on situation. > > I think we can use __builtin_constant_p here. we don't need to modify > kmalloc_index's prototype. > > so what about this? > if you think it makes sense, I'll send patch v4. > > I used KMALLOC_MAX_CACHE_SIZE to assure it's safe size. > it's safer than putting BUILD_BUG_ON_MSG(1, ...) to below if statements > because KMALLOC_MAX_CACHE_SIZE can be less than 32MB. I'm actually inclined to say that Andrew's patch with 'size_is_constant' is the better option, because we want to be explicit about where it's using constant size and where it isn't. I think in tests like kfence_test, it should be permitted to use non-constant size, it's a test after all and performance is no concern. For non-test code, however, we want to ensure size is constant, and therefore having the distinguishing argument makes sense. That way non-test code will not compile if our intent does not match reality. Thanks, -- Marco
On 5/13/21 8:28 AM, Hyeonggon Yoo wrote: > On Wed, May 12, 2021 at 08:40:24PM -0700, Andrew Morton wrote: >> On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: >> >> > On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: >> > > This explodes in mysterious ways. The patch as I have it is appended, >> > > for reference. >> > > >> > > gcc-10.3.0 allmodconfig. >> > > >> > > This patch suppresses the error: >> >> Ah, yes, of course, your patch changes kmalloc_index() to require that >> it always is called with a constant `size'. kfence_test doesn't do >> that. >> >> kfence is being a bit naughty here - the other kmalloc_index() callers >> only comple up the call after verifying that `size' is a compile-time >> constant. Agreed. >> Would something like this work? I'd prefer if we kept kmalloc_index() for constant sizes only. The broken build then warns anyone using it the wrong way that they shouldn't. Besides, it really shouldn't be used outside of slab. But if kfence test really needs this, we could perhaps extract the index determining part out of kmalloc_slab(). Hmm or I guess the kfence tests could just use kmalloc_slab() directly? >> include/linux/slab.h | 12 ++++++++---- >> mm/kfence/kfence_test.c | 4 ++-- >> 2 files changed, 10 insertions(+), 6 deletions(-) >> >> --- a/include/linux/slab.h~b >> +++ a/include/linux/slab.h >> @@ -374,7 +374,8 @@ static __always_inline enum kmalloc_cach >> * Note: there's no need to optimize kmalloc_index because it's evaluated >> * in compile-time. >> */ >> -static __always_inline unsigned int kmalloc_index(size_t size) >> +static __always_inline unsigned int kmalloc_index(size_t size, >> + bool size_is_constant) >> { >> if (!size) >> return 0; >> @@ -410,7 +411,10 @@ static __always_inline unsigned int kmal >> if (size <= 16 * 1024 * 1024) return 24; >> if (size <= 32 * 1024 * 1024) return 25; >> >> - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); >> + if (size_is_constant) >> + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); >> + else >> + BUG(); > > > kfence is randomly generating size. because kfence is using non-constant > size, we should do run-time assertion or compile-time assertion depending > on situation. > > I think we can use __builtin_constant_p here. we don't need to modify > kmalloc_index's prototype. > > so what about this? > if you think it makes sense, I'll send patch v4. > > I used KMALLOC_MAX_CACHE_SIZE to assure it's safe size. > it's safer than putting BUILD_BUG_ON_MSG(1, ...) to below if statements > because KMALLOC_MAX_CACHE_SIZE can be less than 32MB. > > --- include/linux/slab.h.orig 2021-05-12 17:56:54.504738768 +0900 > +++ include/linux/slab.h 2021-05-13 15:06:25.724565850 +0900 > @@ -346,9 +346,18 @@ static __always_inline enum kmalloc_cach > * 1 = 65 .. 96 bytes > * 2 = 129 .. 192 bytes > * n = 2^(n-1)+1 .. 2^n > + * > + * Note: there's no need to optimize kmalloc_index because it's evaluated > + * in compile-time. > */ > static __always_inline unsigned int kmalloc_index(size_t size) > { > + if (__builtin_constant_p(size)) { > + BUILD_BUG_ON_MSG(size > KMALLOC_MAX_CACHE_SIZE , "unexpected size in kmalloc_index()"); > + } else if (size > KMALLOC_MAX_CACHE_SIZE) { > + BUG(); > + } > + > if (!size) > return 0; > > @@ -382,8 +391,6 @@ static __always_inline unsigned int kmal > if (size <= 8 * 1024 * 1024) return 23; > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > - if (size <= 64 * 1024 * 1024) return 26; > - BUG(); > > /* Will never be reached. Needed because the compiler may complain */ > return -1; >
On Thu, May 13, 2021 at 10:51AM +0200, Vlastimil Babka wrote: > On 5/13/21 8:28 AM, Hyeonggon Yoo wrote: > > On Wed, May 12, 2021 at 08:40:24PM -0700, Andrew Morton wrote: > >> On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > >> > On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: > >> > > This explodes in mysterious ways. The patch as I have it is appended, > >> > > for reference. > >> > > > >> > > gcc-10.3.0 allmodconfig. > >> > > > >> > > This patch suppresses the error: > >> > >> Ah, yes, of course, your patch changes kmalloc_index() to require that > >> it always is called with a constant `size'. kfence_test doesn't do > >> that. > >> > >> kfence is being a bit naughty here - the other kmalloc_index() callers > >> only comple up the call after verifying that `size' is a compile-time > >> constant. > > Agreed. It's just a test, and performance doesn't matter for it. The thing is this function lives in <linux/slab.h>, isn't prefixed with __ or anything like that, so it really does look like a public function. > >> Would something like this work? > > I'd prefer if we kept kmalloc_index() for constant sizes only. The broken build > then warns anyone using it the wrong way that they shouldn't. Agreed. Andrew's size_is_constant would do that. Also see my suggestion below to keep the same interface. > Besides, it really > shouldn't be used outside of slab. It's an allocator test. If we want to facilitate testing, it must be allowed to verify or set up test cases that test boundary conditions based on internal state. In the case of kfence_test it wants: the cache's alignment to create accesses that fall on alignment boundaries; and to verify obj_to_index() and objs_per_slab_page() are set up correctly. I think the requirements are: 1. Make the interface hard to abuse. Adding the BUILD_BUG_ON does that. 2. Facilitate testing. > But if kfence test really needs this, we could perhaps extract the index > determining part out of kmalloc_slab(). That would duplicate kmalloc_index()? I don't see the need, let's keep things simple. > Hmm or I guess the kfence tests could just use kmalloc_slab() directly? kmalloc_slab() is internal to slab and should not be exported. It'd require exporting because the tests can be built as modules. kmalloc_index() works perfectly fine, and the test really doesn't care about performance of kmalloc_index(). :-) See my suggestion below that builds on Andrew's size_is_constant but would retain the old interface and support testing. Thanks, -- Marco ------ >8 ------ From: Marco Elver <elver@google.com> Subject: [PATCH] kfence: test: fix for "mm, slub: change run-time assertion in kmalloc_index() to compile-time" Signed-off-by: Marco Elver <elver@google.com> --- include/linux/slab.h | 9 +++++++-- mm/kfence/kfence_test.c | 5 +++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/linux/slab.h b/include/linux/slab.h index 27d142564557..7a10bdc4b7a9 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -350,7 +350,8 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) * Note: there's no need to optimize kmalloc_index because it's evaluated * in compile-time. */ -static __always_inline unsigned int kmalloc_index(size_t size) +static __always_inline unsigned int __kmalloc_index(size_t size, + bool size_is_constant) { if (!size) return 0; @@ -386,11 +387,15 @@ static __always_inline unsigned int kmalloc_index(size_t size) if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); + if (size_is_constant) + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); + else + BUG(); /* Will never be reached. Needed because the compiler may complain */ return -1; } +#define kmalloc_index(s) __kmalloc_index(s, true) #endif /* !CONFIG_SLOB */ void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc; diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c index 4acf4251ee04..7f24b9bcb2ec 100644 --- a/mm/kfence/kfence_test.c +++ b/mm/kfence/kfence_test.c @@ -197,7 +197,7 @@ static void test_cache_destroy(void) static inline size_t kmalloc_cache_alignment(size_t size) { - return kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size)]->align; + return kmalloc_caches[kmalloc_type(GFP_KERNEL)][__kmalloc_index(size, false)]->align; } /* Must always inline to match stack trace against caller. */ @@ -267,7 +267,8 @@ static void *test_alloc(struct kunit *test, size_t size, gfp_t gfp, enum allocat if (is_kfence_address(alloc)) { struct page *page = virt_to_head_page(alloc); - struct kmem_cache *s = test_cache ?: kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size)]; + struct kmem_cache *s = test_cache ?: + kmalloc_caches[kmalloc_type(GFP_KERNEL)][__kmalloc_index(size, false)]; /* * Verify that various helpers return the right values
On 5/13/21 12:31 PM, Marco Elver wrote: > On Thu, May 13, 2021 at 10:51AM +0200, Vlastimil Babka wrote: >> On 5/13/21 8:28 AM, Hyeonggon Yoo wrote: >> > On Wed, May 12, 2021 at 08:40:24PM -0700, Andrew Morton wrote: >> >> On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: >> >> > On Wed, May 12, 2021 at 07:52:27PM -0700, Andrew Morton wrote: >> >> > > This explodes in mysterious ways. The patch as I have it is appended, >> >> > > for reference. >> >> > > >> >> > > gcc-10.3.0 allmodconfig. >> >> > > >> >> > > This patch suppresses the error: >> >> >> >> Ah, yes, of course, your patch changes kmalloc_index() to require that >> >> it always is called with a constant `size'. kfence_test doesn't do >> >> that. >> >> >> >> kfence is being a bit naughty here - the other kmalloc_index() callers >> >> only comple up the call after verifying that `size' is a compile-time >> >> constant. >> >> Agreed. > > It's just a test, and performance doesn't matter for it. Sure. But what if there appear more users where it will matter. Those would get better performance out of kmalloc_slab(). > The thing is this function lives in <linux/slab.h>, isn't prefixed with > __ or anything like that, so it really does look like a public function. > >> >> Would something like this work? >> >> I'd prefer if we kept kmalloc_index() for constant sizes only. The broken build >> then warns anyone using it the wrong way that they shouldn't. > > Agreed. Andrew's size_is_constant would do that. Also see my suggestion > below to keep the same interface. > >> Besides, it really >> shouldn't be used outside of slab. > > It's an allocator test. If we want to facilitate testing, it must be > allowed to verify or set up test cases that test boundary conditions > based on internal state. > > In the case of kfence_test it wants: the cache's alignment to create > accesses that fall on alignment boundaries; and to verify obj_to_index() > and objs_per_slab_page() are set up correctly. OK. > I think the requirements are: > > 1. Make the interface hard to abuse. Adding the BUILD_BUG_ON does that. Yes. > 2. Facilitate testing. Right. >> But if kfence test really needs this, we could perhaps extract the index >> determining part out of kmalloc_slab(). > > That would duplicate kmalloc_index()? I don't see the need, let's keep > things simple. They are already "duplicated". But one is tailored for constant sizes, the other for variable sizes. >> Hmm or I guess the kfence tests could just use kmalloc_slab() directly? > > kmalloc_slab() is internal to slab and should not be exported. So should be kmalloc_index(). However it needs to have the full implementation in a header accessible to all kmalloc() users to work, so it's there, visible to anyone. > It'd > require exporting because the tests can be built as modules. That's true. > kmalloc_index() works perfectly fine, and the test really doesn't care > about performance of kmalloc_index(). :-) OK then. > See my suggestion below that builds on Andrew's size_is_constant but > would retain the old interface and support testing. I can accept that, but please also modify/expand the newly added comment. Now it's *normally* evaluated in compile-time. And there should be warning that anyone calling it with size_is_constant == false should do that only in context where performance (and code bloat, most likely too) doesn't matter, such as unit test. Thanks, Vlastimil > Thanks, > -- Marco > > ------ >8 ------ > > From: Marco Elver <elver@google.com> > Subject: [PATCH] kfence: test: fix for "mm, slub: change run-time assertion in > kmalloc_index() to compile-time" > > Signed-off-by: Marco Elver <elver@google.com> > --- > include/linux/slab.h | 9 +++++++-- > mm/kfence/kfence_test.c | 5 +++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 27d142564557..7a10bdc4b7a9 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -350,7 +350,8 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) > * Note: there's no need to optimize kmalloc_index because it's evaluated > * in compile-time. > */ > -static __always_inline unsigned int kmalloc_index(size_t size) > +static __always_inline unsigned int __kmalloc_index(size_t size, > + bool size_is_constant) > { > if (!size) > return 0; > @@ -386,11 +387,15 @@ static __always_inline unsigned int kmalloc_index(size_t size) > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > > - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > + if (size_is_constant) > + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > + else > + BUG(); > > /* Will never be reached. Needed because the compiler may complain */ > return -1; > } > +#define kmalloc_index(s) __kmalloc_index(s, true) > #endif /* !CONFIG_SLOB */ > > void *__kmalloc(size_t size, gfp_t flags) __assume_kmalloc_alignment __malloc; > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index 4acf4251ee04..7f24b9bcb2ec 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -197,7 +197,7 @@ static void test_cache_destroy(void) > > static inline size_t kmalloc_cache_alignment(size_t size) > { > - return kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size)]->align; > + return kmalloc_caches[kmalloc_type(GFP_KERNEL)][__kmalloc_index(size, false)]->align; > } > > /* Must always inline to match stack trace against caller. */ > @@ -267,7 +267,8 @@ static void *test_alloc(struct kunit *test, size_t size, gfp_t gfp, enum allocat > > if (is_kfence_address(alloc)) { > struct page *page = virt_to_head_page(alloc); > - struct kmem_cache *s = test_cache ?: kmalloc_caches[kmalloc_type(GFP_KERNEL)][kmalloc_index(size)]; > + struct kmem_cache *s = test_cache ?: > + kmalloc_caches[kmalloc_type(GFP_KERNEL)][__kmalloc_index(size, false)]; > > /* > * Verify that various helpers return the right values >
On Thu, May 13, 2021 at 12:31:38PM +0200, Marco Elver wrote: > ------ >8 ------ > > From: Marco Elver <elver@google.com> > Subject: [PATCH] kfence: test: fix for "mm, slub: change run-time assertion in > kmalloc_index() to compile-time" > > Signed-off-by: Marco Elver <elver@google.com> > --- > include/linux/slab.h | 9 +++++++-- > mm/kfence/kfence_test.c | 5 +++-- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 27d142564557..7a10bdc4b7a9 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -350,7 +350,8 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) > * Note: there's no need to optimize kmalloc_index because it's evaluated > * in compile-time. > */ > -static __always_inline unsigned int kmalloc_index(size_t size) > +static __always_inline unsigned int __kmalloc_index(size_t size, > + bool size_is_constant) > { > if (!size) > return 0; > @@ -386,11 +387,15 @@ static __always_inline unsigned int kmalloc_index(size_t size) > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > > - BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > + if (size_is_constant) > + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > + else > + BUG(); what about checking size it on top of kmalloc_index? because by definition of KMALLOC_SHIFT_HIGH, it's not always 25. it can be less than 25. for some situations. below is what I suggested beofre. for just reference: --- include/linux/slab.h.orig 2021-05-12 17:56:54.504738768 +0900 +++ include/linux/slab.h 2021-05-13 15:06:25.724565850 +0900 @@ -346,9 +346,18 @@ static __always_inline enum kmalloc_cach * 1 = 65 .. 96 bytes * 2 = 129 .. 192 bytes * n = 2^(n-1)+1 .. 2^n + * + * Note: there's no need to optimize kmalloc_index because it's evaluated + * in compile-time. */ static __always_inline unsigned int kmalloc_index(size_t size) { + if (__builtin_constant_p(size)) { + BUILD_BUG_ON_MSG(size > KMALLOC_MAX_CACHE_SIZE , "unexpected size in kmalloc_index()"); + } else if (size > KMALLOC_MAX_CACHE_SIZE) { + BUG(); + } + if (!size) return 0; @@ -382,8 +391,6 @@ static __always_inline unsigned int kmal if (size <= 8 * 1024 * 1024) return 23; if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - if (size <= 64 * 1024 * 1024) return 26; - BUG(); /* Will never be reached. Needed because the compiler may complain */ return -1;
On Thu, May 13, 2021 at 01:37:38PM +0200, Vlastimil Babka wrote: > > See my suggestion below that builds on Andrew's size_is_constant but > > would retain the old interface and support testing. > > I can accept that, but please also modify/expand the newly added comment. Now > it's *normally* evaluated in compile-time. And there should be warning that > anyone calling it with size_is_constant == false should do that only in context > where performance (and code bloat, most likely too) doesn't matter, such as unit > test. > > Thanks, Vlastimil > > > Thanks, I completely agree on what Vlastimil said. there should be comment saying that generally you should not use kmalloc_index with size_is_const == true. and the caller MUST guarantee that size_is_const is correct. if not, it would be so confusing.
On Thu, May 13, 2021 at 09:08:29PM +0900, Hyeonggon Yoo wrote: > I completely agree on what Vlastimil said. there should be comment > saying that generally you should not use kmalloc_index with > size_is_const == true. sorry there was human error. what I mean was: size_is_const == false
On Thu, 13 May 2021 at 14:03, Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > On Thu, May 13, 2021 at 12:31:38PM +0200, Marco Elver wrote: [...] > what about checking size it on top of kmalloc_index? because by definition of > KMALLOC_SHIFT_HIGH, it's not always 25. it can be less than 25. for some > situations. > > below is what I suggested beofre. for just reference: This doesn't solve the problem. We want the compiler to complain whenever kmalloc_index() is used with non-constant in normal code. But it should be possible to use it in allocator tests regardless of size. Either that or export kmalloc_slab(), but I think that's worse. I'll send my patch with an updated comment. > --- include/linux/slab.h.orig 2021-05-12 17:56:54.504738768 +0900 > +++ include/linux/slab.h 2021-05-13 15:06:25.724565850 +0900 > @@ -346,9 +346,18 @@ static __always_inline enum kmalloc_cach > * 1 = 65 .. 96 bytes > * 2 = 129 .. 192 bytes > * n = 2^(n-1)+1 .. 2^n > + * > + * Note: there's no need to optimize kmalloc_index because it's evaluated > + * in compile-time. > */ > static __always_inline unsigned int kmalloc_index(size_t size) > { > + if (__builtin_constant_p(size)) { > + BUILD_BUG_ON_MSG(size > KMALLOC_MAX_CACHE_SIZE , "unexpected size in kmalloc_index()"); > + } else if (size > KMALLOC_MAX_CACHE_SIZE) { > + BUG(); > + } > + > if (!size) > return 0; > > @@ -382,8 +391,6 @@ static __always_inline unsigned int kmal > if (size <= 8 * 1024 * 1024) return 23; > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > - if (size <= 64 * 1024 * 1024) return 26; > - BUG(); > > /* Will never be reached. Needed because the compiler may complain */ > return -1;
On Thu, May 13, 2021 at 02:29:13PM +0200, Marco Elver wrote: > On Thu, 13 May 2021 at 14:03, Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote: > > On Thu, May 13, 2021 at 12:31:38PM +0200, Marco Elver wrote: > [...] > > what about checking size it on top of kmalloc_index? because by definition of > > KMALLOC_SHIFT_HIGH, it's not always 25. it can be less than 25. for some > > situations. > > > > below is what I suggested beofre. for just reference: > > This doesn't solve the problem. We want the compiler to complain > whenever kmalloc_index() is used with non-constant in normal code. in the beginning, I thought kmalloc_index is called only from kmalloc. but if kmalloc_index is called from other place, I think it should correctly check its size. that's what kmalloc_index should do. or... should it be solved as another patch?
On Thu, May 13, 2021 at 02:29:13PM +0200, Marco Elver wrote: > This doesn't solve the problem. We want the compiler to complain > whenever kmalloc_index() is used with non-constant in normal code. But > it should be possible to use it in allocator tests regardless of size. > Either that or export kmalloc_slab(), but I think that's worse. I'll > send my patch with an updated comment. to explain in more detail, in include/linux/slab.h: static __always_inline void *kmalloc(size_t size, gfp_t flags) { if (__builtin_constant_p(size)) { #ifndef CONFIG_SLOB unsigned int index; #endif if (size > KMALLOC_MAX_CACHE_SIZE) return kmalloc_large(size, flags); #ifndef CONFIG_SLOB index = kmalloc_index(size); it checks if size is bigger than KMALLOC_MAX_CACHE_SIZE. so kmalloc_index works safely because the size was already checked. and definition of KMALLOC_MAX_CACHE_SIZE is in include/linux/slab.h: #ifdef CONFIG_SLAB #define KMALLOC_SHIFT_HIGH ((MAX_ORDER + PAGE_SHIFT - 1) <= 25 ? \ (MAX_ORDER + PAGE_SHIFT - 1) : 25) #define KMALLOC_SHIFT_MAX KMALLOC_SHIFT_HIGH #ifndef KMALLOC_SHIFT_LOW #define KMALLOC_SHIFT_LOW 5 #endif #endif #ifdef CONFIG_SLUB #define KMALLOC_SHIFT_HIGH (PAGE_SHIFT + 1) #define KMALLOC_SHIFT_MAX (MAX_ORDER + PAGE_SHIFT - 1) #ifndef KMALLOC_SHIFT_LOW #define KMALLOC_SHIFT_LOW 3 #endif #endif #ifdef CONFIG_SLOB #define KMALLOC_SHIFT_HIGH PAGE_SHIFT #define KMALLOC_SHIFT_MAX (MAX_ORDER + PAGE_SHIFT - 1) #ifndef KMALLOC_SHIFT_LOW #define KMALLOC_SHIFT_LOW 3 #endif #endif so if kmalloc_index is called from another place other than kmalloc, it's not safe to assume that the supported size is 32MB. Thanks, Hyeonggon
Hello Vlastimil, recently kbuild-all test bot reported compile error on clang 10.0.1, with defconfig. Nathan Chancellor wrote: > I think this happens because arch_prepare_optimized_kprobe() calls kzalloc() > with a size of MAX_OPTINSN_SIZE, which is > > #define MAX_OPTINSN_SIZE \ > (((unsigned long)optprobe_template_end - \ > (unsigned long)optprobe_template_entry) + \ > MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE) > and the optprobe_template_{end,entry} are not evaluated as constants. > > I am not sure what the solution is. There seem to be a growing list of issues > with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring > LLVM 11 and newer to build the kernel, given this affects a defconfig. > Cheers, > Nathan I think it's because kmalloc compiles successfully when size is constant, and kmalloc_index isn't. so I think compiler seems to be confused. currently if size is non-constant, kmalloc calls dummy function __kmalloc, which always returns NULL. so what about changing kmalloc to do compile-time assertion too, and track all callers that are calling kmalloc with non-constant argument. How do you think? If you think it is the solution, I'll do that work.
On 5/15/21 11:09 PM, Hyeonggon Yoo wrote: > Hello Vlastimil, recently kbuild-all test bot reported compile error on > clang 10.0.1, with defconfig. Hm yes, catching some compiler bug was something that was noted to be possible to happen. > Nathan Chancellor wrote: >> I think this happens because arch_prepare_optimized_kprobe() calls kzalloc() >> with a size of MAX_OPTINSN_SIZE, which is >> >> #define MAX_OPTINSN_SIZE \ >> (((unsigned long)optprobe_template_end - \ >> (unsigned long)optprobe_template_entry) + \ >> MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE) > >> and the optprobe_template_{end,entry} are not evaluated as constants. >> >> I am not sure what the solution is. There seem to be a growing list of issues >> with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring >> LLVM 11 and newer to build the kernel, given this affects a defconfig. >> Cheers, >> Nathan > > > I think it's because kmalloc compiles successfully when size is constant, > and kmalloc_index isn't. so I think compiler seems to be confused. > > currently if size is non-constant, kmalloc calls dummy function __kmalloc, > which always returns NULL. That's a misunderstanding. __kmalloc() is not a dummy function, you probably found only the header declaration. > so what about changing kmalloc to do compile-time assertion too, and track > all callers that are calling kmalloc with non-constant argument. kmalloc() is expected to be called with both constant and non-constant size. __builtin_constant_p() is used to determine which implementation to use. One based on kmalloc_index(), other on __kmalloc(). It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p() as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE seems it could be a "link-time constant". Maybe we could extend Marco Elver's followup patch that uses BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could use BUG() also if the compiler is LLVM < 11 or something. What would be the proper code for this condition? > How do you think? If you think it is the solution, I'll do that work. >
On Sat, May 15, 2021 at 11:24:25PM +0200, Vlastimil Babka wrote: > > That's a misunderstanding. __kmalloc() is not a dummy function, you > probably found only the header declaration. > Sorry, that was totally my misunderstanding. I was reading dummy function in arch/alpha/boot/bootpz.c:415. I wrongly configured the tool. > It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p() > as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE > seems it could be a "link-time constant". That is what I was missing. Thank you for kindly explaining it. > Maybe we could extend Marco Elver's followup patch that uses > BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could > use BUG() also if the compiler is LLVM < 11 or something. What would be > the proper code for this condition? Fixing clang's bug in linux kernel doesn't seem to be a solution. So now I understand why Nathan said we might require LLVM > 11. I thought I should do something to fix it because I sent the patch. but I was misunderstanding a lot. Thank you sincerely for letting me know. Thanks, Hyeonggon
On Sat, May 15, 2021 at 11:24:25PM +0200, Vlastimil Babka wrote: > On 5/15/21 11:09 PM, Hyeonggon Yoo wrote: > > Hello Vlastimil, recently kbuild-all test bot reported compile error on > > clang 10.0.1, with defconfig. > > Hm yes, catching some compiler bug was something that was noted to be > possible to happen. > > > Nathan Chancellor wrote: > >> I think this happens because arch_prepare_optimized_kprobe() calls kzalloc() > >> with a size of MAX_OPTINSN_SIZE, which is > >> > >> #define MAX_OPTINSN_SIZE \ > >> (((unsigned long)optprobe_template_end - \ > >> (unsigned long)optprobe_template_entry) + \ > >> MAX_OPTIMIZED_LENGTH + JMP32_INSN_SIZE) > > > >> and the optprobe_template_{end,entry} are not evaluated as constants. > >> > >> I am not sure what the solution is. There seem to be a growing list of issues > >> with LLVM 10 that were fixed in LLVM 11, which might necessitate requiring > >> LLVM 11 and newer to build the kernel, given this affects a defconfig. > >> Cheers, > >> Nathan > > > > > > I think it's because kmalloc compiles successfully when size is constant, > > and kmalloc_index isn't. so I think compiler seems to be confused. > > > > currently if size is non-constant, kmalloc calls dummy function __kmalloc, > > which always returns NULL. > > That's a misunderstanding. __kmalloc() is not a dummy function, you > probably found only the header declaration. > > > so what about changing kmalloc to do compile-time assertion too, and track > > all callers that are calling kmalloc with non-constant argument. > > kmalloc() is expected to be called with both constant and non-constant > size. __builtin_constant_p() is used to determine which implementation > to use. One based on kmalloc_index(), other on __kmalloc(). > > It appears clang 10.0.1 is mistakenly evaluating __builtin_constant_p() > as true. Probably something to do with LTO, because MAX_OPTINSN_SIZE > seems it could be a "link-time constant". This happens with x86_64 defconfig so LTO is not involved. However, the explanation makes sense, given that the LLVM change I landed on changes the sparse conditional constant propagation pass, which I believe can influence how LLVM handles __builtin_constant_p(). > Maybe we could extend Marco Elver's followup patch that uses > BUILD_BUG_ON vs BUG() depending on size_is_constant parameter. It could > use BUG() also if the compiler is LLVM < 11 or something. What would be > the proper code for this condition? This should work I think: diff --git a/include/linux/slab.h b/include/linux/slab.h index 9d316aac0aba..1b653266f2aa 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - if (size_is_constant) + if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); else BUG();
On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote: > This should work I think: compiled well with clang-10.0.1, clang-11.0.0, and gcc-10.2.0 with x86_64 default config. is the condition CONFIG_CLANG_VERSION > 110000, not including 110000 it self? > diff --git a/include/linux/slab.h b/include/linux/slab.h > index 9d316aac0aba..1b653266f2aa 100644 > --- a/include/linux/slab.h > +++ b/include/linux/slab.h > @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, > if (size <= 16 * 1024 * 1024) return 24; > if (size <= 32 * 1024 * 1024) return 25; > > - if (size_is_constant) > + if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) > BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); > else > BUG();
On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote: > On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote: >> This should work I think: > > compiled well with clang-10.0.1, clang-11.0.0, > and gcc-10.2.0 with x86_64 default config. > > is the condition CONFIG_CLANG_VERSION > 110000, > not including 110000 it self? Ah sorry, that should definitely be >= :( That is what I get for writing an email that late... in reality, it probably won't matter due to the availability of 11.0.1 and 11.1.0 but it should absolutely be changed. I have not given Nick's patch a go yet but would something like this be acceptable? If so, did you want me to send a formal fixup patch or did you want to send a v4? I have no personal preference. >> diff --git a/include/linux/slab.h b/include/linux/slab.h >> index 9d316aac0aba..1b653266f2aa 100644 >> --- a/include/linux/slab.h >> +++ b/include/linux/slab.h >> @@ -413,7 +413,7 @@ static __always_inline unsigned int __kmalloc_index(size_t size, >> if (size <= 16 * 1024 * 1024) return 24; >> if (size <= 32 * 1024 * 1024) return 25; >> >> - if (size_is_constant) >> + if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && size_is_constant) >> BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); >> else >> BUG();
On Mon, May 17, 2021 at 05:43:22PM -0700, Nathan Chancellor wrote: > Ah sorry, that should definitely be >= :( > > That is what I get for writing an email that late... in reality, it probably > won't matter due to the availability of 11.0.1 and 11.1.0 but it should > absolutely be changed. > I have not given Nick's patch a go yet but would something like this be > acceptable? If so, did you want me to send a formal fixup patch or did you > want to send a v4? I have no personal preference. I think fixup patch patch will be better as we can undo it later. I don't think Nick's patch is needed because that code is not related with clang version, and we don't need that code even in clang 10. then is there something I can help for now? thanks, Hyeonggon
On 5/18/21 2:43 AM, Nathan Chancellor wrote: > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote: >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote: >>> This should work I think: >> >> compiled well with clang-10.0.1, clang-11.0.0, >> and gcc-10.2.0 with x86_64 default config. >> >> is the condition CONFIG_CLANG_VERSION > 110000, >> not including 110000 it self? Good spot. > Ah sorry, that should definitely be >= :( > > That is what I get for writing an email that late... in reality, it probably > won't matter due to the availability of 11.0.1 and 11.1.0 but it should > absolutely be changed. > > I have not given Nick's patch a go yet but would something like this be > acceptable? Yes. > If so, did you want me to send a formal fixup patch or did you want > to send a v4? I have no personal preference. At this point a fixup is the usual way. Andrew might squash it to the original patch (also with Marco's fixup) before sending to Linus. >>> diff --git a/include/linux/slab.h b/include/linux/slab.h >>> index 9d316aac0aba..1b653266f2aa 100644 >>> --- a/include/linux/slab.h >>> +++ b/include/linux/slab.h >>> @@ -413,7 +413,7 @@ static __always_inline unsigned int >>> __kmalloc_index(size_t size, >>> if (size <= 16 * 1024 * 1024) return 24; >>> if (size <= 32 * 1024 * 1024) return 25; >>> - if (size_is_constant) >>> + if ((IS_ENABLED(CONFIG_CC_IS_GCC) || CONFIG_CLANG_VERSION > 110000) && >>> size_is_constant) >>> BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); >>> else >>> BUG(); >
On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote: > On 5/18/21 2:43 AM, Nathan Chancellor wrote: > > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote: > >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote: > >>> This should work I think: > >> > >> compiled well with clang-10.0.1, clang-11.0.0, > >> and gcc-10.2.0 with x86_64 default config. > >> > >> is the condition CONFIG_CLANG_VERSION > 110000, > >> not including 110000 it self? > > Good spot. Thanks! > > Ah sorry, that should definitely be >= :( > > > > That is what I get for writing an email that late... in reality, it probably > > won't matter due to the availability of 11.0.1 and 11.1.0 but it should > > absolutely be changed. > > > > I have not given Nick's patch a go yet but would something like this be > > acceptable? > > Yes. You mean Nick's patch to added with Nathan's code? I'm not sure we need this, but will add it if you can accept it. I'll send fixup patch soon. tell me if I can improve anything on it. Thanks, Hyeonggon
On 5/18/21 1:18 PM, Hyeonggon Yoo wrote: > On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote: >> On 5/18/21 2:43 AM, Nathan Chancellor wrote: >> > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote: >> >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote: >> >>> This should work I think: >> >> >> >> compiled well with clang-10.0.1, clang-11.0.0, >> >> and gcc-10.2.0 with x86_64 default config. >> >> >> >> is the condition CONFIG_CLANG_VERSION > 110000, >> >> not including 110000 it self? >> >> Good spot. > > Thanks! > >> > Ah sorry, that should definitely be >= :( >> > >> > That is what I get for writing an email that late... in reality, it probably >> > won't matter due to the availability of 11.0.1 and 11.1.0 but it should >> > absolutely be changed. >> > >> > I have not given Nick's patch a go yet but would something like this be >> > acceptable? >> >> Yes. > > You mean Nick's patch to added with Nathan's code? No, I thought Nathan was asking about his own proposal. I don't think Nick's patch that adds 26 index solves the issue. Nathan's proposal fixed with '>=' is OK. > I'm not sure we need this, but will add it if you can accept it. > > I'll send fixup patch soon. tell me if I can improve > anything on it. > > Thanks, > Hyeonggon >
On Tue, May 18, 2021 at 01:34:07PM +0200, Vlastimil Babka wrote: > On 5/18/21 1:18 PM, Hyeonggon Yoo wrote: > > On Tue, May 18, 2021 at 11:28:17AM +0200, Vlastimil Babka wrote: > >> On 5/18/21 2:43 AM, Nathan Chancellor wrote: > >> > On 5/17/2021 5:38 PM, Hyeonggon Yoo wrote: > >> >> On Sat, May 15, 2021 at 11:34:49PM -0700, Nathan Chancellor wrote: > >> >>> This should work I think: > >> >> > >> >> compiled well with clang-10.0.1, clang-11.0.0, > >> >> and gcc-10.2.0 with x86_64 default config. > >> >> > >> >> is the condition CONFIG_CLANG_VERSION > 110000, > >> >> not including 110000 it self? > >> > >> Good spot. > > > > Thanks! > > > >> > Ah sorry, that should definitely be >= :( > >> > > >> > That is what I get for writing an email that late... in reality, it probably > >> > won't matter due to the availability of 11.0.1 and 11.1.0 but it should > >> > absolutely be changed. > >> > > >> > I have not given Nick's patch a go yet but would something like this be > >> > acceptable? > >> > >> Yes. > > > > You mean Nick's patch to added with Nathan's code? > > No, I thought Nathan was asking about his own proposal. I don't think Nick's > patch that adds 26 index solves the issue. Nathan's proposal fixed with '>=' is OK. Ah, Okay! I sent the patch. Thanks, Hyeonggon
diff --git a/include/linux/slab.h b/include/linux/slab.h index 6d454886bcaf..df1937309df2 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -345,6 +345,9 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags) * 1 = 65 .. 96 bytes * 2 = 129 .. 192 bytes * n = 2^(n-1)+1 .. 2^n + * + * Note: there's no need to optimize kmalloc_index because it's evaluated + * in compile-time. */ static __always_inline unsigned int kmalloc_index(size_t size) { @@ -381,8 +384,8 @@ static __always_inline unsigned int kmalloc_index(size_t size) if (size <= 8 * 1024 * 1024) return 23; if (size <= 16 * 1024 * 1024) return 24; if (size <= 32 * 1024 * 1024) return 25; - if (size <= 64 * 1024 * 1024) return 26; - BUG(); + + BUILD_BUG_ON_MSG(1, "unexpected size in kmalloc_index()"); /* Will never be reached. Needed because the compiler may complain */ return -1; diff --git a/mm/slab_common.c b/mm/slab_common.c index fe8b68482670..97664bbe8147 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1192,8 +1192,8 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags) /* * kmalloc_info[] is to make slub_debug=,kmalloc-xx option work at boot time. - * kmalloc_index() supports up to 2^26=64MB, so the final entry of the table is - * kmalloc-67108864. + * kmalloc_index() supports up to 2^25=32MB, so the final entry of the table is + * kmalloc-32M. */ const struct kmalloc_info_struct kmalloc_info[] __initconst = { INIT_KMALLOC_INFO(0, 0), @@ -1221,8 +1221,7 @@ const struct kmalloc_info_struct kmalloc_info[] __initconst = { INIT_KMALLOC_INFO(4194304, 4M), INIT_KMALLOC_INFO(8388608, 8M), INIT_KMALLOC_INFO(16777216, 16M), - INIT_KMALLOC_INFO(33554432, 32M), - INIT_KMALLOC_INFO(67108864, 64M) + INIT_KMALLOC_INFO(33554432, 32M) }; /*
currently when size is not supported by kmalloc_index, compiler will generate a run-time BUG() while compile-time error is also possible, and better. so changed BUG to BUILD_BUG_ON_MSG to make compile-time check possible. also removed code that allocates more than 32MB because current implementation supports only up to 32MB. Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- include/linux/slab.h | 7 +++++-- mm/slab_common.c | 7 +++---- 2 files changed, 8 insertions(+), 6 deletions(-)