[PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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

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)
 };
 
 /*
-- 
2.25.1

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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!

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Vlastimil Babka]

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)
>  };
>  
>  /*
> 

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Andrew Morton]

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)
 };
 
 /*
_

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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.

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Andrew Morton]

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
_

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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;

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Marco Elver]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Marco Elver]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Vlastimil Babka]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Marco Elver]

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
-- 
2.31.1.607.g51e8a6a459-goog

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Vlastimil Babka]

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
> 

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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.

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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;

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Marco Elver]

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;

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Marco Elver]

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;

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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?

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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

[PATCH] kfence: test: fix for "mm, slub: change run-time assertion in kmalloc_index() to compile-time"

[Marco Elver]

Enable using kmalloc_index() in allocator test modules again where the
size may be non-constant, while ensuring normal usage always passes a
constant size.

Split the definition into __kmalloc_index(size, size_is_constant), and a
definition of kmalloc_index(s), matching the old kmalloc_index()
interface, but that still requires size_is_constant==true. This ensures
that normal usage of kmalloc_index() always passes a constant size.

While the __-prefix should make it clearer that the function is to be
used with care, also rewrite the "Note" to highlight the restriction
(and add a hint to kmalloc_slab()).

The alternative considered here is to export kmalloc_slab(), but given
it is internal to mm/ and not in <linux/slab.h>, we should probably
avoid exporting it. Allocator test modules will work just fine by using
__kmalloc_index(s, false).

Link: https://lkml.kernel.org/r/20210512195227.245000695c9014242e9a00e5@linux-foundation.org
Signed-off-by: Marco Elver <elver@google.com>
---
 include/linux/slab.h    | 15 +++++++++++----
 mm/kfence/kfence_test.c |  5 +++--
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 27d142564557..465c5d9d9630 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -347,10 +347,13 @@ static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
  * 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.
+ * Note: __kmalloc_index() is compile-time optimized, and not runtime optimized;
+ * typical usage is via kmalloc_index() and therefore evaluated at compile-time.
+ * Callers where !size_is_constant should only be test modules, where runtime
+ * overheads of __kmalloc_index() can be tolerated.  Also see kmalloc_slab().
  */
-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 +389,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
-- 
2.31.1.607.g51e8a6a459-goog

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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.

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Vlastimil Babka]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Nathan Chancellor]

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();

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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();

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Nathan Chancellor]

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();

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Vlastimil Babka]

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();
> 

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Vlastimil Babka]

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
> 

Re: [PATCH v3] mm, slub: change run-time assertion in kmalloc_index() to compile-time

[Hyeonggon Yoo]

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