linux-ext4 - Re: [PATCH V2] fix bb_prealloc_list corruption due to wrong group locking

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]

Message-ID: <20090316171240.GA16242@skywalker>
Date:	Mon, 16 Mar 2009 22:42:40 +0530
From:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Eric Sandeen <sandeen@...hat.com>
Cc:	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH V2] fix bb_prealloc_list corruption due to wrong group
	locking

On Mon, Mar 16, 2009 at 11:47:37AM -0500, Eric Sandeen wrote:
> 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 <sandeen@...hat.com>
> ---
> 
> ndex: linux-2.6/fs/ext4/mballoc.c
> ===================================================================
> --- linux-2.6.orig/fs/ext4/mballoc.c
> +++ linux-2.6/fs/ext4/mballoc.c
> @@ -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;
> @@ -3603,8 +3604,12 @@ 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);
> +	grp_blk = pa->pa_pstart;
> +	/* If linear, pa_pstart is in the next block group when pa is used up */


	/* If linear, pa_pstart may be in the next block group when pa is used up */
			       ^^^^^^^^^^


> +	if (pa->pa_linear)
> +		grp_blk--;
> +
> +	ext4_get_group_no_and_offset(sb, grp_blk, &grp, NULL);
> 
>  	/*
>  	 * possible race:
> 

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html