Message-ID: <f1013767-9099-5b38-e3cd-b8caa639f66c@huawei.com>
Date:   Fri, 19 Jun 2020 14:37:52 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Eric Biggers <ebiggers@...nel.org>
CC:     Satya Tangirala <satyat@...gle.com>,
        <linux-fscrypt@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
        <linux-f2fs-devel@...ts.sourceforge.net>,
        <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 3/4] f2fs: add inline encryption support

On 2020/6/19 12:20, Eric Biggers wrote:
> On Fri, Jun 19, 2020 at 10:39:34AM +0800, Chao Yu wrote:
>> Hi Eric,
>>
>> On 2020/6/19 2:13, Eric Biggers wrote:
>>> Hi Chao,
>>>
>>> On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
>>>>> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>>>  
>>>>>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>>>>>  
>>>>> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
>>>>> -			io->last_block_in_bio, fio->new_blkaddr))
>>>>> +	if (io->bio &&
>>>>> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
>>>>> +			      fio->new_blkaddr) ||
>>>>> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
>>>>> +				       fio->page->index, fio)))
>>>>
>>>> bio_page->index, fio)))
>>>>
>>>>>  		__submit_merged_bio(io);
>>>>>  alloc_new:
>>>>>  	if (io->bio == NULL) {
>>>>> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>>>  			goto skip;
>>>>>  		}
>>>>>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
>>>>> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
>>>>> +				       fio->page->index, fio, GFP_NOIO);
>>>>
>>>> bio_page->index, fio, GFP_NOIO);
>>>>
>>>
>>> We're using ->mapping->host and ->index.  Ordinarily that would mean the page
>>> needs to be a pagecache page.  But bio_page can also be a compressed page or a
>>> bounce page containing fs-layer encrypted contents.
>>
>> I'm concerning about compression + inlinecrypt case.
>>
>>>
>>> Is your suggestion to keep using fio->page->mapping->host (since encrypted pages
>>
>> Yup,
>>
>>> don't have a mapping), but start using bio_page->index (since f2fs apparently
>>
>> I meant that we need to use bio_page->index as tweak value in write path to
>> keep consistent as we did in read path, otherwise we may read the wrong
>> decrypted data later to incorrect tweak value.
>>
>> - f2fs_read_multi_pages (only comes from compression inode)
>>  - f2fs_alloc_dic
>>   - f2fs_set_compressed_page(page, cc->inode,
>> 					start_idx + i + 1, dic);
>>                                         ^^^^^^^^^^^^^^^^^
>>   - dic->cpages[i] = page;
>>  - for ()
>>      struct page *page = dic->cpages[i];
>>      if (!bio)
>>        - f2fs_grab_read_bio(..., page->index,..)
>>         - f2fs_set_bio_crypt_ctx(..., first_idx, ..)   /* first_idx == cpage->index */
>>
>> You can see that cpage->index was set to page->index + 1, that's why we need
>> to use one of cpage->index/page->index as tweak value all the time rather than
>> using both index mixed in read/write path.
>>
>> But note that for fs-layer encryption, we have used cpage->index as tweak value,
>> so here I suggest we can keep consistent to use cpage->index in inlinecrypt case.
> 
> Yes, inlinecrypt mustn't change the ciphertext that gets written to disk.
> 
>>
>>> *does* set ->index for compressed pages, and if the file uses fs-layer
>>> encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?
>>>
>>> Does this mean the code is currently broken for compression + inline encryption
>>> because it's using the wrong ->index?  I think the answer is no, since
>>
>> I guess it's broken now for compression + inlinecrypt case.
>>
>>> f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
>>> pages along with the compressed pages.  In that case, your suggestion would be a
>>> cleanup rather than a fix?
>>
>> That's a fix.
>>
> 
> FWIW, I tested this, and it actually works both before and after your suggested
> change.  The reason is that f2fs_write_compressed_pages() actually passes the
> pagecache pages sequentially starting at 'start_idx_of_cluster(cc) + 1' for the
> length of the compressed cluster.  That matches the '+ 1' adjustment elsewhere,
> so we have fio->page->index == bio_page->index.

I've checked the code, yes, that's correct.

> 
> I personally think the way the f2fs compression code works is really confusing.
> Compressed pages don't have a 1:1 correspondence to pagecache pages, so there
> should *not* be a pagecache page passed around when writing a compressed page.

The only place we always use fio->page is:
- f2fs_submit_page_write
 - trace_f2fs_submit_page_write(fio->page,..)
  - f2fs__submit_page_bio
   __entry->dev		= page_file_mapping(page)->host->i_sb->s_dev;
   __entry->ino		= page_file_mapping(page)->host->i_ino;

For compression case, we can get rid of using this parameter because bio_page
(fio->compressed_page) has correct mapping info, however for fs-layer encryption
case, bio_page (fio->encrypted_page, allocated by fscrypt_alloc_bounce_page())
has not correct mapping info.

> 
> Anyway, here's the test script I used in case anyone else wants to use it.  But
> we really need to write a proper f2fs compression + encryption test for xfstests
> which decrypts and decompresses a file in userspace and verifies we get back the
> original data.  (There are already ciphertext verification tests, but they don't
> cover compression.)  Note that this test is needed even for the filesystem-layer
> encryption which is currently supported.

Yes, let me check how to make this testcase a bit later.

> 
> #!/bin/bash
> 
> set -e
> 
> DEV=/dev/vdb
> 
> umount /mnt &> /dev/null || true
> mkfs.f2fs -f -O encrypt,compression,extra_attr $DEV
> head -c 1000000 /dev/zero > /tmp/testdata
> 
> for opt1 in '-o inlinecrypt' ''; do
>         mount $DEV /mnt $opt1
>         rm -rf /mnt/.fscrypt /mnt/dir
>         fscrypt setup /mnt
>         mkdir /mnt/dir
>         chattr +c /mnt/dir
>         echo hunter2 | fscrypt encrypt /mnt/dir --quiet --source=custom_passphrase --name=secret
>         cp /tmp/testdata /mnt/dir/file
>         umount /mnt
>         for opt2 in '-o inlinecrypt' ''; do
>                 mount $DEV /mnt $opt2
>                 echo hunter2 | fscrypt unlock /mnt/dir --quiet
>                 cmp /mnt/dir/file /tmp/testdata
>                 umount /mnt
>         done
> done
> .
> 

Powered by blists - more mailing lists