Skip to contentSkip to conversation

mm/slab: fix unbounded recursion in free path with memalloc profiling

PATCH Applied Harry Yoo 2 participants 6 messages Aug 1

Subsystems (2)
lore
Share & tools
lore

Harry Yoo

author
#0 lore raw

cover mm/slab: fix unbounded recursion in free path with memalloc profiling

From: "Harry Yoo" <[email protected]>

This is backport of "mm/slab: fix unbounded recursion in free path with memalloc profiling" series [1] for 7.1.

The last patch has a slight functional difference compared to mainline due to lack of SLAB_ALLOC_* flags. I described those subtlties in the backporting notes in detail.

[1] https://lore.kernel.org/all/[email protected]

Harry Yoo (Oracle) (3):
mm/slab: decouple SLAB_NO_SHEAVES from SLAB_NO_OBJ_EXT lib/alloc_tag: introduce mem_alloc_profiling_permanently_disabled() mm/slab: prevent unbounded recursion in free path with new kmalloc
type

 include/linux/alloc_tag.h |  3 ++
 include/linux/slab.h      | 19 +++++++-
 lib/alloc_tag.c           |  9 ++++
 mm/slab.h                 | 28 ++++++++++-
 mm/slab_common.c          | 13 ++++++
 mm/slub.c                 | 97 +++++++++++++++------------------------
 6 files changed, 104 insertions(+), 65 deletions(-)
Signature · 1 line
2.53.0
author
#1 lore raw

PATCH 1/3 mm/slab: decouple SLAB_NO_SHEAVES from SLAB_NO_OBJ_EXT

From: "Harry Yoo (Oracle)" <[email protected]>

commit 982e31382d9a1a3c8c4e6a13702a53711f4efe9f upstream.

Bootstrap caches are created with SLAB_NO_OBJ_EXT to disallow sheaves and obj_exts.

To allow disabling obj_exts while allowing sheaves, decouple SLAB_NO_SHEAVES from SLAB_NO_OBJ_EXT. Bootstrap caches now have both SLAB_NO_SHEAVES and SLAB_NO_OBJ_EXT.

No functional change intended.

---
 include/linux/slab.h | 13 +++++++++++--
 mm/slub.c            | 10 ++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)
include/linux/slab.h +11 -2
index 1a69255bb87f..fda7e24a762b 100644
@@ -58,10 +58,13 @@ enum _slab_flag_bits {
 #endif
 	_SLAB_OBJECT_POISON,
 	_SLAB_CMPXCHG_DOUBLE,
+#ifdef CONFIG_SLAB_OBJ_EXT
 	_SLAB_NO_OBJ_EXT,
-#if defined(CONFIG_SLAB_OBJ_EXT) && defined(CONFIG_64BIT)
+#ifdef CONFIG_64BIT
 	_SLAB_OBJ_EXT_IN_OBJ,
 #endif
+#endif
+	_SLAB_NO_SHEAVES,
 	_SLAB_FLAGS_LAST_BIT
 };
 
@@ -239,8 +242,14 @@ enum _slab_flag_bits {
 #endif
 #define SLAB_TEMPORARY		SLAB_RECLAIM_ACCOUNT	/* Objects are short-lived */
 
-/* Slab created using create_boot_cache */
+/* Slab caches without obj_exts array */
+#ifdef CONFIG_SLAB_OBJ_EXT
 #define SLAB_NO_OBJ_EXT		__SLAB_FLAG_BIT(_SLAB_NO_OBJ_EXT)
+#else
+#define SLAB_NO_OBJ_EXT		__SLAB_FLAG_UNUSED
+#endif
+
+#define SLAB_NO_SHEAVES		__SLAB_FLAG_BIT(_SLAB_NO_SHEAVES)
 
 #if defined(CONFIG_SLAB_OBJ_EXT) && defined(CONFIG_64BIT)
 #define SLAB_OBJ_EXT_IN_OBJ	__SLAB_FLAG_BIT(_SLAB_OBJ_EXT_IN_OBJ)
mm/slub.c +6 -4
index fa24b985a783..33e64f21ecc1 100644
@@ -7726,12 +7726,12 @@ static unsigned int calculate_sheaf_capacity(struct kmem_cache *s,
 		return 0;
 
 	/*
-	 * Bootstrap caches can't have sheaves for now (SLAB_NO_OBJ_EXT).
+	 * Bootstrap caches can't have sheaves for now (SLAB_NO_SHEAVES).
 	 * SLAB_NOLEAKTRACE caches (e.g., kmemleak's object_cache) must not
 	 * have sheaves to avoid recursion when sheaf allocation triggers
 	 * kmemleak tracking.
 	 */
-	if (s->flags & (SLAB_NO_OBJ_EXT | SLAB_NOLEAKTRACE))
+	if (s->flags & (SLAB_NO_SHEAVES | SLAB_NOLEAKTRACE))
 		return 0;
 
 	/*
@@ -8513,7 +8513,8 @@ void __init kmem_cache_init(void)
 
 	create_boot_cache(kmem_cache_node, "kmem_cache_node",
 			sizeof(struct kmem_cache_node),
-			SLAB_HWCACHE_ALIGN | SLAB_NO_OBJ_EXT, 0, 0);
+			SLAB_HWCACHE_ALIGN | SLAB_NO_SHEAVES | SLAB_NO_OBJ_EXT,
+			0, 0);
 
 	hotplug_node_notifier(slab_memory_callback, SLAB_CALLBACK_PRI);
 
@@ -8523,7 +8524,8 @@ void __init kmem_cache_init(void)
 	create_boot_cache(kmem_cache, "kmem_cache",
 			offsetof(struct kmem_cache, per_node) +
 				nr_node_ids * sizeof(struct kmem_cache_per_node_ptrs),
-			SLAB_HWCACHE_ALIGN | SLAB_NO_OBJ_EXT, 0, 0);
+			SLAB_HWCACHE_ALIGN | SLAB_NO_SHEAVES | SLAB_NO_OBJ_EXT,
+			0, 0);
 
 	kmem_cache = bootstrap(&boot_kmem_cache);
 	kmem_cache_node = bootstrap(&boot_kmem_cache_node);
Signature · 1 line
2.53.0
author
#2 lore raw

PATCH 2/3 lib/alloc_tag: introduce mem_alloc_profiling_permanently_disabled()

In reply to Harry Yoo

From: "Harry Yoo (Oracle)" <[email protected]>

commit a37b0066a10aabf3c968b4566706fb866eaf9a85 upstream.

mem_alloc_profiling_enabled() tells whether memalloc profiling is currently enabled. However, even when this function returns false, it can be enabled later.

However, this is not enough. Some optimizations can be applied only when memalloc profiling is permanently disabled. For example, to skip the creation of KMALLOC_NO_OBJ_EXT caches at boot time, mem_profiling must be set to "never", "0" w/ debugging on, or have been shutdown so that it can no longer be enabled.

Introduce mem_alloc_profiling_permanently_disabled() for this purpose.

---
 include/linux/alloc_tag.h | 3 +++
 lib/alloc_tag.c           | 9 +++++++++
 2 files changed, 12 insertions(+)
include/linux/alloc_tag.h +3 -0
index 02de2ede560f..7e7cdc7612be 100644
@@ -134,6 +134,8 @@ static inline bool mem_alloc_profiling_enabled(void)
 				   &mem_alloc_profiling_key);
 }
 
+bool mem_alloc_profiling_permanently_disabled(void);
+
 static inline struct alloc_tag_counters alloc_tag_read(struct alloc_tag *tag)
 {
 	struct alloc_tag_counters v = { 0, 0 };
@@ -239,6 +241,7 @@ static inline bool alloc_tag_is_inaccurate(struct alloc_tag *tag)
 
 #define DEFINE_ALLOC_TAG(_alloc_tag)
 static inline bool mem_alloc_profiling_enabled(void) { return false; }
+static inline bool mem_alloc_profiling_permanently_disabled(void) { return true; }
 static inline void alloc_tag_add(union codetag_ref *ref, struct alloc_tag *tag,
 				 size_t bytes) {}
 static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes) {}
lib/alloc_tag.c +9 -0
index a9ab88f416b9..e3a923594604 100644
@@ -26,6 +26,15 @@ static bool mem_profiling_support = true;
 static bool mem_profiling_support;
 #endif
 
+/*
+ * Memory allocation profiling is permanently disabled and cannot be enabled.
+ * Must be called after setup_early_mem_profiling().
+ */
+bool mem_alloc_profiling_permanently_disabled(void)
+{
+	return !mem_profiling_support;
+}
+
 static struct codetag_type *alloc_tag_cttype;
 
 #ifdef CONFIG_ARCH_MODULE_NEEDS_WEAK_PER_CPU
Signature · 1 line
2.53.0
author
#3 lore raw

PATCH 3/3 mm/slab: prevent unbounded recursion in free path with new kmalloc type

In reply to Harry Yoo

From: "Harry Yoo (Oracle)" <[email protected]>

commit d9e6a7623938968e3752b67e37eaff097e559a54 upstream.

Commit 280ea9c3154b ("mm/slab: avoid allocating slabobj_ext array from its own slab") avoided recursive allocation of obj_exts from kmalloc caches of the same size, by bumping the obj_exts array's allocation size whenever the array size equals the size of the object being allocated.

However, as reported by Danielle Costantino and Shakeel Butt, even slabs from kmalloc caches of different sizes can form a cycle by allocating obj_exts arrays from each other [1]:

What happened: a KMALLOC_NORMAL slab's obj_exts array (used by allocation profiling / memcg accounting) is itself kmalloc()'d from a KMALLOC_NORMAL cache, so the "slab holds another slab's obj_exts array" relation can form cycles. With sizeof(struct slabobj_ext) == 16 and the host's geometry:

- kmalloc-512 has 64 objects/slab -> array is 64*16 == 1024 bytes,

    served from kmalloc-1k;

- kmalloc-1k has 32 objects/slab -> array is 32*16 == 512 bytes, served from kmalloc-512.

A kmalloc-512 slab and a kmalloc-1k slab therefore hold each other's obj_exts array. Discarding one frees the other's array, which empties and discards that slab, which frees the first's array, and so on:
__free_slab() -> free_slab_obj_exts() -> kfree() -> discard_slab() -> __free_slab() recurses along the cycle until the stack is exhausted.

With memory allocation profiling, this allows unbounded recursion in the free path and led to a stack overflow on a production host in the Meta fleet [1]:

  BUG: TASK stack guard page was hit
  Oops: stack guard page
  RIP: 0010:kfree+0x8/0x5d0
  Call Trace:
   __free_slab+0x66/0xc0
   kfree+0x3f0/0x5d0
   ... ( ~125x __free_slab <-> kfree ) ...
   <kernel driver freeing a resource>
   do_syscall_64

It is proposed [1] to resolve this issue by always serving the obj_exts array allocation from kmalloc caches (or large kmalloc) of sizes larger than the object size. However, as pointed out by Vlastimil Babka [2], this can waste an excessive amount of memory as slabs from large kmalloc sizes (e.g. kmalloc-8k) generally need obj_exts arrays much smaller than the object size.

Therefore, rather than bumping the size, let us take a different approach; disallow formation of cycles between kmalloc types when allocating obj_exts arrays. Currently, all obj_exts arrays are served from normal kmalloc caches. Cycles cannot be created if obj_exts arrays of normal kmalloc caches are served from a special kmalloc type that can never have obj_exts arrays.

To achieve this, create a new kmalloc type called KMALLOC_NO_OBJ_EXT. KMALLOC_NO_OBJ_EXT caches are created with SLAB_NO_OBJ_EXT flag when either 1) memory allocation profiling is not permanently disabled, or 2) kmalloc types with a priority higher than KMALLOC_CGROUP are aliased with KMALLOC_NORMAL.

Sheaf bootstrapping for KMALLOC_NO_OBJ_EXT caches now must be deferred because allocation of a barn can trigger obj_exts array allocation of normal kmalloc caches when the KMALLOC_NO_OBJ_EXT cache for that size is not ready yet. For simplicity, perform bootstrapping of sheaves for all kmalloc caches later.

Introduce a new slab alloc flag, SLAB_ALLOC_NO_OBJ_EXT, to prevent allocation of obj_exts arrays, and let kmalloc_slab() override the type to KMALLOC_NO_OBJ_EXT when specified. Note that kmalloc_type() remains unchanged because kmalloc_flags() bypasses the kmalloc fastpath.

Do not pass SLAB_ALLOC_NO_RECURSE to kmalloc_flags() in alloc_slab_obj_exts() and instead use SLAB_ALLOC_NO_OBJ_EXT only when the objects are allocated from normal kmalloc caches. While this prevents unbounded recursive allocation of obj_exts, it allows KMALLOC_NO_OBJ_EXT caches to have sheaves.

Since sheaf allocations specify SLAB_ALLOC_NO_RECURSE that prevents allocation of both sheaves and obj_exts arrays, the recursion depth is bounded.

obj_exts arrays for non-kmalloc-normal caches can now have a valid tag. Do not call mark_obj_codetag_empty() when freeing an obj_exts array to avoid false warnings. KMALLOC_NO_OBJ_EXT don't need this as they never allocate those arrays.

[[email protected]: Backport notes:
- Fix a minor conflict due to missing partitioned kmalloc caches in 7.1.

- Use __GFP_NO_OBJ_EXT instead of SLAB_ALLOC_NO_OBJ_EXT since slab's internal alloc_flags do not exist in 7.1.

- Since there is no way to distinguish between no obj_exts vs. no recursion in 7.1 due to lack of SLAB_ALLOC_* flags, sheaves for normal kmalloc caches are allocated from kmalloc-no-objext-* unlike upstream.

Also, allocations from kmalloc-no-objext cannot allocate sheaves and may observe slightly higher contention. Only 7.1 has this problem as kmalloc caches don't have sheaves in LTS kernels.

- Apply the __GFP_NO_OBJ_EXT flag to the !allow_spin path in alloc_slab_obj_exts(). ]

---
 include/linux/slab.h |  6 +++
 mm/slab.h            | 28 +++++++++++++-
 mm/slab_common.c     | 13 +++++++
 mm/slub.c            | 87 +++++++++++++++-----------------------------
 4 files changed, 75 insertions(+), 59 deletions(-)
include/linux/slab.h +6 -0
index fda7e24a762b..739f474c5d92 100644
@@ -642,6 +642,9 @@ enum kmalloc_cache_type {
 #endif
 #ifndef CONFIG_MEMCG
 	KMALLOC_CGROUP = KMALLOC_NORMAL,
+#endif
+#ifndef CONFIG_SLAB_OBJ_EXT
+	KMALLOC_NO_OBJ_EXT = KMALLOC_NORMAL,
 #endif
 	KMALLOC_RANDOM_START = KMALLOC_NORMAL,
 	KMALLOC_RANDOM_END = KMALLOC_RANDOM_START + RANDOM_KMALLOC_CACHES_NR,
@@ -655,6 +658,9 @@ enum kmalloc_cache_type {
 #endif
 #ifdef CONFIG_MEMCG
 	KMALLOC_CGROUP,
+#endif
+#ifdef CONFIG_SLAB_OBJ_EXT
+	KMALLOC_NO_OBJ_EXT,
 #endif
 	NR_KMALLOC_TYPES
 };
mm/slab.h +26 -2
index bf2f87acf5e3..50414bd2b919 100644
@@ -365,9 +365,13 @@ static inline struct kmem_cache *
 kmalloc_slab(size_t size, kmem_buckets *b, gfp_t flags, unsigned long caller)
 {
 	unsigned int index;
+	enum kmalloc_cache_type type = kmalloc_type(flags, caller);
+
+	if (flags & __GFP_NO_OBJ_EXT)
+		type = KMALLOC_NO_OBJ_EXT;
 
 	if (!b)
-		b = &kmalloc_caches[kmalloc_type(flags, caller)];
+		b = &kmalloc_caches[type];
 	if (size <= 192)
 		index = kmalloc_size_index[size_index_elem(size)];
 	else
@@ -402,7 +406,8 @@ static inline bool is_kmalloc_normal(struct kmem_cache *s)
 {
 	if (!is_kmalloc_cache(s))
 		return false;
-	return !(s->flags & (SLAB_CACHE_DMA|SLAB_ACCOUNT|SLAB_RECLAIM_ACCOUNT));
+
+	return !(s->flags & (SLAB_CACHE_DMA|SLAB_ACCOUNT|SLAB_RECLAIM_ACCOUNT|SLAB_NO_OBJ_EXT));
 }
 
 bool __kfree_rcu_sheaf(struct kmem_cache *s, void *obj);
@@ -505,6 +510,25 @@ static inline void metadata_access_disable(void)
 	kasan_enable_current();
 }
 
+/*
+ * Return true if KMALLOC_NORMAL caches may need obj_exts arrays.
+ *
+ * Memory allocation profiling requires obj_exts for all caches.
+ * Memcg usually doesn't need them for normal kmalloc caches, but kmalloc types
+ * with a priority higher than KMALLOC_CGROUP can be aliased with KMALLOC_NORMAL.
+ */
+static inline bool need_kmalloc_no_objext(void)
+{
+	if (!mem_alloc_profiling_permanently_disabled())
+		return true;
+
+	if (!mem_cgroup_kmem_disabled() &&
+			(KMALLOC_NORMAL == KMALLOC_RECLAIM))
+		return true;
+
+	return false;
+}
+
 #ifdef CONFIG_SLAB_OBJ_EXT
 
 /*
mm/slab_common.c +13 -0
index 8b661fff5eed..214374233fb8 100644
@@ -843,6 +843,12 @@ EXPORT_SYMBOL(kmalloc_size_roundup);
 #define KMALLOC_RANDOM_NAME(N, sz)
 #endif
 
+#ifdef CONFIG_SLAB_OBJ_EXT
+#define KMALLOC_NO_OBJ_EXT_NAME(sz) .name[KMALLOC_NO_OBJ_EXT] = "kmalloc-no-objext-" #sz,
+#else
+#define KMALLOC_NO_OBJ_EXT_NAME(sz)
+#endif
+
 #define INIT_KMALLOC_INFO(__size, __short_size)			\
 {								\
 	.name[KMALLOC_NORMAL]  = "kmalloc-" #__short_size,	\
@@ -850,6 +856,7 @@ EXPORT_SYMBOL(kmalloc_size_roundup);
 	KMALLOC_CGROUP_NAME(__short_size)			\
 	KMALLOC_DMA_NAME(__short_size)				\
 	KMALLOC_RANDOM_NAME(RANDOM_KMALLOC_CACHES_NR, __short_size)	\
+	KMALLOC_NO_OBJ_EXT_NAME(__short_size)			\
 	.size = __size,						\
 }
 
@@ -957,6 +964,12 @@ new_kmalloc_cache(int idx, enum kmalloc_cache_type type)
 			return;
 		}
 		flags |= SLAB_ACCOUNT;
+	} else if (IS_ENABLED(CONFIG_SLAB_OBJ_EXT) && type == KMALLOC_NO_OBJ_EXT) {
+		if (!need_kmalloc_no_objext()) {
+			kmalloc_caches[type][idx] = kmalloc_caches[KMALLOC_NORMAL][idx];
+			return;
+		}
+		flags |= SLAB_NO_OBJ_EXT | SLAB_NO_MERGE;
 	} else if (IS_ENABLED(CONFIG_ZONE_DMA) && (type == KMALLOC_DMA)) {
 		flags |= SLAB_CACHE_DMA;
 	}
mm/slub.c +30 -57

141 lines not on this page · raw message

Signature · 1 line
2.53.0
author
#4 lore raw

cover mm/slab: fix unbounded recursion in free path with memalloc profiling

In reply to Harry Yoo

Ouch, I did not add "PATCH" prefix by mistake. Not sure if it makes stable maintainers life harder.

Fixes for 6.18 and 6.12 will be submitted over few days as I review and test them.

Quoted 10 lines · Harry Yoo

From: "Harry Yoo" <[email protected]>

This is backport of "mm/slab: fix unbounded recursion in free path with memalloc profiling" series [1] for 7.1.

The last patch has a slight functional difference compared to mainline due to lack of SLAB_ALLOC_* flags. I described those subtlties in the backporting notes in detail.

[1] https://lore.kernel.org/all/[email protected]

Also, I did not include the first patch from [1]
(mm/slab: fix unbounded recursion in free path with memalloc profiling) as it's already queued for 7.1.

Quoted 16 lines

Harry Yoo (Oracle) (3):
mm/slab: decouple SLAB_NO_SHEAVES from SLAB_NO_OBJ_EXT lib/alloc_tag: introduce mem_alloc_profiling_permanently_disabled() mm/slab: prevent unbounded recursion in free path with new kmalloc
type

 include/linux/alloc_tag.h |  3 ++
 include/linux/slab.h      | 19 +++++++-
 lib/alloc_tag.c           |  9 ++++
 mm/slab.h                 | 28 ++++++++++-
 mm/slab_common.c          | 13 ++++++
 mm/slub.c                 | 97 +++++++++++++++------------------------
 6 files changed, 104 insertions(+), 65 deletions(-)
Signature · 1 line
2.53.0
Signature · 2 lines
Cheers,
Harry / Hyeonggon
maintainer Applied
#5 lore raw
In reply to Harry Yoo
Quoted 6 lines · Harry Yoo

This is backport of "mm/slab: fix unbounded recursion in free path with memalloc profiling" series [1] for 7.1.

The last patch has a slight functional difference compared to mainline due to lack of SLAB_ALLOC_* flags. I described those subtlties in the backporting notes in detail.

Queued the series for 7.1, thanks.

Signature · 2 lines
Thanks,
Sasha