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 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 <[email protected]>

---
 include/linux/slab.h | 7 +++++--
 mm/slab_common.c     | 7 +++----
 2 files changed, 8 insertions(+), 6 deletions(-)

include/linux/slab.h +5 -2

index 6d454886bcaf..df1937309df2 100644
@@ -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;

mm/slab_common.c +3 -4

index fe8b68482670..97664bbe8147 100644
@@ -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.

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:

mm/kfence/kfence_test.c renamed from mm/kfence/kfence_test.c~a +7 -7

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

Quoted 47 lines · Andrew Morton

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:

mm/kfence/kfence_test.c renamed from mm/kfence/kfence_test.c~a +7 -7

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

Quoted 2 lines

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.

Quoted 7 lines · On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <[email protected]> wrote:

Quoted 6 lines · Andrew Morton

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

include/linux/slab.h renamed from include/linux/slab.h~b +2 -1

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

Quoted 35 lines · Andrew Morton

Quoted 7 lines · On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <[email protected]> wrote:

Quoted 6 lines · Andrew Morton

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

include/linux/slab.h renamed from include/linux/slab.h~b +2 -1

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

include/linux/slab.h static __always_inline unsigned int kmal · line 417

Quoted 8 lines

include/linux/slab.h renamed from include/linux/slab.h~b +4 -1

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

Quoted 10 lines · Andrew Morton

Quoted 7 lines · On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <[email protected]> wrote:

Quoted 6 lines · Andrew Morton

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:

Quoted 7 lines

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.

Quoted 1 line

Would something like this work?

Quoted 17 lines

 include/linux/slab.h    |   12 ++++++++----
 mm/kfence/kfence_test.c |    4 ++--
 2 files changed, 10 insertions(+), 6 deletions(-)

include/linux/slab.h renamed from include/linux/slab.h~b +2 -1

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

include/linux/slab.h static __always_inline unsigned int kmal · line 417

Quoted 8 lines

include/linux/slab.h renamed from include/linux/slab.h~b +4 -1

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

linux/slab.h renamed from linux/slab.h.orig +9 -2

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

Quoted 14 lines · Hyeonggon Yoo

Quoted 13 lines · Andrew Morton

Quoted 7 lines · On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <[email protected]> wrote:

Quoted 6 lines · Andrew Morton

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line

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?

Quoted 15 lines · Vlastimil Babka

Quoted 14 lines · Hyeonggon Yoo

Quoted 13 lines · Andrew Morton

Quoted 7 lines · On Thu, 13 May 2021 12:12:20 +0900 Hyeonggon Yoo <[email protected]> wrote:

Quoted 6 lines · Andrew Morton

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.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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 <[email protected]>
Subject: [PATCH] kfence: test: fix for "mm, slub: change run-time assertion in
kmalloc_index() to compile-time"

Signed-off-by Marco Elver <[email protected]>

---
 include/linux/slab.h    | 9 +++++++--
 mm/kfence/kfence_test.c | 5 +++--
 2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/slab.h +7 -2

index 27d142564557..7a10bdc4b7a9 100644
@@ -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;

mm/kfence/kfence_test.c +3 -2

index 4acf4251ee04..7f24b9bcb2ec 100644
@@ -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

Quoted 2 lines

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

Quoted 1 line

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.

------ >8 ------

From: Marco Elver <[email protected]>
Subject: [PATCH] kfence: test: fix for "mm, slub: change run-time assertion in
kmalloc_index() to compile-time"

Signed-off-by Marco Elver <[email protected]>

---
 include/linux/slab.h    | 9 +++++++--
 mm/kfence/kfence_test.c | 5 +++--
 2 files changed, 10 insertions(+), 4 deletions(-)

include/linux/slab.h +2 -1

index 27d142564557..7a10bdc4b7a9 100644
@@ -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;

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:

linux/slab.h 2021-05-13 15:06:25.724565850 +0900 renamed from linux/slab.h.orig 2021-05-12 17:56:54.504738768 +0900 +9 -2

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

Quoted 1 line · Hyeonggon Yoo

On Thu, May 13, 2021 at 12:31:38PM +0200, Marco Elver wrote:

[...]

Quoted 5 lines

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.

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.

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

Hello Vlastimil, recently kbuild-all test bot reported compile error on clang 10.0.1, with defconfig.

Quoted 8 lines · Nathan Chancellor

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)

Quoted 7 lines

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.

That's a misunderstanding. __kmalloc() is not a dummy function, you probably found only the header declaration.

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?

Quoted 2 lines · Hyeonggon Yoo

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.

Quoted 18 lines

Quoted 8 lines · Nathan Chancellor

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)

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 2 lines

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?

This should work I think:

include/linux/slab.h +1 -1

index 9d316aac0aba..1b653266f2aa 100644
@@ -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();

Quoted 1 line · Nathan Chancellor

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?

Quoted 13 lines

include/linux/slab.h +1 -1

index 9d316aac0aba..1b653266f2aa 100644
@@ -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();

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.

Quoted 8 lines · Hyeonggon Yoo

Quoted 1 line · Nathan Chancellor

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.

Quoted 14 lines

Quoted 14 lines

include/linux/slab.h +1 -0

index 9d316aac0aba..1b653266f2aa 100644
@@ -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();

Quoted 4 lines · Nathan Chancellor

Quoted 3 lines · Hyeonggon Yoo

Quoted 1 line · Nathan Chancellor

This should work I think:

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 5 lines · Vlastimil Babka

Quoted 4 lines · Nathan Chancellor

Quoted 3 lines · Hyeonggon Yoo

Quoted 1 line · Nathan Chancellor

This should work I think:

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

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!

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 25 lines · Hyeonggon Yoo

Quoted 5 lines · Vlastimil Babka

Quoted 4 lines · Nathan Chancellor

Quoted 3 lines · Hyeonggon Yoo

Quoted 1 line · Nathan Chancellor

This should work I think:

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

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!

Quoted 9 lines

Quoted 8 lines

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?

Quoted 3 lines

Quoted 1 line

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.