mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] fs: jffs2: remove slab cache substitute with malloc
@ 2021-11-19 10:10 Holger Assmann
  2021-11-22  8:49 ` Ahmad Fatoum
  0 siblings, 1 reply; 3+ messages in thread
From: Holger Assmann @ 2021-11-19 10:10 UTC (permalink / raw)
  To: barebox; +Cc: Holger Assmann

From: Sascha Hauer <s.hauer@pengutronix.de>

Function jffs2_create_slab_caches() was called by the probing stage
every time a new jffs2 volume was mounted. This has lead to the memory
allocation pointers for slab caches to become overwritten. As a result
the system crashes at least when trying to unmount more than one volume.

In Barebox, the respective (pseudo) slab caches are designed to work as a
substitute when code gets ported from Linux. They are no real caches, but
function as an interface for malloc and can therefore directly be replaced
by it.

Furthermore, the compressor initialization also suffered from being
called with every probing of a jffs2 volume. We therefore introduce a
variable that counts the amount of jffs2 probing and ensures compressor
init/exit only to happen with the first/last volume being (un)mouted.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
---
 fs/jffs2/fs.c       |  45 +++++++++-------
 fs/jffs2/malloc.c   | 129 +++++++-------------------------------------
 fs/jffs2/nodelist.h |   2 -
 3 files changed, 43 insertions(+), 133 deletions(-)

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index c1d04c397d..a27f67dea3 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -386,6 +386,8 @@ void jffs2_flash_cleanup(struct jffs2_sb_info *c) {
 	}
 }
 
+static int jffs2_probe_cnt;
+
 static int jffs2_probe(struct device_d *dev)
 {
 	struct fs_device_d *fsdev;
@@ -408,28 +410,28 @@ static int jffs2_probe(struct device_d *dev)
 
 	sb->s_fs_info = ctx;
 
-        ret = jffs2_compressors_init();
-        if (ret) {
-		pr_err("error: Failed to initialise compressors\n");
-		goto err_out;
-        }
-
-        ret = jffs2_create_slab_caches();
-        if (ret) {
-		pr_err("error: Failed to initialise slab caches\n");
-		goto err_compressors;
-        }
-
-        if (jffs2_fill_super(fsdev, 0)) {
-		dev_err(dev, "no valid jffs2 found\n");
-		ret = -EINVAL;
-		goto err_slab;
+	if (!jffs2_probe_cnt) {
+		ret = jffs2_compressors_init();
+        	if (ret) {
+			pr_err("error: Failed to initialise compressors\n");
+			goto err_out;
+        	}
+
+        	if (ret) {
+			pr_err("error: Failed to initialise slab caches\n");
+			goto err_compressors;
+        	}
+
+        	if (jffs2_fill_super(fsdev, 0)) {
+			dev_err(dev, "no valid jffs2 found\n");
+			ret = -EINVAL;
+		}
 	}
 
+	jffs2_probe_cnt++;
+
 	return 0;
 
-err_slab:
-        jffs2_destroy_slab_caches();
 err_compressors:
 	jffs2_compressors_exit();
 err_out:
@@ -445,8 +447,11 @@ static void jffs2_remove(struct device_d *dev)
 	fsdev = dev_to_fs_device(dev);
 	sb = &fsdev->sb;
 
-	jffs2_destroy_slab_caches();
-	jffs2_compressors_exit();
+	jffs2_probe_cnt--;
+
+	if (!jffs2_probe_cnt) {
+		jffs2_compressors_exit();
+	}
 
 	jffs2_put_super(sb);
 }
diff --git a/fs/jffs2/malloc.c b/fs/jffs2/malloc.c
index b7afc68cea..869330ea28 100644
--- a/fs/jffs2/malloc.c
+++ b/fs/jffs2/malloc.c
@@ -17,99 +17,6 @@
 #include <linux/jffs2.h>
 #include "nodelist.h"
 
-/* These are initialised to NULL in the kernel startup code.
-   If you're porting to other operating systems, beware */
-static struct kmem_cache *full_dnode_slab;
-static struct kmem_cache *raw_dirent_slab;
-static struct kmem_cache *raw_inode_slab;
-static struct kmem_cache *tmp_dnode_info_slab;
-static struct kmem_cache *raw_node_ref_slab;
-static struct kmem_cache *node_frag_slab;
-static struct kmem_cache *inode_cache_slab;
-#ifdef CONFIG_JFFS2_FS_XATTR
-static struct kmem_cache *xattr_datum_cache;
-static struct kmem_cache *xattr_ref_cache;
-#endif
-
-int __init jffs2_create_slab_caches(void)
-{
-	full_dnode_slab = kmem_cache_create("jffs2_full_dnode",
-					    sizeof(struct jffs2_full_dnode),
-					    0, 0, NULL);
-	if (!full_dnode_slab)
-		goto err;
-
-	raw_dirent_slab = kmem_cache_create("jffs2_raw_dirent",
-					    sizeof(struct jffs2_raw_dirent),
-					    0, SLAB_HWCACHE_ALIGN, NULL);
-	if (!raw_dirent_slab)
-		goto err;
-
-	raw_inode_slab = kmem_cache_create("jffs2_raw_inode",
-					   sizeof(struct jffs2_raw_inode),
-					   0, SLAB_HWCACHE_ALIGN, NULL);
-	if (!raw_inode_slab)
-		goto err;
-
-	tmp_dnode_info_slab = kmem_cache_create("jffs2_tmp_dnode",
-						sizeof(struct jffs2_tmp_dnode_info),
-						0, 0, NULL);
-	if (!tmp_dnode_info_slab)
-		goto err;
-
-	raw_node_ref_slab = kmem_cache_create("jffs2_refblock",
-					      sizeof(struct jffs2_raw_node_ref) * (REFS_PER_BLOCK + 1),
-					      0, 0, NULL);
-	if (!raw_node_ref_slab)
-		goto err;
-
-	node_frag_slab = kmem_cache_create("jffs2_node_frag",
-					   sizeof(struct jffs2_node_frag),
-					   0, 0, NULL);
-	if (!node_frag_slab)
-		goto err;
-
-	inode_cache_slab = kmem_cache_create("jffs2_inode_cache",
-					     sizeof(struct jffs2_inode_cache),
-					     0, 0, NULL);
-	if (!inode_cache_slab)
-		goto err;
-
-#ifdef CONFIG_JFFS2_FS_XATTR
-	xattr_datum_cache = kmem_cache_create("jffs2_xattr_datum",
-					     sizeof(struct jffs2_xattr_datum),
-					     0, 0, NULL);
-	if (!xattr_datum_cache)
-		goto err;
-
-	xattr_ref_cache = kmem_cache_create("jffs2_xattr_ref",
-					   sizeof(struct jffs2_xattr_ref),
-					   0, 0, NULL);
-	if (!xattr_ref_cache)
-		goto err;
-#endif
-
-	return 0;
- err:
-	jffs2_destroy_slab_caches();
-	return -ENOMEM;
-}
-
-void jffs2_destroy_slab_caches(void)
-{
-	kmem_cache_destroy(full_dnode_slab);
-	kmem_cache_destroy(raw_dirent_slab);
-	kmem_cache_destroy(raw_inode_slab);
-	kmem_cache_destroy(tmp_dnode_info_slab);
-	kmem_cache_destroy(raw_node_ref_slab);
-	kmem_cache_destroy(node_frag_slab);
-	kmem_cache_destroy(inode_cache_slab);
-#ifdef CONFIG_JFFS2_FS_XATTR
-	kmem_cache_destroy(xattr_datum_cache);
-	kmem_cache_destroy(xattr_ref_cache);
-#endif
-}
-
 struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)
 {
 	struct jffs2_full_dirent *ret;
@@ -127,7 +34,7 @@ void jffs2_free_full_dirent(struct jffs2_full_dirent *x)
 struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
 {
 	struct jffs2_full_dnode *ret;
-	ret = kmem_cache_alloc(full_dnode_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_full_dnode));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -135,13 +42,13 @@ struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
 void jffs2_free_full_dnode(struct jffs2_full_dnode *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(full_dnode_slab, x);
+	free(x);
 }
 
 struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
 {
 	struct jffs2_raw_dirent *ret;
-	ret = kmem_cache_alloc(raw_dirent_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_raw_dirent));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -149,13 +56,13 @@ struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
 void jffs2_free_raw_dirent(struct jffs2_raw_dirent *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(raw_dirent_slab, x);
+	free(x);
 }
 
 struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
 {
 	struct jffs2_raw_inode *ret;
-	ret = kmem_cache_alloc(raw_inode_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_raw_inode));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -163,13 +70,13 @@ struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
 void jffs2_free_raw_inode(struct jffs2_raw_inode *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(raw_inode_slab, x);
+	free(x);
 }
 
 struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
 {
 	struct jffs2_tmp_dnode_info *ret;
-	ret = kmem_cache_alloc(tmp_dnode_info_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_tmp_dnode_info));
 	dbg_memalloc("%p\n",
 		ret);
 	return ret;
@@ -178,14 +85,14 @@ struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
 void jffs2_free_tmp_dnode_info(struct jffs2_tmp_dnode_info *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(tmp_dnode_info_slab, x);
+	free(x);
 }
 
 static struct jffs2_raw_node_ref *jffs2_alloc_refblock(void)
 {
 	struct jffs2_raw_node_ref *ret;
 
-	ret = kmem_cache_alloc(raw_node_ref_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_raw_node_ref) * (REFS_PER_BLOCK + 1));
 	if (ret) {
 		int i = 0;
 		for (i=0; i < REFS_PER_BLOCK; i++) {
@@ -242,13 +149,13 @@ int jffs2_prealloc_raw_node_refs(struct jffs2_sb_info *c,
 void jffs2_free_refblock(struct jffs2_raw_node_ref *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(raw_node_ref_slab, x);
+	free(x);
 }
 
 struct jffs2_node_frag *jffs2_alloc_node_frag(void)
 {
 	struct jffs2_node_frag *ret;
-	ret = kmem_cache_alloc(node_frag_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_node_frag));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -256,13 +163,13 @@ struct jffs2_node_frag *jffs2_alloc_node_frag(void)
 void jffs2_free_node_frag(struct jffs2_node_frag *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(node_frag_slab, x);
+	free(x);
 }
 
 struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
 {
 	struct jffs2_inode_cache *ret;
-	ret = kmem_cache_alloc(inode_cache_slab, GFP_KERNEL);
+	ret = malloc(sizeof(struct jffs2_inode_cache));
 	dbg_memalloc("%p\n", ret);
 	return ret;
 }
@@ -270,14 +177,14 @@ struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
 void jffs2_free_inode_cache(struct jffs2_inode_cache *x)
 {
 	dbg_memalloc("%p\n", x);
-	kmem_cache_free(inode_cache_slab, x);
+	free(x);
 }
 
 #ifdef CONFIG_JFFS2_FS_XATTR
 struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
 {
 	struct jffs2_xattr_datum *xd;
-	xd = kmem_cache_zalloc(xattr_datum_cache, GFP_KERNEL);
+	xd = malloc(sizeof(struct jffs2_xattr_datum));
 	dbg_memalloc("%p\n", xd);
 	if (!xd)
 		return NULL;
@@ -291,13 +198,13 @@ struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
 void jffs2_free_xattr_datum(struct jffs2_xattr_datum *xd)
 {
 	dbg_memalloc("%p\n", xd);
-	kmem_cache_free(xattr_datum_cache, xd);
+	free(xd);
 }
 
 struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
 {
 	struct jffs2_xattr_ref *ref;
-	ref = kmem_cache_zalloc(xattr_ref_cache, GFP_KERNEL);
+	ref = malloc(sizeof(struct jffs2_xattr_ref));
 	dbg_memalloc("%p\n", ref);
 	if (!ref)
 		return NULL;
@@ -310,6 +217,6 @@ struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
 void jffs2_free_xattr_ref(struct jffs2_xattr_ref *ref)
 {
 	dbg_memalloc("%p\n", ref);
-	kmem_cache_free(xattr_ref_cache, ref);
+	free(ref);
 }
 #endif
diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 20deb639f6..7ea18cd2fc 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -441,8 +441,6 @@ int jffs2_do_crccheck_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *i
 void jffs2_do_clear_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f);
 
 /* malloc.c */
-int jffs2_create_slab_caches(void);
-void jffs2_destroy_slab_caches(void);
 
 struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize);
 void jffs2_free_full_dirent(struct jffs2_full_dirent *);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] fs: jffs2: remove slab cache substitute with malloc
  2021-11-19 10:10 [PATCH] fs: jffs2: remove slab cache substitute with malloc Holger Assmann
@ 2021-11-22  8:49 ` Ahmad Fatoum
  2021-11-22  9:26   ` Sascha Hauer
  0 siblings, 1 reply; 3+ messages in thread
From: Ahmad Fatoum @ 2021-11-22  8:49 UTC (permalink / raw)
  To: Holger Assmann, barebox

On 19.11.21 11:10, Holger Assmann wrote:
> From: Sascha Hauer <s.hauer@pengutronix.de>
> 
> Function jffs2_create_slab_caches() was called by the probing stage
> every time a new jffs2 volume was mounted. This has lead to the memory
> allocation pointers for slab caches to become overwritten. As a result
> the system crashes at least when trying to unmount more than one volume.

Freeing data still in use is a bug.

> In Barebox, the respective (pseudo) slab caches are designed to work as a
> substitute when code gets ported from Linux. They are no real caches, but
> function as an interface for malloc and can therefore directly be replaced
> by it.

Replacing one API with another is clean up and not really related to the
issue here.

> Furthermore, the compressor initialization also suffered from being
> called with every probing of a jffs2 volume. We therefore introduce a
> variable that counts the amount of jffs2 probing and ensures compressor
> init/exit only to happen with the first/last volume being (un)mouted.

That also sounds like a bug, although the commit message isn't clear
what the ramifications are.

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
> ---
>  fs/jffs2/fs.c       |  45 +++++++++-------
>  fs/jffs2/malloc.c   | 129 +++++++-------------------------------------
>  fs/jffs2/nodelist.h |   2 -
>  3 files changed, 43 insertions(+), 133 deletions(-)
> 
> diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
> index c1d04c397d..a27f67dea3 100644
> --- a/fs/jffs2/fs.c
> +++ b/fs/jffs2/fs.c
> @@ -386,6 +386,8 @@ void jffs2_flash_cleanup(struct jffs2_sb_info *c) {
>  	}
>  }
>  
> +static int jffs2_probe_cnt;
> +
>  static int jffs2_probe(struct device_d *dev)
>  {
>  	struct fs_device_d *fsdev;
> @@ -408,28 +410,28 @@ static int jffs2_probe(struct device_d *dev)
>  
>  	sb->s_fs_info = ctx;
>  
> -        ret = jffs2_compressors_init();
> -        if (ret) {
> -		pr_err("error: Failed to initialise compressors\n");
> -		goto err_out;
> -        }
> -
> -        ret = jffs2_create_slab_caches();
> -        if (ret) {
> -		pr_err("error: Failed to initialise slab caches\n");
> -		goto err_compressors;
> -        }
> -
> -        if (jffs2_fill_super(fsdev, 0)) {
> -		dev_err(dev, "no valid jffs2 found\n");
> -		ret = -EINVAL;
> -		goto err_slab;
> +	if (!jffs2_probe_cnt) {
> +		ret = jffs2_compressors_init();
> +        	if (ret) {
> +			pr_err("error: Failed to initialise compressors\n");
> +			goto err_out;
> +        	}
> +
> +        	if (ret) {

This branch is never entered. You likely meant to remove it.

> +			pr_err("error: Failed to initialise slab caches\n");
> +			goto err_compressors;
> +        	}
> +
> +        	if (jffs2_fill_super(fsdev, 0)) {
> +			dev_err(dev, "no valid jffs2 found\n");
> +			ret = -EINVAL;
> +		}

This is buggy. Why would you call a function taking a fsdev
only on the first ever mount? The next mount would have another fsdev.
By the looks of it, I'd assume reading from a second jffs2 mount
is now broken.

>  	}
>  
> +	jffs2_probe_cnt++;
> +
>  	return 0;
>  
> -err_slab:
> -        jffs2_destroy_slab_caches();
>  err_compressors:
>  	jffs2_compressors_exit();
>  err_out:
> @@ -445,8 +447,11 @@ static void jffs2_remove(struct device_d *dev)
>  	fsdev = dev_to_fs_device(dev);
>  	sb = &fsdev->sb;
>  
> -	jffs2_destroy_slab_caches();
> -	jffs2_compressors_exit();
> +	jffs2_probe_cnt--;
> +
> +	if (!jffs2_probe_cnt) {
> +		jffs2_compressors_exit();
> +	}
>  
>  	jffs2_put_super(sb);
>  }
> diff --git a/fs/jffs2/malloc.c b/fs/jffs2/malloc.c
> index b7afc68cea..869330ea28 100644
> --- a/fs/jffs2/malloc.c
> +++ b/fs/jffs2/malloc.c
> @@ -17,99 +17,6 @@
>  #include <linux/jffs2.h>
>  #include "nodelist.h"
>  
> -/* These are initialised to NULL in the kernel startup code.
> -   If you're porting to other operating systems, beware */
> -static struct kmem_cache *full_dnode_slab;
> -static struct kmem_cache *raw_dirent_slab;
> -static struct kmem_cache *raw_inode_slab;
> -static struct kmem_cache *tmp_dnode_info_slab;
> -static struct kmem_cache *raw_node_ref_slab;
> -static struct kmem_cache *node_frag_slab;
> -static struct kmem_cache *inode_cache_slab;
> -#ifdef CONFIG_JFFS2_FS_XATTR
> -static struct kmem_cache *xattr_datum_cache;
> -static struct kmem_cache *xattr_ref_cache;
> -#endif
> -
> -int __init jffs2_create_slab_caches(void)
> -{
> -	full_dnode_slab = kmem_cache_create("jffs2_full_dnode",
> -					    sizeof(struct jffs2_full_dnode),
> -					    0, 0, NULL);
> -	if (!full_dnode_slab)
> -		goto err;
> -
> -	raw_dirent_slab = kmem_cache_create("jffs2_raw_dirent",
> -					    sizeof(struct jffs2_raw_dirent),
> -					    0, SLAB_HWCACHE_ALIGN, NULL);
> -	if (!raw_dirent_slab)
> -		goto err;
> -
> -	raw_inode_slab = kmem_cache_create("jffs2_raw_inode",
> -					   sizeof(struct jffs2_raw_inode),
> -					   0, SLAB_HWCACHE_ALIGN, NULL);
> -	if (!raw_inode_slab)
> -		goto err;
> -
> -	tmp_dnode_info_slab = kmem_cache_create("jffs2_tmp_dnode",
> -						sizeof(struct jffs2_tmp_dnode_info),
> -						0, 0, NULL);
> -	if (!tmp_dnode_info_slab)
> -		goto err;
> -
> -	raw_node_ref_slab = kmem_cache_create("jffs2_refblock",
> -					      sizeof(struct jffs2_raw_node_ref) * (REFS_PER_BLOCK + 1),
> -					      0, 0, NULL);
> -	if (!raw_node_ref_slab)
> -		goto err;
> -
> -	node_frag_slab = kmem_cache_create("jffs2_node_frag",
> -					   sizeof(struct jffs2_node_frag),
> -					   0, 0, NULL);
> -	if (!node_frag_slab)
> -		goto err;
> -
> -	inode_cache_slab = kmem_cache_create("jffs2_inode_cache",
> -					     sizeof(struct jffs2_inode_cache),
> -					     0, 0, NULL);
> -	if (!inode_cache_slab)
> -		goto err;
> -
> -#ifdef CONFIG_JFFS2_FS_XATTR
> -	xattr_datum_cache = kmem_cache_create("jffs2_xattr_datum",
> -					     sizeof(struct jffs2_xattr_datum),
> -					     0, 0, NULL);
> -	if (!xattr_datum_cache)
> -		goto err;
> -
> -	xattr_ref_cache = kmem_cache_create("jffs2_xattr_ref",
> -					   sizeof(struct jffs2_xattr_ref),
> -					   0, 0, NULL);
> -	if (!xattr_ref_cache)
> -		goto err;
> -#endif
> -
> -	return 0;
> - err:
> -	jffs2_destroy_slab_caches();
> -	return -ENOMEM;
> -}
> -
> -void jffs2_destroy_slab_caches(void)
> -{
> -	kmem_cache_destroy(full_dnode_slab);
> -	kmem_cache_destroy(raw_dirent_slab);
> -	kmem_cache_destroy(raw_inode_slab);
> -	kmem_cache_destroy(tmp_dnode_info_slab);
> -	kmem_cache_destroy(raw_node_ref_slab);
> -	kmem_cache_destroy(node_frag_slab);
> -	kmem_cache_destroy(inode_cache_slab);
> -#ifdef CONFIG_JFFS2_FS_XATTR
> -	kmem_cache_destroy(xattr_datum_cache);
> -	kmem_cache_destroy(xattr_ref_cache);
> -#endif
> -}
> -
>  struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize)
>  {
>  	struct jffs2_full_dirent *ret;
> @@ -127,7 +34,7 @@ void jffs2_free_full_dirent(struct jffs2_full_dirent *x)
>  struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
>  {
>  	struct jffs2_full_dnode *ret;
> -	ret = kmem_cache_alloc(full_dnode_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_full_dnode));

Please prefer sizeof(*ret) over hardcoding the size where possible.
This makes it easy to verify that the type is indeed correct.

>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -135,13 +42,13 @@ struct jffs2_full_dnode *jffs2_alloc_full_dnode(void)
>  void jffs2_free_full_dnode(struct jffs2_full_dnode *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(full_dnode_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
>  {
>  	struct jffs2_raw_dirent *ret;
> -	ret = kmem_cache_alloc(raw_dirent_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_raw_dirent));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -149,13 +56,13 @@ struct jffs2_raw_dirent *jffs2_alloc_raw_dirent(void)
>  void jffs2_free_raw_dirent(struct jffs2_raw_dirent *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(raw_dirent_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
>  {
>  	struct jffs2_raw_inode *ret;
> -	ret = kmem_cache_alloc(raw_inode_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_raw_inode));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -163,13 +70,13 @@ struct jffs2_raw_inode *jffs2_alloc_raw_inode(void)
>  void jffs2_free_raw_inode(struct jffs2_raw_inode *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(raw_inode_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
>  {
>  	struct jffs2_tmp_dnode_info *ret;
> -	ret = kmem_cache_alloc(tmp_dnode_info_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_tmp_dnode_info));
>  	dbg_memalloc("%p\n",
>  		ret);
>  	return ret;
> @@ -178,14 +85,14 @@ struct jffs2_tmp_dnode_info *jffs2_alloc_tmp_dnode_info(void)
>  void jffs2_free_tmp_dnode_info(struct jffs2_tmp_dnode_info *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(tmp_dnode_info_slab, x);
> +	free(x);
>  }
>  
>  static struct jffs2_raw_node_ref *jffs2_alloc_refblock(void)
>  {
>  	struct jffs2_raw_node_ref *ret;
>  
> -	ret = kmem_cache_alloc(raw_node_ref_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_raw_node_ref) * (REFS_PER_BLOCK + 1));
>  	if (ret) {
>  		int i = 0;
>  		for (i=0; i < REFS_PER_BLOCK; i++) {
> @@ -242,13 +149,13 @@ int jffs2_prealloc_raw_node_refs(struct jffs2_sb_info *c,
>  void jffs2_free_refblock(struct jffs2_raw_node_ref *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(raw_node_ref_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_node_frag *jffs2_alloc_node_frag(void)
>  {
>  	struct jffs2_node_frag *ret;
> -	ret = kmem_cache_alloc(node_frag_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_node_frag));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -256,13 +163,13 @@ struct jffs2_node_frag *jffs2_alloc_node_frag(void)
>  void jffs2_free_node_frag(struct jffs2_node_frag *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(node_frag_slab, x);
> +	free(x);
>  }
>  
>  struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
>  {
>  	struct jffs2_inode_cache *ret;
> -	ret = kmem_cache_alloc(inode_cache_slab, GFP_KERNEL);
> +	ret = malloc(sizeof(struct jffs2_inode_cache));
>  	dbg_memalloc("%p\n", ret);
>  	return ret;
>  }
> @@ -270,14 +177,14 @@ struct jffs2_inode_cache *jffs2_alloc_inode_cache(void)
>  void jffs2_free_inode_cache(struct jffs2_inode_cache *x)
>  {
>  	dbg_memalloc("%p\n", x);
> -	kmem_cache_free(inode_cache_slab, x);
> +	free(x);
>  }
>  
>  #ifdef CONFIG_JFFS2_FS_XATTR
>  struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
>  {
>  	struct jffs2_xattr_datum *xd;
> -	xd = kmem_cache_zalloc(xattr_datum_cache, GFP_KERNEL);
> +	xd = malloc(sizeof(struct jffs2_xattr_datum));
>  	dbg_memalloc("%p\n", xd);
>  	if (!xd)
>  		return NULL;
> @@ -291,13 +198,13 @@ struct jffs2_xattr_datum *jffs2_alloc_xattr_datum(void)
>  void jffs2_free_xattr_datum(struct jffs2_xattr_datum *xd)
>  {
>  	dbg_memalloc("%p\n", xd);
> -	kmem_cache_free(xattr_datum_cache, xd);
> +	free(xd);
>  }
>  
>  struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
>  {
>  	struct jffs2_xattr_ref *ref;
> -	ref = kmem_cache_zalloc(xattr_ref_cache, GFP_KERNEL);
> +	ref = malloc(sizeof(struct jffs2_xattr_ref));
>  	dbg_memalloc("%p\n", ref);
>  	if (!ref)
>  		return NULL;
> @@ -310,6 +217,6 @@ struct jffs2_xattr_ref *jffs2_alloc_xattr_ref(void)
>  void jffs2_free_xattr_ref(struct jffs2_xattr_ref *ref)
>  {
>  	dbg_memalloc("%p\n", ref);
> -	kmem_cache_free(xattr_ref_cache, ref);
> +	free(ref);
>  }
>  #endif

I think all changes of this file are unrelated to the bug. Could you split this up?

> diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
> index 20deb639f6..7ea18cd2fc 100644
> --- a/fs/jffs2/nodelist.h
> +++ b/fs/jffs2/nodelist.h
> @@ -441,8 +441,6 @@ int jffs2_do_crccheck_inode(struct jffs2_sb_info *c, struct jffs2_inode_cache *i
>  void jffs2_do_clear_inode(struct jffs2_sb_info *c, struct jffs2_inode_info *f);
>  
>  /* malloc.c */
> -int jffs2_create_slab_caches(void);
> -void jffs2_destroy_slab_caches(void);
>  
>  struct jffs2_full_dirent *jffs2_alloc_full_dirent(int namesize);
>  void jffs2_free_full_dirent(struct jffs2_full_dirent *);

Cheers,
Ahmad


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] fs: jffs2: remove slab cache substitute with malloc
  2021-11-22  8:49 ` Ahmad Fatoum
@ 2021-11-22  9:26   ` Sascha Hauer
  0 siblings, 0 replies; 3+ messages in thread
From: Sascha Hauer @ 2021-11-22  9:26 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: Holger Assmann, barebox

On Mon, Nov 22, 2021 at 09:49:35AM +0100, Ahmad Fatoum wrote:
> On 19.11.21 11:10, Holger Assmann wrote:
> > From: Sascha Hauer <s.hauer@pengutronix.de>
> > 
> > Function jffs2_create_slab_caches() was called by the probing stage
> > every time a new jffs2 volume was mounted. This has lead to the memory
> > allocation pointers for slab caches to become overwritten. As a result
> > the system crashes at least when trying to unmount more than one volume.
> 
> Freeing data still in use is a bug.
> 
> > In Barebox, the respective (pseudo) slab caches are designed to work as a
> > substitute when code gets ported from Linux. They are no real caches, but
> > function as an interface for malloc and can therefore directly be replaced
> > by it.
> 
> Replacing one API with another is clean up and not really related to the
> issue here.

The bug in the jffs2 code is that it allocates global data structures in
probe():

	...
        jffs2_compressors_init();
	...
	jffs2_create_slab_caches();
	...

jffs2_create_slab_caches() has several calls to kmem_cache_create()
which does nothing more than allocating the context data structure for
the kmem_cache. Probing a second jffs2 will overwrite the pointers
returned by kmem_cache_create() leading to a double free when it the
two jffs2 fs are unmounted and jffs2_destroy_slab_caches() is called.
Replacing the kmem_caches with pure malloc indeed fixes this as it makes
allocating of kmem_cache context structure unnecessary.

jffs2_compressors_init() has the same problem. We still need this
function though, so the solution here is to introduce a reference
counting so that it is done exactly once in the first probe().

We can introduce the reference counting for both
jffs2_compressors_init() and jffs2_create_slab_caches() in patch 1/2
and replace kmem_cache with malloc in 2/2. Then 2/2 would indeed only be
a cleanup.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-11-22  9:47 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 10:10 [PATCH] fs: jffs2: remove slab cache substitute with malloc Holger Assmann
2021-11-22  8:49 ` Ahmad Fatoum
2021-11-22  9:26   ` Sascha Hauer

mail archive of the barebox mailing list

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lore.barebox.org/barebox/0 barebox/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 barebox barebox/ https://lore.barebox.org/barebox \
		barebox@lists.infradead.org
	public-inbox-index barebox

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git