linux-ext4 - Re: [PATCH] ext4: move_extent explicitly invalidate page buffers

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:	Wed, 10 Dec 2014 17:21:50 +0800
From:	Xiaoguang Wang <wangxg.fnst@...fujitsu.com>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
CC:	<linux-ext4@...r.kernel.org>, <tytso@....edu>
Subject: Re: [PATCH] ext4: move_extent explicitly invalidate page buffers

Hi,

On 10/23/2014 09:03 PM, Dmitry Monakhov wrote:
> Dmitry Monakhov <dmonakhov@...nvz.org> writes:
> 
>> In hard core test-cases such as ext4/301, ext4/302 some bh may
>> becomes dirty so try_to_release_page() will fail and result in
>> false positive EBUSY failures. We can easily fix that by
>> explicit ->invalidatepage() after we holds page which is locked
>> and uptodate.
> Sorry. This patch is not correct. Please ignore it.

Sorry to bother.
I'd like to know whether you're going to send patches to fix this issue, thanks?

If try_to_release_page(...) failed, some bh's state would be BH_Uptodate, BH_Dirty,
BH_Mapped and BH_Unwritten. In this patch, against such page, you call mext_page_mkuptodate(),
but it then calls bh_submit_read()... I wonder whether we should start some write
operation, thanks!
I attached a log, which I once sent to you, thanks!

Regards,
Xiaoguang Wang

>>
>> Tested-by: Xiaoguang Wang<wangxg.fnst@...fujitsu.com>
>> Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
>> ---
>>  fs/ext4/move_extent.c |   10 +++++++---
>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
>> index c2b2b02..76c45b6 100644
>> --- a/fs/ext4/move_extent.c
>> +++ b/fs/ext4/move_extent.c
>> @@ -348,8 +348,9 @@ again:
>>  		     !try_to_release_page(pagep[0], 0)) ||
>>  		    (page_has_private(pagep[1]) &&
>>  		     !try_to_release_page(pagep[1], 0))) {
>> -			*err = -EBUSY;
>> -			goto drop_data_sem;
>> +			/* One of buffers is busy, fall back data copy */
>> +			ext4_double_up_write_data_sem(orig_inode, donor_inode);
>> +			goto data_copy;
>>  		}
>>  		replaced_count = ext4_swap_extents(handle, orig_inode,
>>  						   donor_inode, orig_blk_offset,
>> @@ -360,12 +361,15 @@ again:
>>  		goto unlock_pages;
>>  	}
>>  data_copy:
>> -	*err = mext_page_mkuptodate(pagep[0], from, from + replaced_size);
>> +	/* In order to drop all buffers we have to make page fully uptodate */
>> +	*err = mext_page_mkuptodate(pagep[0], 0, PAGE_CACHE_SIZE);
>>  	if (*err)
>>  		goto unlock_pages;
>>  
>>  	/* At this point all buffers in range are uptodate, old mapping layout
>>  	 * is no longer required, try to drop it now. */
>> +	do_invalidatepage(pagep[0], 0, PAGE_CACHE_SIZE);
>> +	do_invalidatepage(pagep[1], 0, PAGE_CACHE_SIZE);
>>  	if ((page_has_private(pagep[0]) && !try_to_release_page(pagep[0], 0)) ||
>>  	    (page_has_private(pagep[1]) && !try_to_release_page(pagep[1], 0))) {
>>  		*err = -EBUSY;
>> -- 
>> 1.7.1
>>
>> --
>> 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


View attachment "0001-patch-bh-debug.patch.patch" of type "text/x-patch" (2160 bytes)

View attachment "dmesg.info" of type "text/plain" (40234 bytes)