linux-ext4 - Re: [PATCH] ext4: fix sleep inside spinlock issue aka #14739

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: <20091210161553.GG26516@atrey.karlin.mff.cuni.cz>
Date:	Thu, 10 Dec 2009 17:15:53 +0100
From:	Jan Kara <jack@...e.cz>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
Cc:	linux-ext4@...r.kernel.org, cmm@...ibm.com
Subject: Re: [PATCH] ext4: fix sleep inside spinlock issue aka #14739

> drop i_block_reservation_lock before vfs_dq_reserve_block().
> this patch fix http://bugzilla.kernel.org/show_bug.cgi?id=14739
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
  Sorry if someone already refused this (I didn't follow the previous
discussion too closely) but: Looking at the code I see no reason why
ext4_claim_free_blocks needs i_block_reservation_lock. In fact mballoc
calls this function without the lock. So could not we just compute
'total' under the lock, release it, reserve quota and then claim free
blocks? You'd get rid of undoing the block reservation and obtain quota
and blocks in the usual order...

								Honza

> ---
>  fs/ext4/inode.c |   29 ++++++++++++++++-------------
>  1 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 942e183..f693768 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1852,19 +1852,8 @@ repeat:
>  	md_needed = mdblocks - EXT4_I(inode)->i_reserved_meta_blocks;
>  	total = md_needed + nrblocks;
>  
> -	/*
> -	 * Make quota reservation here to prevent quota overflow
> -	 * later. Real quota accounting is done at pages writeout
> -	 * time.
> -	 */
> -	if (vfs_dq_reserve_block(inode, total)) {
> -		spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> -		return -EDQUOT;
> -	}
> -
>  	if (ext4_claim_free_blocks(sbi, total)) {
>  		spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> -		vfs_dq_release_reservation_block(inode, total);
>  		if (ext4_should_retry_alloc(inode->i_sb, &retries)) {
>  			yield();
>  			goto repeat;
> @@ -1872,10 +1861,24 @@ repeat:
>  		return -ENOSPC;
>  	}
>  	EXT4_I(inode)->i_reserved_data_blocks += nrblocks;
> -	EXT4_I(inode)->i_reserved_meta_blocks = mdblocks;
> +	EXT4_I(inode)->i_reserved_meta_blocks += md_needed;
> +	spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> +
> +	/*
> +	 * Make quota reservation here to prevent quota overflow
> +	 * later. Real quota accounting is done at pages writeout
> +	 * time.
> +	 */
> +	if (!vfs_dq_reserve_block(inode, total))
> +		return 0;       /* success */
>  
> +	/* Quota reservation has failed, revert inode's reservation */
> +	percpu_counter_sub(&sbi->s_dirtyblocks_counter, total);
> +	spin_lock(&EXT4_I(inode)->i_block_reservation_lock);
> +	EXT4_I(inode)->i_reserved_data_blocks -= nrblocks;
> +	EXT4_I(inode)->i_reserved_meta_blocks -= md_needed;
>  	spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
> -	return 0;       /* success */
> +	return -EDQUOT;
>  }
>  
>  static void ext4_da_release_space(struct inode *inode, int to_free)
> -- 
> 1.6.0.4
> 
> --
> 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
-- 
Jan Kara <jack@...e.cz>
SuSE CR Labs
--
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