mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: Holger Assmann <h.assmann@pengutronix.de>, barebox@lists.infradead.org
Subject: Re: [PATCH] fs: jffs2: remove slab cache substitute with malloc
Date: Mon, 22 Nov 2021 09:49:35 +0100	[thread overview]
Message-ID: <e0d5792d-6c06-5567-9b2f-48f70cecd5c4@pengutronix.de> (raw)
In-Reply-To: <20211119101005.715749-1-h.assmann@pengutronix.de>

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


  reply	other threads:[~2021-11-22  8:56 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-19 10:10 Holger Assmann
2021-11-22  8:49 ` Ahmad Fatoum [this message]
2021-11-22  9:26   ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e0d5792d-6c06-5567-9b2f-48f70cecd5c4@pengutronix.de \
    --to=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=h.assmann@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox