linux-kernel - Re: [RFC, PATCH 2/6] jbd: replace potentially false assertion with if block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 8 Mar 2008 09:52:36 -0500
From:	Christoph Hellwig <hch@...radead.org>
To:	Duane Griffin <duaneg@...da.com>
Cc:	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	Theodore Tso <tytso@....edu>, sct@...hat.com,
	akpm@...ux-foundation.org, adilger@...sterfs.com
Subject: Re: [RFC, PATCH 2/6] jbd: replace potentially false assertion with
	if block

On Thu, Mar 06, 2008 at 01:59:10AM +0000, Duane Griffin wrote:
> If an error occurs during jbd cache initialisation it is possible for the
> journal_head_cache to be NULL when journal_destroy_journal_head_cache is
> called. Replace the J_ASSERT with an if block to handle the situation
> correctly.
> 
> Note that even with this fix things will break badly if ext3 is statically
> compiled in and cache initialisation fails.

Actually the whole code surrounding this is far too confusing.  The
patch below converts it much simpler code with proper goto unwinding
for initialization failures:

Index: linux-2.6/fs/jbd/journal.c
===================================================================
--- linux-2.6.orig/fs/jbd/journal.c	2008-03-08 15:43:41.000000000 +0100
+++ linux-2.6/fs/jbd/journal.c	2008-03-08 15:50:44.000000000 +0100
@@ -1615,31 +1615,6 @@ static struct kmem_cache *journal_head_c
 static atomic_t nr_journal_heads = ATOMIC_INIT(0);
 #endif
 
-static int journal_init_journal_head_cache(void)
-{
-	int retval;
-
-	J_ASSERT(journal_head_cache == 0);
-	journal_head_cache = kmem_cache_create("journal_head",
-				sizeof(struct journal_head),
-				0,		/* offset */
-				SLAB_TEMPORARY,	/* flags */
-				NULL);		/* ctor */
-	retval = 0;
-	if (journal_head_cache == 0) {
-		retval = -ENOMEM;
-		printk(KERN_EMERG "JBD: no memory for journal_head cache\n");
-	}
-	return retval;
-}
-
-static void journal_destroy_journal_head_cache(void)
-{
-	J_ASSERT(journal_head_cache != NULL);
-	kmem_cache_destroy(journal_head_cache);
-	journal_head_cache = NULL;
-}
-
 /*
  * journal_head splicing and dicing
  */
@@ -1891,59 +1866,45 @@ static inline void jbd_remove_debugfs_en
 
 struct kmem_cache *jbd_handle_cache;
 
-static int __init journal_init_handle_cache(void)
-{
-	jbd_handle_cache = kmem_cache_create("journal_handle",
-				sizeof(handle_t),
-				0,		/* offset */
-				SLAB_TEMPORARY,	/* flags */
-				NULL);		/* ctor */
-	if (jbd_handle_cache == NULL) {
-		printk(KERN_EMERG "JBD: failed to create handle cache\n");
-		return -ENOMEM;
-	}
-	return 0;
-}
-
-static void journal_destroy_handle_cache(void)
-{
-	if (jbd_handle_cache)
-		kmem_cache_destroy(jbd_handle_cache);
-}
-
 /*
  * Module startup and shutdown
  */
 
-static int __init journal_init_caches(void)
+static int __init journal_init(void)
 {
 	int ret;
 
-	ret = journal_init_revoke_caches();
-	if (ret == 0)
-		ret = journal_init_journal_head_cache();
-	if (ret == 0)
-		ret = journal_init_handle_cache();
-	return ret;
-}
+	BUILD_BUG_ON(sizeof(struct journal_superblock_s) != 1024);
 
-static void journal_destroy_caches(void)
-{
-	journal_destroy_revoke_caches();
-	journal_destroy_journal_head_cache();
-	journal_destroy_handle_cache();
-}
+	ret = journal_init_revoke_caches();
+	if (ret)
+		goto out;
 
-static int __init journal_init(void)
-{
-	int ret;
+	ret = -ENOMEM;
+	journal_head_cache = kmem_cache_create("journal_head",
+				sizeof(struct journal_head),
+				0,		/* offset */
+				SLAB_TEMPORARY,	/* flags */
+				NULL);		/* ctor */
+	if (!journal_head_cache)
+		goto out_destroy_revoke_caches;
 
-	BUILD_BUG_ON(sizeof(struct journal_superblock_s) != 1024);
+	jbd_handle_cache = kmem_cache_create("journal_handle",
+				sizeof(handle_t),
+				0,		/* offset */
+				SLAB_TEMPORARY,	/* flags */
+				NULL);		/* ctor */
+	if (!jbd_handle_cache)
+		goto out_destroy_head_cache;
 
-	ret = journal_init_caches();
-	if (ret != 0)
-		journal_destroy_caches();
 	jbd_create_debugfs_entry();
+	return 0;
+
+ out_destroy_head_cache:
+	kmem_cache_destroy(journal_head_cache);
+ out_destroy_revoke_caches:
+	journal_destroy_revoke_caches();
+ out:
 	return ret;
 }
 
@@ -1955,7 +1916,10 @@ static void __exit journal_exit(void)
 		printk(KERN_EMERG "JBD: leaked %d journal_heads!\n", n);
 #endif
 	jbd_remove_debugfs_entry();
-	journal_destroy_caches();
+
+	kmem_cache_destroy(jbd_handle_cache);
+	kmem_cache_destroy(journal_head_cache);
+	journal_destroy_revoke_caches();
 }
 
 MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/