diff mbox

[next/mmotm] ext4: fix cache_es after merge_left

Message ID alpine.LSU.2.00.1209271324060.2184@eggly.anvils
State Superseded, archived
Headers show

Commit Message

Hugh Dickins Sept. 27, 2012, 8:31 p.m. UTC 
Kernel build with CONFIG_DEBUG_SLAB or CONFIG_SLUB_DEBUG slub_debug=FPZ
gives me kernel BUG at fs/ext4/extents_status.c:142!  That's the
BUG_ON(es->start + es->len < es->start) in extent_status_end() called
from ext4_es_insert_extent().  tree->cache_es has been freed and poisoned.

This comes from when ext4_es_try_to_merge_left() merges es into leftward
es1, but ext4_es_insert_extent()'s out then updates cache_es to the freed
extent_status.  ext4_es_try_to_merge_right() does not pose a problem.

Change ext4_es_try_to_merge_left() to return whichever extent_status
should be recorded in tree->cache_es.  Remove cache_es update from
both of them, leaving that to ext4_es_insert_extent()'s out label.

Signed-off-by: Hugh Dickins <hughd@google.com>
---
 fs/ext4/extents_status.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Theodore Ts'o Sept. 27, 2012, 8:39 p.m. UTC | #1 
On Thu, Sep 27, 2012 at 01:31:28PM -0700, Hugh Dickins wrote:
> Kernel build with CONFIG_DEBUG_SLAB or CONFIG_SLUB_DEBUG slub_debug=FPZ
> gives me kernel BUG at fs/ext4/extents_status.c:142!  That's the
> BUG_ON(es->start + es->len < es->start) in extent_status_end() called
> from ext4_es_insert_extent().  tree->cache_es has been freed and poisoned.
> 
> This comes from when ext4_es_try_to_merge_left() merges es into leftward
> es1, but ext4_es_insert_extent()'s out then updates cache_es to the freed
> extent_status.  ext4_es_try_to_merge_right() does not pose a problem.
> 
> Change ext4_es_try_to_merge_left() to return whichever extent_status
> should be recorded in tree->cache_es.  Remove cache_es update from
> both of them, leaving that to ext4_es_insert_extent()'s out label.
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>

Hugh, thanks for finding this bug!

Zheng, you were going to send me an updated patch series; can you take
care of merging this patch into your patch series (and crediting Hugh
with a Signed-off-by as appropriate)?

Thanks!!

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zheng Liu Sept. 27, 2012, 10:53 p.m. UTC | #2 
On Thu, Sep 27, 2012 at 04:39:20PM -0400, Theodore Ts'o wrote:
> On Thu, Sep 27, 2012 at 01:31:28PM -0700, Hugh Dickins wrote:
> > Kernel build with CONFIG_DEBUG_SLAB or CONFIG_SLUB_DEBUG slub_debug=FPZ
> > gives me kernel BUG at fs/ext4/extents_status.c:142!  That's the
> > BUG_ON(es->start + es->len < es->start) in extent_status_end() called
> > from ext4_es_insert_extent().  tree->cache_es has been freed and poisoned.
> > 
> > This comes from when ext4_es_try_to_merge_left() merges es into leftward
> > es1, but ext4_es_insert_extent()'s out then updates cache_es to the freed
> > extent_status.  ext4_es_try_to_merge_right() does not pose a problem.
> > 
> > Change ext4_es_try_to_merge_left() to return whichever extent_status
> > should be recorded in tree->cache_es.  Remove cache_es update from
> > both of them, leaving that to ext4_es_insert_extent()'s out label.
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> 
> Hugh, thanks for finding this bug!
> 
> Zheng, you were going to send me an updated patch series; can you take
> care of merging this patch into your patch series (and crediting Hugh
> with a Signed-off-by as appropriate)?

Yeah, thanks for fixing it, although I have fixed this problem in my new
patch set.  I will add a Signed-off-by into the patch.

Regards,
Zheng
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- mmotm/fs/ext4/extents_status.c	2012-09-26 10:15:29.340071552 -0700
+++ linux/fs/ext4/extents_status.c	2012-09-27 11:52:59.284937056 -0700
@@ -244,24 +244,24 @@  static void ext4_es_free_extent(struct e
 	kmem_cache_free(ext4_es_cachep, es);
 }
 
-static void ext4_es_try_to_merge_left(struct ext4_es_tree *tree,
-				      struct extent_status *es)
+static struct extent_status *
+ext4_es_try_to_merge_left(struct ext4_es_tree *tree, struct extent_status *es)
 {
 	struct extent_status *es1;
 	struct rb_node *node;
 
 	node = rb_prev(&es->rb_node);
 	if (!node)
-		return;
+		return es;
 
 	es1 = rb_entry(node, struct extent_status, rb_node);
 	if (es->start == extent_status_end(es1) + 1) {
 		es1->len += es->len;
 		rb_erase(&es->rb_node, &tree->root);
-		if (es == tree->cache_es)
-			tree->cache_es = es1;
 		ext4_es_free_extent(es);
+		es = es1;	/* Caller will update tree->cache_es to this */
 	}
+	return es;
 }
 
 static void ext4_es_try_to_merge_right(struct ext4_es_tree *tree,
@@ -278,9 +278,8 @@  static void ext4_es_try_to_merge_right(s
 	if (es1->start == extent_status_end(es) + 1) {
 		es->len += es1->len;
 		rb_erase(node, &tree->root);
-		if (es1 == tree->cache_es)
-			tree->cache_es = es;
 		ext4_es_free_extent(es1);
+		/* Caller will update tree->cache_es to es */
 	}
 }
 
@@ -318,7 +317,7 @@  int ext4_es_insert_extent(struct inode *
 		es_debug("cached by [%u/%u)\n", es->start, es->len);
 		es->start = offset;
 		es->len += len;
-		ext4_es_try_to_merge_left(tree, es);
+		es = ext4_es_try_to_merge_left(tree, es);
 		goto out;
 	} else if (es && in_range(offset, es->start, es->len)) {
 		es_debug("cached by [%u/%u)\n", es->start, es->len);