mm: memcg/slab: Prevent recursive kfree() loop
Subsystems (4)
PATCH 1/2 mm: memcg/slab: Prevent recursive kfree() loop
Since the merging of the new slab memory controller in v5.9, the page structure stores a pointer to obj_cgroup pointer array for slab pages. When the slab has no used objects, it can be freed in free_slab() which will call kfree() to free the obj_cgroup pointer array in memcg_alloc_page_obj_cgroups(). If it happens that the obj_cgroup array is the last used object in its slab, that slab may then be freed which may caused kfree() to be called again.
With the right workload, the slab cache may be set up in a way that allows the recursive kfree() calling loop to nest deep enough to cause a kernel stack overflow and panic the system. In fact, we have a reproducer that can cause kernel stack overflow on a s390 system involving kmalloc-rcl-256 and kmalloc-rcl-128 slabs with the following kfree() loop recursively called 74 times:
[ 285.520739] [<000000000ec432fc>] kfree+0x4bc/0x560 [ 285.520740] [<000000000ec43466>] __free_slab+0xc6/0x228 [ 285.520741] [<000000000ec41fc2>] __slab_free+0x3c2/0x3e0 [ 285.520742] [<000000000ec432fc>] kfree+0x4bc/0x560 :
One way to prevent this from happening is to defer the freeing of the obj_cgroup array to a later time like using kfree_rcu() even though we don't really need rcu protection in this case.
The size of rcu_head is just two pointers. The allocated obj_cgroup array should not be less than that. To be safe, however, additional code is added to make sure that this is really the case.
--- mm/memcontrol.c | 9 ++++++++- mm/slab.h | 11 ++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-)
mm/memcontrol.c +8 -1
@@ -2866,10 +2866,17 @@ static struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg)int memcg_alloc_page_obj_cgroups (struct page *page,struct kmem_cache *s,gfp_t gfp,bool new_page) { -unsigned int objects =objs_per_slab_page (s, page); +unsigned int objects;unsigned long memcg_data;void *vec; +/* +* Since kfree_rcu() is used for freeing, we have to make +* sure that the allocated buffer is big enough for rcu_head. +*/ + objects =max (objs_per_slab_page (s, page), + (int )(sizeof (struct rcu_head )/sizeof (void *))); + vec =kcalloc_node (objects,sizeof (struct obj_cgroup *), gfp,page_to_nid (page));if (!vec)
mm/slab.h +10 -1
@@ -242,8 +242,17 @@ int memcg_alloc_page_obj_cgroups(struct page *page, struct kmem_cache *s,static inline void memcg_free_page_obj_cgroups (struct page *page) { -kfree (page_objcgs (page)); +struct { +struct rcu_head rcu; + } *objcgs = (void *)page_objcgs (page); + +/* +* We don't actually need to use rcu to protect objcg pointers. +* kfree_rcu() is used here just to defer the actual freeing to avoid +* a recursive kfree() loop which may lead to kernel stack overflow. +*/ page->memcg_data =0 ; +kfree_rcu (objcgs, rcu); }static inline size_t obj_full_size (struct kmem_cache *s)
Signature · 1 line
2.18.1
PATCH 2/2 mm: memcg/slab: Don't create unfreeable slab
The obj_cgroup array (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocated obj_cgroup array comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (the obj_cgroup array) in the slab.
With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
To avoid the creation of these unfreeable slabs, a check is added to memcg_alloc_page_obj_cgroups() to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.
This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular obj_cgroup array dependency is formed.
--- mm/memcontrol.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
mm/memcontrol.c +13 -1
@@ -2876,12 +2876,24 @@ int memcg_alloc_page_obj_cgroups(struct page *page, struct kmem_cache *s,*/ objects =max (objs_per_slab_page (s, page), (int )(sizeof (struct rcu_head )/sizeof (void *))); - +retry: vec =kcalloc_node (objects,sizeof (struct obj_cgroup *), gfp,page_to_nid (page));if (!vec)return -ENOMEM ; +/* +* The allocated vector should not come from the same slab. +* Otherwise, this slab will never become empty. Double the size +* in this case to make sure that the vector comes from a different +* kmemcache. +*/ +if (unlikely (virt_to_head_page (vec) == page)) { +kfree (vec); + objects *=2 ; + goto retry; + } + memcg_data = (unsigned long ) vec |MEMCG_DATA_OBJCGS ;if (new_page) {/*
Signature · 1 line
2.18.1
Re: [PATCH 2/2] mm: memcg/slab: Don't create unfreeable slab
Quoted 11 lines · Waiman Long
The
obj_cgrouparray (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocatedobj_cgrouparray comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (theobj_cgrouparray) in the slab.With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
Quoted 8 lines
To avoid the creation of these unfreeable slabs, a check is added to
memcg_alloc_page_obj_cgroups()to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular
obj_cgrouparray dependency is formed.
Hm this looks like only a half fix then.
I'm afraid the proper fix is for kmemcg to create own set of caches for the arrays. It would also solve the recursive kfree() issue.
Quoted 37 lines
Fixes 286e04b8ed7a ("mm: memcg/slab: allocate obj_cgroups for non-root slab pages")Signed-off-by Waiman Long <[email protected]>--- mm/memcontrol.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)mm/memcontrol.c +13
-1@@ -2876,12 +2876,24 @@ int memcg_alloc_page_obj_cgroups(struct page *page, struct kmem_cache *s,*/ objects =max (objs_per_slab_page (s, page), (int )(sizeof (struct rcu_head )/sizeof (void *))); - +retry: vec =kcalloc_node (objects,sizeof (struct obj_cgroup *), gfp,page_to_nid (page));if (!vec)return -ENOMEM ; +/* +* The allocated vector should not come from the same slab. +* Otherwise, this slab will never become empty. Double the size +* in this case to make sure that the vector comes from a different +* kmemcache. +*/ +if (unlikely (virt_to_head_page (vec) == page)) { +kfree (vec); + objects *=2 ; + goto retry; + } + memcg_data = (unsigned long ) vec |MEMCG_DATA_OBJCGS ;if (new_page) {/*
Re: [PATCH 2/2] mm: memcg/slab: Don't create unfreeable slab
Quoted 17 lines · Vlastimil Babka
Quoted 16 lines · Waiman Long
The
obj_cgrouparray (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocatedobj_cgrouparray comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (theobj_cgrouparray) in the slab.With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
To avoid the creation of these unfreeable slabs, a check is added to
memcg_alloc_page_obj_cgroups()to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.
Quoted 6 lines
Quoted 3 lines
This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular
obj_cgrouparray dependency is formed.Hm this looks like only a half fix then.
I'm afraid the proper fix is for kmemcg to create own set of caches for the arrays. It would also solve the recursivekfree()issue.
Right, this is a possible solution. However, the objcg pointers array should need that much memory. Creating its own set of kmemcaches may seem like an overkill.
Cheers,
Longman
Re: [PATCH 2/2] mm: memcg/slab: Don't create unfreeable slab
Quoted 21 lines · Waiman Long
Quoted 20 lines · Vlastimil Babka
Quoted 19 lines · Waiman Long
The
obj_cgrouparray (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocatedobj_cgrouparray comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (theobj_cgrouparray) in the slab.With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
To avoid the creation of these unfreeable slabs, a check is added to
memcg_alloc_page_obj_cgroups()to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular
obj_cgrouparray dependency is formed.
Quoted 7 lines
Quoted 3 lines
Hm this looks like only a half fix then.
I'm afraid the proper fix is for kmemcg to create own set of caches for the arrays. It would also solve the recursivekfree()issue.Right, this is a possible solution. However, the objcg pointers array should need that much memory. Creating its own set of kmemcaches may seem like an overkill.
Well if we go that way, there might be additional benefits:
depending of gfp flags, kmalloc() would allocate from:
kmalloc-* caches that never have kmemcg objects, thus can be used for the objcg pointer arrays
kmalloc-cg-* caches that have only kmemcg unreclaimable objects kmalloc-rcl-* and dma-kmalloc-* can stay with on-demand memcg_alloc_page_obj_cgroups()
This way we fully solve the issues that this patchset solves. In addition we get better separation between kmemcg and !kmemcg thus save memory - no allocation of the array as soon as a single object appears in slab. For "kmalloc-8" we now have 8 bytes for the useful data and 8 bytes for the obj_cgroup pointer.
Vlastimil
Quoted 3 lines
Cheers,
Longman
Re: [PATCH 2/2] mm: memcg/slab: Don't create unfreeable slab
Quoted 41 lines · Vlastimil Babka
Quoted 21 lines · Waiman Long
Quoted 20 lines · Vlastimil Babka
Quoted 19 lines · Waiman Long
The
obj_cgrouparray (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocatedobj_cgrouparray comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (theobj_cgrouparray) in the slab.With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
To avoid the creation of these unfreeable slabs, a check is added to
memcg_alloc_page_obj_cgroups()to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular
obj_cgrouparray dependency is formed.Quoted 7 lines
Quoted 3 lines
Hm this looks like only a half fix then.
I'm afraid the proper fix is for kmemcg to create own set of caches for the arrays. It would also solve the recursivekfree()issue.Right, this is a possible solution. However, the objcg pointers array should need that much memory. Creating its own set of kmemcaches may seem like an overkill.
Well if we go that way, there might be additional benefits:
depending of gfp flags,
kmalloc()would allocate from:kmalloc-* caches that never have kmemcg objects, thus can be used for the objcg pointer arrays
kmalloc-cg-* caches that have only kmemcg unreclaimable objects kmalloc-rcl-* and dma-kmalloc-* can stay with on-demandmemcg_alloc_page_obj_cgroups()
Quoted 5 lines
This way we fully solve the issues that this patchset solves. In addition we get better separation between kmemcg and !kmemcg thus save memory - no allocation of the array as soon as a single object appears in slab. For "kmalloc-8" we now have 8 bytes for the useful data and 8 bytes for the
obj_cgrouppointer.
Yes this seems like a better approach.
Re: [PATCH 2/2] mm: memcg/slab: Don't create unfreeable slab
Quoted 39 lines · Shakeel Butt
Quoted 38 lines · Vlastimil Babka
Quoted 21 lines · Waiman Long
Quoted 20 lines · Vlastimil Babka
Quoted 19 lines · Waiman Long
The
obj_cgrouparray (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocatedobj_cgrouparray comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (theobj_cgrouparray) in the slab.With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
To avoid the creation of these unfreeable slabs, a check is added to
memcg_alloc_page_obj_cgroups()to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular
obj_cgrouparray dependency is formed.Quoted 6 lines
Quoted 3 lines
Hm this looks like only a half fix then.
I'm afraid the proper fix is for kmemcg to create own set of caches for the arrays. It would also solve the recursivekfree()issue.Right, this is a possible solution. However, the objcg pointers array should need that much memory. Creating its own set of kmemcaches may seem like an overkill.
Well if we go that way, there might be additional benefits:
depending of gfp flags,
kmalloc()would allocate from:kmalloc-* caches that never have kmemcg objects, thus can be used for the objcg pointer arrays
kmalloc-cg-* caches that have only kmemcg unreclaimable objects kmalloc-rcl-* and dma-kmalloc-* can stay with on-demandmemcg_alloc_page_obj_cgroups()
Quoted 7 lines
Quoted 5 lines
This way we fully solve the issues that this patchset solves. In addition we get better separation between kmemcg and !kmemcg thus save memory - no allocation of the array as soon as a single object appears in slab. For "kmalloc-8" we now have 8 bytes for the useful data and 8 bytes for the
obj_cgrouppointer.Yes this seems like a better approach.
OK, I will try to go this route then if there is no objection from others.
From slabinfo, the objs/slab numbers range from 4-512. That means we need kmalloc-cg-{32,64,128,256,512,1k,2k,4k}. A init function to set up the new kmemcaches and an allocation function that use the proper kmemcaches to allocate from.
Cheers,
Longman
Re: [PATCH 2/2] mm: memcg/slab: Don't create unfreeable slab
Quoted 45 lines · Waiman Long
Quoted 44 lines · Shakeel Butt
Quoted 43 lines · Vlastimil Babka
Quoted 21 lines · Waiman Long
Quoted 20 lines · Vlastimil Babka
Quoted 19 lines · Waiman Long
The
obj_cgrouparray (memcg_data) embedded in the page structure is allocated at the first instance an accounted memory allocation happens. With the right size object, it is possible that the allocatedobj_cgrouparray comes from the same slab that requires memory accounting. If this happens, the slab will never become empty again as there is at least one object left (theobj_cgrouparray) in the slab.With instructmentation code added to detect this situation, I got 76 hits on the kmalloc-192 slab when booting up a test kernel on a VM. So this can really happen.
To avoid the creation of these unfreeable slabs, a check is added to
memcg_alloc_page_obj_cgroups()to detect that and double the size of the array in case it happens to make sure that it comes from a different kmemcache.This change, however, does not completely eliminate the presence of unfreeable slabs which can still happen if a circular
obj_cgrouparray dependency is formed.Quoted 6 lines
Quoted 3 lines
Hm this looks like only a half fix then.
I'm afraid the proper fix is for kmemcg to create own set of caches for the arrays. It would also solve the recursivekfree()issue.Right, this is a possible solution. However, the objcg pointers array should need that much memory. Creating its own set of kmemcaches may seem like an overkill.
Well if we go that way, there might be additional benefits:
depending of gfp flags,
kmalloc()would allocate from:kmalloc-* caches that never have kmemcg objects, thus can be used for the objcg pointer arrays
kmalloc-cg-* caches that have only kmemcg unreclaimable objects kmalloc-rcl-* and dma-kmalloc-* can stay with on-demandmemcg_alloc_page_obj_cgroups()This way we fully solve the issues that this patchset solves. In addition we get better separation between kmemcg and !kmemcg thus save memory - no allocation of the array as soon as a single object appears in slab. For "kmalloc-8" we now have 8 bytes for the useful data and 8 bytes for the obj_cgroup pointer.
Quoted 5 lines
Quoted 2 lines
Yes this seems like a better approach.
OK, I will try to go this route then if there is no objection from others.
From slabinfo, the objs/slab numbers range from 4-512. That means we need kmalloc-cg-{32,64,128,256,512,1k,2k,4k}. A init function to set up the new kmemcaches and an allocation function that use the proper kmemcaches to allocate from.
I think I had misinterpreted the kmalloc-* setup. In this case, the kmalloc-cg-* should have the same set of sizes as kmalloc-*.
Cheers,
Longman