* [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
@ 2023-09-08 23:06 Rafael Aquini
2023-09-08 23:21 ` Waiman Long
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rafael Aquini @ 2023-09-08 23:06 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, Andrew Morton, Vlastimil Babka, Waiman Long,
Rafael Aquini, stable
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.
Fixes: 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")
Signed-off-by: Rafael Aquini <aquini@redhat.com>
Cc: stable@vger.kernel.org
---
mm/slab_common.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index cd71f9581e67..31e581dc6e85 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -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);
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
2023-09-08 23:06 [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy() Rafael Aquini
@ 2023-09-08 23:21 ` Waiman Long
2023-09-09 1:16 ` Matthew Wilcox
2023-09-11 15:06 ` Vlastimil Babka
2 siblings, 0 replies; 6+ messages in thread
From: Waiman Long @ 2023-09-08 23:21 UTC (permalink / raw)
To: Rafael Aquini, linux-mm
Cc: linux-kernel, Andrew Morton, Vlastimil Babka, Rafael Aquini, stable
On 9/8/23 19:06, Rafael Aquini wrote:
> 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.
>
> Fixes: 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> Cc: stable@vger.kernel.org
> ---
> mm/slab_common.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index cd71f9581e67..31e581dc6e85 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -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);
Thanks for fixing this corner case.
Reviewed-by: Waiman Long <longman@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
2023-09-08 23:06 [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy() Rafael Aquini
2023-09-08 23:21 ` Waiman Long
@ 2023-09-09 1:16 ` Matthew Wilcox
2023-09-09 1:51 ` Rafael Aquini
2023-09-11 15:06 ` Vlastimil Babka
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2023-09-09 1:16 UTC (permalink / raw)
To: Rafael Aquini
Cc: linux-mm, linux-kernel, Andrew Morton, Vlastimil Babka,
Waiman Long, Rafael Aquini, stable
On Fri, Sep 08, 2023 at 07:06:49PM -0400, Rafael Aquini wrote:
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
2023-09-09 1:16 ` Matthew Wilcox
@ 2023-09-09 1:51 ` Rafael Aquini
0 siblings, 0 replies; 6+ messages in thread
From: Rafael Aquini @ 2023-09-09 1:51 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-mm, linux-kernel, Andrew Morton, Vlastimil Babka,
Waiman Long, Rafael Aquini, stable
On Sat, Sep 09, 2023 at 02:16:00AM +0100, Matthew Wilcox wrote:
> On Fri, Sep 08, 2023 at 07:06:49PM -0400, Rafael Aquini wrote:
> > 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
2023-09-08 23:06 [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy() Rafael Aquini
2023-09-08 23:21 ` Waiman Long
2023-09-09 1:16 ` Matthew Wilcox
@ 2023-09-11 15:06 ` Vlastimil Babka
2023-09-11 16:47 ` Rafael Aquini
2 siblings, 1 reply; 6+ messages in thread
From: Vlastimil Babka @ 2023-09-11 15:06 UTC (permalink / raw)
To: Rafael Aquini, linux-mm
Cc: linux-kernel, Andrew Morton, Waiman Long, Rafael Aquini, stable
On 9/9/23 01:06, Rafael Aquini wrote:
> 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.
>
> Fixes: 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")
> Signed-off-by: Rafael Aquini <aquini@redhat.com>
> Cc: stable@vger.kernel.org
Thanks, added to slab.git. Tweaked the code a bit:
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.6/hotfixes&id=46a9ea6681907a3be6b6b0d43776dccc62cad6cf
> ---
> mm/slab_common.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index cd71f9581e67..31e581dc6e85 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy()
2023-09-11 15:06 ` Vlastimil Babka
@ 2023-09-11 16:47 ` Rafael Aquini
0 siblings, 0 replies; 6+ messages in thread
From: Rafael Aquini @ 2023-09-11 16:47 UTC (permalink / raw)
To: Vlastimil Babka
Cc: linux-mm, linux-kernel, Andrew Morton, Waiman Long,
Rafael Aquini, stable
On Mon, Sep 11, 2023 at 05:06:15PM +0200, Vlastimil Babka wrote:
> On 9/9/23 01:06, Rafael Aquini wrote:
> > 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.
> >
> > Fixes: 0495e337b703 ("mm/slab_common: Deleting kobject in kmem_cache_destroy() without holding slab_mutex/cpu_hotplug_lock")
> > Signed-off-by: Rafael Aquini <aquini@redhat.com>
> > Cc: stable@vger.kernel.org
>
> Thanks, added to slab.git. Tweaked the code a bit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/slab.git/commit/?h=slab/for-6.6/hotfixes&id=46a9ea6681907a3be6b6b0d43776dccc62cad6cf
>
Thank you, Vlastimil.
> > ---
> > mm/slab_common.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > index cd71f9581e67..31e581dc6e85 100644
> > --- a/mm/slab_common.c
> > +++ b/mm/slab_common.c
> > @@ -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);
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-11 22:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-08 23:06 [PATCH] mm/slab_common: fix slab_caches list corruption after kmem_cache_destroy() Rafael Aquini
2023-09-08 23:21 ` Waiman Long
2023-09-09 1:16 ` Matthew Wilcox
2023-09-09 1:51 ` Rafael Aquini
2023-09-11 15:06 ` Vlastimil Babka
2023-09-11 16:47 ` Rafael Aquini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.