linux-kernel - Re: [PATCH 3/4] f2fs: compress: fix error path in prepare_compress

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: <e5c45ba2-6437-c84a-11b3-abe8c16a5c6c@huawei.com>
Date:   Tue, 7 Jan 2020 09:35:00 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Jaegeuk Kim <jaegeuk@...nel.org>
CC:     <linux-f2fs-devel@...ts.sourceforge.net>,
        <linux-kernel@...r.kernel.org>, <chao@...nel.org>
Subject: Re: [PATCH 3/4] f2fs: compress: fix error path in
 prepare_compress_overwrite()

On 2020/1/7 3:08, Jaegeuk Kim wrote:
> On 01/06, Chao Yu wrote:
>> - fix to release cluster pages in retry flow
>> - fix to call f2fs_put_dnode() & __do_map_lock() in error path
>>
>> Signed-off-by: Chao Yu <yuchao0@...wei.com>
>> ---
>>  fs/f2fs/compress.c | 22 ++++++++++++++++------
>>  1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>> index fc4510729654..3390351d2e39 100644
>> --- a/fs/f2fs/compress.c
>> +++ b/fs/f2fs/compress.c
>> @@ -626,20 +626,26 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
>>  	}
>>  
>>  	for (i = 0; i < cc->cluster_size; i++) {
>> +		f2fs_bug_on(sbi, cc->rpages[i]);
>> +
>>  		page = find_lock_page(mapping, start_idx + i);
>>  		f2fs_bug_on(sbi, !page);
>>  
>>  		f2fs_wait_on_page_writeback(page, DATA, true, true);
>>  
>> -		cc->rpages[i] = page;
>> +		f2fs_compress_ctx_add_page(cc, page);
>>  		f2fs_put_page(page, 0);
>>  
>>  		if (!PageUptodate(page)) {
>> -			for (idx = 0; idx < cc->cluster_size; idx++) {
>> -				f2fs_put_page(cc->rpages[idx],
>> -						(idx <= i) ? 1 : 0);
>> +			for (idx = 0; idx <= i; idx++) {
>> +				unlock_page(cc->rpages[idx]);
>>  				cc->rpages[idx] = NULL;
>>  			}
>> +			for (idx = 0; idx < cc->cluster_size; idx++) {
>> +				page = find_lock_page(mapping, start_idx + idx);
> 
> Why do we need to lock the pages again?

Here, all pages in cluster has one extra reference count, we need to find all
pages, and release those references on them.

cc->rpages may not record all pages' pointers, so we can not use

f2fs_put_page(cc->rpages[idx], (idx <= i) ? 1 : 0); to release all pages' references.

BTW, find_get_page() should be fine to instead find_lock_page().

> 
>> +				f2fs_put_page(page, 1);
>> +				f2fs_put_page(page, 0);
>> +			}
>>  			kvfree(cc->rpages);
>>  			cc->nr_rpages = 0;
>>  			goto retry;
>> @@ -654,16 +660,20 @@ static int prepare_compress_overwrite(struct compress_ctx *cc,
>>  		for (i = cc->cluster_size - 1; i > 0; i--) {
>>  			ret = f2fs_get_block(&dn, start_idx + i);
>>  			if (ret) {
>> -				/* TODO: release preallocate blocks */
>>  				i = cc->cluster_size;
>> -				goto unlock_pages;
>> +				break;
>>  			}
>>  
>>  			if (dn.data_blkaddr != NEW_ADDR)
>>  				break;
>>  		}
>>  
>> +		f2fs_put_dnode(&dn);
> 
> We don't neeed this, since f2fs_reserve_block() put the dnode.

Correct.

Thanks,

> 
>> +
>>  		__do_map_lock(sbi, F2FS_GET_BLOCK_PRE_AIO, false);
>> +
>> +		if (ret)
>> +			goto unlock_pages;
>>  	}
>>  
>>  	*fsdata = cc->rpages;
>> -- 
>> 2.18.0.rc1
> .
>