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]
Date:	Tue, 28 Aug 2012 20:29:52 +0400
From:	Dmitry Monakhov <dmonakhov@...nvz.org>
To:	linux-ext4@...r.kernel.org
Cc:	tytso@....edu, a-fujita@...jp.nec.com
Subject: Re: [PATCH 2/3] ext4: basic bug shield for move_extent_per_page

On Tue, 28 Aug 2012 20:21:42 +0400, Dmitry Monakhov <dmonakhov@...nvz.org> wrote:
In order to test MOVE_EXT ioctl code i use run e4defrag and fsstress
in parallel:

View attachment "285" of type "text/plain" (2522 bytes)


> The move_extent_per_page function is just one big peace of crap.
> 
> Non-full list of bugs:
> 1) uninitialized extent optimization does not hold page's lock,
>    and simply replace brunches, so writeback code goes
>    crazy because block mapping changed under it's feets
>    kernel BUG at fs/ext4/inode.c:1434!
> 
> 2) uninitialized extent may became initialized right after we
>    drop i_data_sem, so extent state must be rechecked
> 
> 3) Locked pages goes up to date via following sequence:
>    ->readpage(page); lock_page(page); use_that_page(page)
>    But after readpage() one may invalidate it because it is
>    uptodate and unlocked (reclaimer does that)
>    As result kernel bug at include/linux/buffer_head.c:133!
> 4) We call write_begin() with already opened stansaction which
>    result in following deadlock:
> ->move_extent_per_page()
>   ->ext4_journal_start()-> hold journal transaction
>   ->write_begin()
>     ->ext4_da_write_begin()
>       ->ext4_nonda_switch()
>         ->writeback_inodes_sb_if_idle()  --> will wait for journal_stop()
> 
> 5) try_to_release_page() may fail and it does fail if one of page's bh was
>    pinned by journal
> 
> 6) If we about to change page's mapping we MUST hold it's lock during entire
>    remapping procedure, this is true for both pages(original and donor one)
> 
> Fixes:
> 
> - Avoid (1) and (2) simply by temproraly drop uninitialized extent handling
>   optimization, this will be reimplemented later.
> 
> - Fix (3) by manually forcing page to uptodate state w/o dropping it's lock
> 
> - Fix (4) by rearranging existing locking:
>   from: journal_start(); ->write_begin
>   to: write_begin(); journal_extend()
> - Fix (5) simply by checking retvalue
> - (6) requires full function's code rearrangement, will be done later.
> ---
>  fs/ext4/move_extent.c |   74 ++++++++++++++++++++++++-------------------------
>  1 files changed, 36 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index c5826c6..c5ca317 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -812,11 +812,6 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	 * inode and donor_inode may change each different metadata blocks.
>  	 */
>  	jblocks = ext4_writepage_trans_blocks(orig_inode) * 2;
> -	handle = ext4_journal_start(orig_inode, jblocks);
> -	if (IS_ERR(handle)) {
> -		*err = PTR_ERR(handle);
> -		return 0;
> -	}
>  
>  	if (segment_eq(get_fs(), KERNEL_DS))
>  		w_flags |= AOP_FLAG_UNINTERRUPTIBLE;
> @@ -824,19 +819,6 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	orig_blk_offset = orig_page_offset * blocks_per_page +
>  		data_offset_in_page;
>  
> -	/*
> -	 * If orig extent is uninitialized one,
> -	 * it's not necessary force the page into memory
> -	 * and then force it to be written out again.
> -	 * Just swap data blocks between orig and donor.
> -	 */
> -	if (uninit) {
> -		replaced_count = mext_replace_branches(handle, orig_inode,
> -						donor_inode, orig_blk_offset,
> -						block_len_in_page, err);
> -		goto out2;
> -	}
> -
>  	offs = (long long)orig_blk_offset << orig_inode->i_blkbits;
>  
>  	/* Calculate data_size */
> @@ -862,12 +844,28 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  				 &page, &fsdata);
>  	if (unlikely(*err < 0))
>  		goto out;
> +	handle = journal_current_handle();
>  
> +	if (!page_has_buffers(page))
> +		create_empty_buffers(page, 1 << orig_inode->i_blkbits, 0);
> +
> +	/* Force page uptodate similar block_write_commit */
> +	page_zero_new_buffers(page, 0, PAGE_SIZE);
>  	if (!PageUptodate(page)) {
> -		mapping->a_ops->readpage(o_filp, page);
> -		lock_page(page);
> +		struct buffer_head *head, *bh;
> +		bh = head =  page_buffers(page);
> +		do {
> +			if (!bh_uptodate_or_lock(bh)) {
> +				if (bh_submit_read(bh) < 0) {
> +					put_bh(bh);
> +					err2  = -EIO;
> +					goto drop_page;
> +				}
> +			}
> +			bh = bh->b_this_page;
> +		} while (bh != head);
>  	}
> -
> +	SetPageUptodate(page);
>  	/*
>  	 * try_to_release_page() doesn't call releasepage in writeback mode.
>  	 * We should care about the order of writing to the same file
> @@ -876,9 +874,15 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  	 * writeback of the page.
>  	 */
>  	wait_on_page_writeback(page);
> -
> -	/* Release old bh and drop refs */
> -	try_to_release_page(page, 0);
> +	/* Finally page is fully uptodate and locked, it is time to drop
> +	 * old mapping info, function may fail other task hold reference
> +	 * on page's bh */
> +	if (!try_to_release_page(page, 0)) {
> +		replaced_size = 0;
> +		goto write_end;
> +	}
> +	if (ext4_journal_extend(handle, jblocks) < 0)
> +		goto write_end;
>  
>  	replaced_count = mext_replace_branches(handle, orig_inode, donor_inode,
>  					orig_blk_offset, block_len_in_page,
> @@ -889,7 +893,7 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  			replaced_size =
>  				block_len_in_page << orig_inode->i_blkbits;
>  		} else
> -			goto out;
> +			goto drop_page;
>  	}
>  
>  	if (!page_has_buffers(page))
> @@ -903,30 +907,24 @@ move_extent_per_page(struct file *o_filp, struct inode *donor_inode,
>  		*err = ext4_get_block(orig_inode,
>  				(sector_t)(orig_blk_offset + i), bh, 0);
>  		if (*err < 0)
> -			goto out;
> +			goto drop_page;
>  
>  		if (bh->b_this_page != NULL)
>  			bh = bh->b_this_page;
>  	}
> -
> +write_end:
>  	*err = a_ops->write_end(o_filp, mapping, offs, data_size, replaced_size,
>  			       page, fsdata);
> -	page = NULL;
> -
>  out:
> -	if (unlikely(page)) {
> -		if (PageLocked(page))
> -			unlock_page(page);
> -		page_cache_release(page);
> -		ext4_journal_stop(handle);
> -	}
> -out2:
> -	ext4_journal_stop(handle);
> -
>  	if (err2)
>  		*err = err2;
>  
>  	return replaced_count;
> +drop_page:
> +	unlock_page(page);
> +	page_cache_release(page);
> +	ext4_journal_stop(handle);
> +	goto out;
>  }
>  
>  /**
> -- 
> 1.7.7.6
> 
> --
> 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

Powered by blists - more mailing lists