linux-ext4 - Re: [PATCH 3/6] f2fs: skip truncate when verity in progress in ->write

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: <13be698c-1a3d-9f6a-66d8-b9024b7963f3@huawei.com>
Date:   Tue, 13 Aug 2019 10:40:39 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     <linux-fscrypt@...r.kernel.org>, <linux-ext4@...r.kernel.org>,
        <linux-f2fs-devel@...ts.sourceforge.net>
Subject: Re: [PATCH 3/6] f2fs: skip truncate when verity in progress in
 ->write_begin()

Hi Eric,

On 2019/8/13 6:58, Eric Biggers wrote:
> Hi Chao,
> 
> On Mon, Aug 12, 2019 at 08:25:33PM +0800, Chao Yu wrote:
>> Hi Eric,
>>
>> On 2019/8/12 5:35, Eric Biggers wrote:
>>> From: Eric Biggers <ebiggers@...gle.com>
>>>
>>> When an error (e.g. ENOSPC) occurs during f2fs_write_begin() when called
>>> from f2fs_write_merkle_tree_block(), skip truncating the file.  i_size
>>> is not meaningful in this case, and the truncation is handled by
>>> f2fs_end_enable_verity() instead.
>>>
>>> Fixes: 60d7bf0f790f ("f2fs: add fs-verity support")
>>> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
>>> ---
>>>  fs/f2fs/data.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 3f525f8a3a5fa..00b03fb87bd9b 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2476,7 +2476,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
>>>  	struct inode *inode = mapping->host;
>>>  	loff_t i_size = i_size_read(inode);
>>>  
>>> -	if (to > i_size) {
>>
>> Maybe adding one single line comment to mention that it's redundant/unnecessary
>> truncation here is better, if I didn't misunderstand this.
>>
>> Thanks,
>>
>>> +	if (to > i_size && !f2fs_verity_in_progress(inode)) {
>>>  		down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
>>>  		down_write(&F2FS_I(inode)->i_mmap_sem);
>>>  
> 
> Do you mean add a comment instead of the !f2fs_verity_in_progress() check, or in
> addition to it?  ->write_begin(), ->writepages(), and ->write_end() are all

Sorry, I didn't make this very clear, I meant adding the comment in addition on
above change.

> supposed to ignore i_size when verity is in progress, so I don't think this
> particular part should be different, even if technically it's still correct to
> truncate twice.  Also, ext4 needs this check in its ->write_begin() for locking
> reasons; we should keep f2fs similar.

Agreed.

> 
> How about having both a comment and the check, like:
> 
>         /* In the fs-verity case, f2fs_end_enable_verity() does the truncate */
>         if (to > i_size && !f2fs_verity_in_progress(inode)) {

The comment looks good to me. :)

Thanks,

> 
> - Eric
> .
>