fix bb_prealloc_list corruption due to wrong group locking

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

(this was on backported ext4 from 2.6.29)

We hit a BUG() in __list_add from ext4_mb_new_inode_pa() because the list head pointed to a removed item:

list_add corruption. next->prev should be ffff81042f2fe158, but was 0000000000200200

(0000000000200200 is LIST_POISON2, set when the item is deleted)

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow is this:

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so its critical that we get the right group number back for this pa->pa_pstart block.

however, ext4_mb_put_pa passes in (pa->pa_pstart - 1) with a comment, "-1 is to protect from crossing allocation group"

Other list-manipulators do not use the "-1" so we have the potential to lock the wrong group and race. Given how the ext4_get_group_no_and_offset() function works, it doesn't seem to me that the subtraction is correct.

Quoted 27 lines · Eric Sandeen

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

(this was on backported ext4 from 2.6.29)

We hit a BUG() in __list_add from ext4_mb_new_inode_pa() because the list head pointed to a removed item:

list_add corruption. next->prev should be ffff81042f2fe158, but was 0000000000200200

(0000000000200200 is LIST_POISON2, set when the item is deleted)

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow is this:

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so its critical that we get the right group number back for this pa->pa_pstart block.

however, ext4_mb_put_pa passes in (pa->pa_pstart - 1) with a comment, "-1 is to protect from crossing allocation group"

Quoted 4 lines

Other list-manipulators do not use the "-1" so we have the potential to lock the wrong group and race. Given how the ext4_get_group_no_and_offset() function works, it doesn't seem to me that the subtraction is correct.

Hm, unless pa_pstart gets advanced to the point where it's in the next group when it's used up... might be more reading to do here.

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

(this was on backported ext4 from 2.6.29)

We hit a BUG() in __list_add from ext4_mb_new_inode_pa() because the list head pointed to a removed item:

list_add corruption. next->prev should be ffff81042f2fe158, but was 0000000000200200

(0000000000200200 is LIST_POISON2, set when the item is deleted)

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow is this:

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so its critical that we get the right group number back for this pa->pa_pstart block.

however, ext4_mb_put_pa passes in (pa->pa_pstart - 1) with a comment, "-1 is to protect from crossing allocation group"

Other list-manipulators do not use the "-1" so we have the potential to lock the wrong group and race. Given how the ext4_get_group_no_and_offset() function works, it doesn't seem to me that the subtraction is correct.

I've not been able to reproduce the bug, so this is by inspection.

Signed-off-by Eric Sandeen <[email protected]>

---

Index: linux-2.6/fs/ext4/mballoc.c
===================================================================

Quoted 49 lines · Eric Sandeen

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

(this was on backported ext4 from 2.6.29)

We hit a BUG() in __list_add from ext4_mb_new_inode_pa() because the list head pointed to a removed item:

list_add corruption. next->prev should be ffff81042f2fe158, but was 0000000000200200

(0000000000200200 is LIST_POISON2, set when the item is deleted)

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow is this:

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so its critical that we get the right group number back for this pa->pa_pstart block.

however, ext4_mb_put_pa passes in (pa->pa_pstart - 1) with a comment, "-1 is to protect from crossing allocation group"

Other list-manipulators do not use the "-1" so we have the potential to lock the wrong group and race. Given how the ext4_get_group_no_and_offset() function works, it doesn't seem to me that the subtraction is correct.

I've not been able to reproduce the bug, so this is by inspection.

Signed-off-by Eric Sandeen <[email protected]>

---

Index: linux-2.6/fs/ext4/mballoc.c
===================================================================

fs/ext4/mballoc.c +1 -2

@@ -3603,8 +3603,7 @@ static void ext4_mb_put_pa(struct ext4_a
 	pa->pa_deleted = 1;
 	spin_unlock(&pa->pa_lock);
 
-	/* -1 is to protect from crossing allocation group */
-	ext4_get_group_no_and_offset(sb, pa->pa_pstart - 1, &grp, NULL);
+	ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
 

fs/ext4/mballoc.c static void ext4_mb_put_pa(struct ext4_a · line 3609

Quoted 3 lines

fs/ext4/mballoc.c +0 -0

 	/*
 	 * possible race:

But the change is needed for lg prealloc space because locality group prealloc reduce pa_pstart on block allocation and once fully allocated pa_pstart can point to the next block group.

But what you found is also
correct for inode prealloc space. I guess the code broke due to FLEX_BG because without FLEX_BG pa_pstart will never be the first block in the group so even for inode prealloc space pa_pstart - 1 would be in the same group.

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow to remove an album is like this:

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so it's critical that we get the right group number back for this prealloc context, to lock the right group (the one associated with this pa) and prevent concurrent list manipulation.

however, ext4_mb_put_pa() passes in (pa->pa_pstart - 1) with a comment, /* -1 is to protect from crossing allocation group */

This makes sense for the group_pa, where pa_pstart is advanced by the length which has been used (in ext4_mb_release_context()), and when the entire length has been used, pa_pstart has been advanced to the first block of the next group.

However, for inode_pa, pa_pstart is never advanced; it's just set once to the first block in the group and not moved after that. So in this case, if we subtract one in ext4_mb_put_pa(), we are actually locking the *previous* group, and opening the race with the other threads which do not subtract off the extra block.

Signed-off-by Eric Sandeen <[email protected]>

---

ndex: linux-2.6/fs/ext4/mballoc.c
===================================================================

fs/ext4/mballoc.c +1 -0

@@ -3589,6 +3589,7 @@ static void ext4_mb_put_pa(struct ext4_a
 			struct super_block *sb, struct ext4_prealloc_space *pa)
 {
 	ext4_group_t grp;
+	ext4_fsblk_t grp_blk;
 
 	if (!atomic_dec_and_test(&pa->pa_count) || pa->pa_free != 0)
 		return;

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow to remove an album is like this:

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so it's critical that we get the right group number back for this prealloc context, to lock the right group (the one associated with this pa) and prevent concurrent list manipulation.

Quoted 7 lines · Eric Sandeen

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa

ext4_lock_group(sb, group) is supposed to protect this list for each group, and a common code flow to remove an album is like this:

Quoted 8 lines

    ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
    ext4_lock_group(sb, grp);
    list_del(&pa->pa_group_list);
    ext4_unlock_group(sb, grp);

so it's critical that we get the right group number back for this prealloc context, to lock the right group (the one associated with this pa) and prevent concurrent list manipulation.

Eric, this may just be coincidence, but is it possible that this may be related to our bitmap problem I described last week? We haven't tracked it down yet but it certainly smells like a race and your fix corrects just such a race in the same code.

The bitmap problem, btw, involves stuff apparently being marked as used when it's really free (or something very much like that), ultimately resulting in double frees.

Hi Frank - I don't *think* so just because deleted items are poisoned and I would expect that we'd trip over a bad pointer in the corrupted list item as the first indicator of trouble... but I could be wrong.

I think you said you could reproduce it, right? So certainly worth testing with this fix I suppose.

Quoted 4 lines · Eric Sandeen

Hi Frank - I don't *think* so just because deleted items are poisoned and I would expect that we'd trip over a bad pointer in the corrupted list item as the first indicator of trouble... but I could be wrong.

Quoted 2 lines

I think you said you could reproduce it, right? So certainly worth testing with this fix I suppose.

Yeah, we're working on doing that now. We're not running with a lot of debugging turned on so we may or may not actually fall over if we try to traverse freed (or just wrong) list entries. If it isn't the same race, though, then there's still one lurking in this code and that would be a hell of a coincidence.

Quoted 5 lines · Frank Mayhar

Quoted 4 lines · Eric Sandeen

Hi Frank - I don't *think* so just because deleted items are poisoned and I would expect that we'd trip over a bad pointer in the corrupted list item as the first indicator of trouble... but I could be wrong.

Quoted 7 lines

Quoted 2 lines

I think you said you could reproduce it, right? So certainly worth testing with this fix I suppose.

Yeah, we're working on doing that now. We're not running with a lot of debugging turned on so we may or may not actually fall over if we try to traverse freed (or just wrong) list entries. If it isn't the same race, though, then there's still one lurking in this code and that would be a hell of a coincidence.

FYI we're running now with this patch in both a test and a production environment and haven't (yet) seen the bitmap inconsistency we were encountering regularly. So far we have over 18 hours of runtime. So, despite the enormous coincidence, our problem appears to be due to the same cause.

Thanks for the fix, Eric!

Good deal; I'll just trust that it's fixed, and maybe at some point I'll sort out how they're related ;)

This is for Red Hat bug 490026,
EXT4 panic, list corruption in ext4_mb_new_inode_pa