On 19.11.21 11:10, Holger Assmann wrote: > From: Sascha Hauer <[email protected]> > > 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 <[email protected]> > Signed-off-by: Holger Assmann <[email protected]> > --- > 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 [email protected] http://lists.infradead.org/mailman/listinfo/barebox
