diff mbox series

[1/1] UBUNTU: SAUCE: Revert "mm/slub: fix a memory leak in sysfs_slab_add()"

Message ID 20200910100406.758051-2-kleber.souza@canonical.com
State New
Headers show
Series Partial fix for mm/slub regression (LP #1895109) | expand

Commit Message

Kleber Souza Sept. 10, 2020, 10:04 a.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1895109

This reverts commit 8d392dd31bd9205986b442528014c6eec3851491 (commit
dde3c6b72a16c2db826f54b2d49bdea26c3534a2 upstream).

It has been found that linux 5.4.0-45 introduced a regression that is
causing oops when starting LVM snapshots (LP #1894780). A fix for this
issue hasn't been found yet, but reverting dde3c6b72a16 "mm/slub: fix a
memory leak in sysfs_slab_add()" at least doesn't crash one of the
affected systems anymore.

Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
---
 mm/slub.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Thadeu Lima de Souza Cascardo Sept. 10, 2020, 10:10 a.m. UTC | #1
Yeah, the memory leak is certainly better than the bug, though I can't see how
that commit would cause the bug in question.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Andy Whitcroft Sept. 10, 2020, 10:21 a.m. UTC | #2
On Thu, Sep 10, 2020 at 12:04:06PM +0200, Kleber Sacilotto de Souza wrote:
> BugLink: https://bugs.launchpad.net/bugs/1895109
> 
> This reverts commit 8d392dd31bd9205986b442528014c6eec3851491 (commit
> dde3c6b72a16c2db826f54b2d49bdea26c3534a2 upstream).
> 
> It has been found that linux 5.4.0-45 introduced a regression that is
> causing oops when starting LVM snapshots (LP #1894780). A fix for this
> issue hasn't been found yet, but reverting dde3c6b72a16 "mm/slub: fix a
> memory leak in sysfs_slab_add()" at least doesn't crash one of the
> affected systems anymore.
> 
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
> ---
>  mm/slub.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 709e31002504..434e4c6aca80 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5804,10 +5804,8 @@ static int sysfs_slab_add(struct kmem_cache *s)
>  
>  	s->kobj.kset = kset;
>  	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
> -	if (err) {
> -		kobject_put(&s->kobj);
> +	if (err)
>  		goto out;
> -	}
>  
>  	err = sysfs_create_group(&s->kobj, &slab_attr_group);
>  	if (err)
> -- 
> 2.25.1


Ugg.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Thadeu Lima de Souza Cascardo Sept. 14, 2020, 11:54 a.m. UTC | #3
On Thu, Sep 10, 2020 at 07:10:06AM -0300, Thadeu Lima de Souza Cascardo wrote:
> Yeah, the memory leak is certainly better than the bug, though I can't see how
> that commit would cause the bug in question.
> 
> Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Okay, so I looked at the bug and it all makes sense now. I am glad we proceeded
with the revert:

So, kobject_put will call the ktype release, which is
mm/slub.c:kmem_cache_release, which calls
mm/slab_common.c:slab_kmem_cache_release, which calls
mm/slub.c:__kmem_cache_release.

After we sysfs_slab_add returns failure, we call __kmem_cache_release again,
which causes the double free.

So, there is still the case of properly fixing this memleak the reverted commit
claims to fix, and also avoiding creating the duplicate object.

Cascardo.
diff mbox series

Patch

diff --git a/mm/slub.c b/mm/slub.c
index 709e31002504..434e4c6aca80 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5804,10 +5804,8 @@  static int sysfs_slab_add(struct kmem_cache *s)
 
 	s->kobj.kset = kset;
 	err = kobject_init_and_add(&s->kobj, &slab_ktype, NULL, "%s", name);
-	if (err) {
-		kobject_put(&s->kobj);
+	if (err)
 		goto out;
-	}
 
 	err = sysfs_create_group(&s->kobj, &slab_attr_group);
 	if (err)