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.
 
I've not been able to reproduce the bug, so this is by inspection.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


--
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

Eric Sandeen wrote:
> 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.

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.

-Eric
--
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

Eric Sandeen wrote:
> Eric Sandeen wrote:
>> 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.
> 
> 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.

Ok I think I was on the right track here.  It looks like for group_pa
(with pa_linear == 1), pa->pa_pstart is advanced as it's used (actually
in ext4_mb_release_context(), but that's a detail) so by the time it is
used up, pa->pa_pstart has advanced into the next group* and therefore
subtracting one to find the group it belong(ed) to is correct.

However, for inode_pa (with pa_linear == 0) only pa_free is decremented,
and pa_pstart does not move.  Therefore subtracting one from pa_pstart
in ext4_mb_put_pa is actually grabbing the previous block group's lock
in the inode case, and we open a race with other threads which are
locking the correct group.

I'll do a bit more testing/reading but I think that what we probably
need is something like:

static void ext4_mb_put_pa(struct ext4_allocation_context *ac, ...)
{
...
        /* -1 is to protect from crossing allocation group */
	if (pa->pa_linear)
		pa->pa_pstart--;
        ext4_get_group_no_and_offset(sb, pa->pa_pstart, &grp, NULL);
...

Could probably come up with something clearer, but that's the gist of it.

-Eric


*i.e. if pa_start began at 0, and the group had 512 blocks, when all
blocks are used, pa_start has advanced by 512, and "512" is the first
block in the *next* group, so we need to trim one off in that case.
--
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