mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
Subsystems (3)
PATCH 1/1 mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
After the commit in Fixes:, if a module that created a slab cache does not release all of its allocated objects before destroying the cache (at rmmod time), we might end up releasing the kmem_cache object without removing it from the slab_caches list thus corrupting the list as kmem_cache_destroy() ignores the return value from shutdown_cache(), which in turn never removes the kmem_cache object from slabs_list in case __kmem_cache_shutdown() fails to release all of the cache's slabs.
This is easily observable on a kernel built with CONFIG_DEBUG_LIST=y as after that ill release the system will immediately trip on list_add, or list_del, assertions similar to the one shown below as soon as another kmem_cache gets created, or destroyed:
[ 1041.213632] list_del corruption. next->prev should be ffff89f596fb5768, but was 52f1e5016aeee75d. (next=ffff89f595a1b268) [ 1041.219165] ------------[ cut here ]------------ [ 1041.221517] kernel BUG at lib/list_debug.c:62! [ 1041.223452] invalid opcode: 0000 [#1] PREEMPT SMP PTI [ 1041.225408] CPU: 2 PID: 1852 Comm: rmmod Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 1041.228244] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 1041.231212] RIP: 0010:__list_del_entry_valid+0xae/0xb0
Another quick way to trigger this issue, in a kernel with CONFIG_SLUB=y, is to set slub_debug to poison the released objects and then just run cat /proc/slabinfo after removing the module that leaks slab objects, in which case the kernel will panic:
[ 50.954843] general protection fault, probably for non-canonical address 0xa56b6b6b6b6b6b8b: 0000 [#1] PREEMPT SMP PTI [ 50.961545] CPU: 2 PID: 1495 Comm: cat Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 50.966808] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 50.972663] RIP: 0010:get_slabinfo+0x42/0xf0
This patch fixes this issue by properly checking shutdown_cache()'s return value before taking the kmem_cache_release() branch.
--- mm/slab_common.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
mm/slab_common.c +8 -5
@@ -479,7 +479,7 @@ void slab_kmem_cache_release(struct kmem_cache *s)void kmem_cache_destroy (struct kmem_cache *s) { -int refcnt; +int err;bool rcu_set;if (unlikely (!s) || !kasan_check_byte (s))
@@ -490,17 +490,20 @@ void kmem_cache_destroy(struct kmem_cache *s) rcu_set = s->flags &SLAB_TYPESAFE_BY_RCU ; - refcnt = --s->refcount; -if (refcnt) + s->refcount--; +if (s->refcount) { + err = -EBUSY ; goto out_unlock; + } -WARN (shutdown_cache (s), + err =shutdown_cache (s); +WARN (err,"%s %s: Slab cache still has objects when called from %pS" , __func__, s->name, (void *)_RET_IP_); out_unlock:mutex_unlock (&slab_mutex);cpus_read_unlock (); -if (!refcnt && !rcu_set) +if (!err && !rcu_set)kmem_cache_release (s); }EXPORT_SYMBOL (kmem_cache_destroy);
Signature · 1 line
2.41.0
Quoted 69 lines · Rafael Aquini
After the commit in Fixes:, if a module that created a slab cache does not release all of its allocated objects before destroying the cache (at rmmod time), we might end up releasing the
kmem_cacheobject without removing it from theslab_cacheslist thus corrupting the list askmem_cache_destroy()ignores the return value fromshutdown_cache(), which in turn never removes thekmem_cacheobject fromslabs_listin case__kmem_cache_shutdown()fails to release all of the cache's slabs.This is easily observable on a kernel built with
CONFIG_DEBUG_LIST=yas after that ill release the system will immediately trip onlist_add, orlist_del, assertions similar to the one shown below as soon as anotherkmem_cachegets created, or destroyed:[ 1041.213632] list_del corruption. next->prev should be ffff89f596fb5768, but was 52f1e5016aeee75d. (next=ffff89f595a1b268) [ 1041.219165] ------------[ cut here ]------------ [ 1041.221517] kernel BUG at lib/list_debug.c:62! [ 1041.223452] invalid opcode: 0000 [#1] PREEMPT SMP PTI [ 1041.225408] CPU: 2 PID: 1852 Comm: rmmod Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 1041.228244] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 1041.231212] RIP: 0010:__list_del_entry_valid+0xae/0xb0Another quick way to trigger this issue, in a kernel with
CONFIG_SLUB=y, is to setslub_debugto poison the released objects and then just run cat/proc/slabinfoafter removing the module that leaks slab objects, in which case the kernel will panic:[ 50.954843] general protection fault, probably for non-canonical address 0xa56b6b6b6b6b6b8b: 0000 [#1] PREEMPT SMP PTI [ 50.961545] CPU: 2 PID: 1495 Comm: cat Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 50.966808] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 50.972663] RIP: 0010:get_slabinfo+0x42/0xf0This patch fixes this issue by properly checking shutdown_cache()'s return value before taking the
kmem_cache_release()branch.Fixes 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")Signed-off-by Rafael Aquini <[email protected]>--- mm/slab_common.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)mm/slab_common.c +7
-4@@ -479,7 +479,7 @@ void slab_kmem_cache_release(struct kmem_cache *s)void kmem_cache_destroy (struct kmem_cache *s) { -int refcnt; +int err;bool rcu_set;if (unlikely (!s) || !kasan_check_byte (s))@@ -490,17 +490,20 @@ void kmem_cache_destroy(struct kmem_cache *s) rcu_set = s->flags &SLAB_TYPESAFE_BY_RCU ; - refcnt = --s->refcount; -if (refcnt) + s->refcount--; +if (s->refcount) { + err = -EBUSY ; goto out_unlock; + } - WARN(shutdown_cache(s), + err =shutdown_cache (s); + WARN(err,
mm/slab_common.c void kmem_cache_destroy(struct kmem_cache *s) · line 509
Quoted 10 lines
mm/slab_common.c +1
-1"%s %s: Slab cache still has objects when called from %pS" , __func__, s->name, (void *)_RET_IP_); out_unlock:mutex_unlock (&slab_mutex);cpus_read_unlock (); -if (!refcnt && !rcu_set) +if (!err && !rcu_set)kmem_cache_release (s); }EXPORT_SYMBOL (kmem_cache_destroy);
Thanks for fixing this corner case.
Quoted 2 lines · Rafael Aquini
This patch fixes this issue by properly checking shutdown_cache()'s return value before taking the
kmem_cache_release()branch.
Is this the right way to fix this problem? If the module destroys the slab cache, it's not going to be possible to free any of the objects still allocated from the cache. I feel that we should treat this as implicitly freeing all the objects that were allocated from the cache rather than saying the cache is still busy.
Quoted 10 lines · Matthew Wilcox
Quoted 2 lines · Rafael Aquini
This patch fixes this issue by properly checking shutdown_cache()'s return value before taking the
kmem_cache_release()branch.Is this the right way to fix this problem? If the module destroys the slab cache, it's not going to be possible to free any of the objects still allocated from the cache. I feel that we should treat this as implicitly freeing all the objects that were allocated from the cache rather than saying the cache is still busy.
Leaving the cache with the unfreeable slabs "alone" is how it was historically done, and we have to fix this corner case opened by 0495e337b703 this way to address the corruption on stable releases without changing their established and expected behavior.
I think your proposal for a different behavior upon cache destruction is something we should discuss for future releases, but it is orthogonal to this required follow-up fix.
Quoted 31 lines · Rafael Aquini
After the commit in Fixes:, if a module that created a slab cache does not release all of its allocated objects before destroying the cache (at rmmod time), we might end up releasing the
kmem_cacheobject without removing it from theslab_cacheslist thus corrupting the list askmem_cache_destroy()ignores the return value fromshutdown_cache(), which in turn never removes thekmem_cacheobject fromslabs_listin case__kmem_cache_shutdown()fails to release all of the cache's slabs.This is easily observable on a kernel built with
CONFIG_DEBUG_LIST=yas after that ill release the system will immediately trip onlist_add, orlist_del, assertions similar to the one shown below as soon as anotherkmem_cachegets created, or destroyed:[ 1041.213632] list_del corruption. next->prev should be ffff89f596fb5768, but was 52f1e5016aeee75d. (next=ffff89f595a1b268) [ 1041.219165] ------------[ cut here ]------------ [ 1041.221517] kernel BUG at lib/list_debug.c:62! [ 1041.223452] invalid opcode: 0000 [#1] PREEMPT SMP PTI [ 1041.225408] CPU: 2 PID: 1852 Comm: rmmod Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 1041.228244] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 1041.231212] RIP: 0010:__list_del_entry_valid+0xae/0xb0Another quick way to trigger this issue, in a kernel with
CONFIG_SLUB=y, is to setslub_debugto poison the released objects and then just run cat/proc/slabinfoafter removing the module that leaks slab objects, in which case the kernel will panic:[ 50.954843] general protection fault, probably for non-canonical address 0xa56b6b6b6b6b6b8b: 0000 [#1] PREEMPT SMP PTI [ 50.961545] CPU: 2 PID: 1495 Comm: cat Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 50.966808] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 50.972663] RIP: 0010:get_slabinfo+0x42/0xf0
Quoted 6 lines
This patch fixes this issue by properly checking shutdown_cache()'s return value before taking the
kmem_cache_release()branch.Fixes 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")Signed-off-by Rafael Aquini <[email protected]>
Thanks, added to slab.git. Tweaked the code a bit:
Quoted 42 lines
--- mm/slab_common.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)mm/slab_common.c +8
-5@@ -479,7 +479,7 @@ void slab_kmem_cache_release(struct kmem_cache *s)void kmem_cache_destroy (struct kmem_cache *s) { -int refcnt; +int err;bool rcu_set;if (unlikely (!s) || !kasan_check_byte (s))@@ -490,17 +490,20 @@ void kmem_cache_destroy(struct kmem_cache *s) rcu_set = s->flags &SLAB_TYPESAFE_BY_RCU ; - refcnt = --s->refcount; -if (refcnt) + s->refcount--; +if (s->refcount) { + err = -EBUSY ; goto out_unlock; + } -WARN (shutdown_cache (s), + err =shutdown_cache (s); +WARN (err,"%s %s: Slab cache still has objects when called from %pS" , __func__, s->name, (void *)_RET_IP_); out_unlock:mutex_unlock (&slab_mutex);cpus_read_unlock (); -if (!refcnt && !rcu_set) +if (!err && !rcu_set)kmem_cache_release (s); }EXPORT_SYMBOL (kmem_cache_destroy);
Quoted 35 lines · Vlastimil Babka
Quoted 34 lines · Rafael Aquini
After the commit in Fixes:, if a module that created a slab cache does not release all of its allocated objects before destroying the cache (at rmmod time), we might end up releasing the
kmem_cacheobject without removing it from theslab_cacheslist thus corrupting the list askmem_cache_destroy()ignores the return value fromshutdown_cache(), which in turn never removes thekmem_cacheobject fromslabs_listin case__kmem_cache_shutdown()fails to release all of the cache's slabs.This is easily observable on a kernel built with
CONFIG_DEBUG_LIST=yas after that ill release the system will immediately trip onlist_add, orlist_del, assertions similar to the one shown below as soon as anotherkmem_cachegets created, or destroyed:[ 1041.213632] list_del corruption. next->prev should be ffff89f596fb5768, but was 52f1e5016aeee75d. (next=ffff89f595a1b268) [ 1041.219165] ------------[ cut here ]------------ [ 1041.221517] kernel BUG at lib/list_debug.c:62! [ 1041.223452] invalid opcode: 0000 [#1] PREEMPT SMP PTI [ 1041.225408] CPU: 2 PID: 1852 Comm: rmmod Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 1041.228244] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 1041.231212] RIP: 0010:__list_del_entry_valid+0xae/0xb0Another quick way to trigger this issue, in a kernel with
CONFIG_SLUB=y, is to setslub_debugto poison the released objects and then just run cat/proc/slabinfoafter removing the module that leaks slab objects, in which case the kernel will panic:[ 50.954843] general protection fault, probably for non-canonical address 0xa56b6b6b6b6b6b8b: 0000 [#1] PREEMPT SMP PTI [ 50.961545] CPU: 2 PID: 1495 Comm: cat Kdump: loaded Tainted: G B W OE 6.5.0 #15 [ 50.966808] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS edk2-20230524-3.fc37 05/24/2023 [ 50.972663] RIP: 0010:get_slabinfo+0x42/0xf0This patch fixes this issue by properly checking shutdown_cache()'s return value before taking the
kmem_cache_release()branch.
Quoted 8 lines
Quoted 3 lines
Fixes 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")Signed-off-by Rafael Aquini <[email protected]>Thanks, added to slab.git. Tweaked the code a bit:
Thank you, Vlastimil.
Quoted 44 lines
Quoted 42 lines
--- mm/slab_common.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)mm/slab_common.c +8
-5@@ -479,7 +479,7 @@ void slab_kmem_cache_release(struct kmem_cache *s)void kmem_cache_destroy (struct kmem_cache *s) { -int refcnt; +int err;bool rcu_set;if (unlikely (!s) || !kasan_check_byte (s))@@ -490,17 +490,20 @@ void kmem_cache_destroy(struct kmem_cache *s) rcu_set = s->flags &SLAB_TYPESAFE_BY_RCU ; - refcnt = --s->refcount; -if (refcnt) + s->refcount--; +if (s->refcount) { + err = -EBUSY ; goto out_unlock; + } -WARN (shutdown_cache (s), + err =shutdown_cache (s); +WARN (err,"%s %s: Slab cache still has objects when called from %pS" , __func__, s->name, (void *)_RET_IP_); out_unlock:mutex_unlock (&slab_mutex);cpus_read_unlock (); -if (!refcnt && !rcu_set) +if (!err && !rcu_set)kmem_cache_release (s); }EXPORT_SYMBOL (kmem_cache_destroy);