Re: [RFC PATCH V2 00/11] Ext4 encryption support for blocksize < pagesize

[Eric Biggers <ebiggers3@gmail.com>]

Hi Chandan,

On Wed, Feb 21, 2018 at 03:27:34PM +0530, Chandan Rajendra wrote:
> On Wednesday, February 21, 2018 6:18:21 AM IST Eric Biggers wrote:
> > Hi Chandan,
> > 
> > On Mon, Feb 12, 2018 at 03:13:36PM +0530, Chandan Rajendra wrote:
> > > This patchset implements code to support encryption of Ext4 filesystem
> > > instances that have blocksize less than pagesize. The patchset has
> > > been tested on both ppc64 and x86_64 machines.
> > > 
> > > Eric, fscrypt_mpage_readpages() (originally, ext4_mpage_readpages())
> > > still retains the ability to read non-encrypted file data. Please let
> > > me know if the code has to be changed such that
> > > fscrypt_mpage_readpages() makes it mandatory for the file's data to be
> > > encrypted.
> > > 
> > > TODO:
> > > F2FS and UBIFS code needs to be updated to make use of the newly added
> > > fscrypt functions. I will do that in the next version of the patchset.
> > > 
> > > Changelog:
> > > "RFC V1" -> "RFC V2":
> > > 1. Ext4's "encryption aware" functionality in fs/ext4/readpage.c has
> > >    been moved to fs/crypto/.
> > > 2. fscrypt functions have now been renamed to indicate that they work
> > >    on blocks rather than pages.
> > >    Eric, I have renamed completion_pages() to fscrypt_complete_pages()
> > >    rather than to fscrypt_complete_blocks(). This is because we have a
> > >    new function fscrypt_complete_block() (which operates on a single
> > >    block) and IMHO having the identifier fscrypt_complete_blocks()
> > >    which differs from it by just one letter would confuse the reader.
> > > 3. ext4_block_write_begin() now clears BH_Uptodate flag when
> > >    decryption of boundary blocks fail.
> > > 4. fscrypt_encrypt_page() (now renamed to fscrypt_encrypt_block()) is
> > >    now split into two functions. fscrypt_prep_ciphertext_page()
> > >    allocates and initializes the fscrypt context and the bounce
> > >    page. fscrypt_encrypt_block() is limited to encrypting the
> > >    filesystem's block.
> > > 5. fscrypt_zeroout_range() has been updated to work on blocksize <
> > >    pagesize scenario.
> > > 6. Documentation/filesystems/fscrypt.rst has been updated to indicate
> > >    encryption support for blocksize < pagesize.
> > > 
> > > Thanks to Eric Biggers for providing review comments for "RFC V1".
> > > 
> > 
> > Thanks for the new version of the patchset.
> > 
> > I see you decided to move ext4's readpages to fs/crypto/.  Did you also consider
> > the other alternatives I had suggested, such as adding an encryption callback to
> > the generic mpage_readpages(), or making fscrypt non-modular and then calling it
> > directly from mpage_readpages()?  Maybe you did, but I don't see the tradeoffs
> > addressed in the patchset at all.  The patches need to explain *why* you're
> > doing what you're doing, not just *what* you're doing.
> 
> I had glanced through F2FS and UBIFS source code. F2FS has its own version of
> mpage_readpage[s] and UBIFS does not use mpage_readpage[s]
> functionality. This was the major reason for deciding to not go with the
> approach of having a decryption call back passed to mpage_readpage[s].
> 
> Apart from the reason of memory being wasted on systems which do not require
> files to be encrypted, the previously listed reason of mpage_readpage[s] not
> being used by F2FS and UBIFS also played a role is deciding against invoking
> fscrypt_decrypt_bio_blocks() from within mpage_readpages().
> 

Sure, F2FS and UBIFS don't use mpage_readpages(), block_write_full_page(), or
__block_write_begin().  But that doesn't mean it's a good idea to copy and paste
all of those functions from generic code to fs/crypto or to fs/ext4 just to add
encryption support.  It will be difficult to maintain two copies of the code.

The other option I suggested, which I think you still haven't addressed at all,
is adding an encryption/decryption callback to those functions, which would be
provided by the filesystem.  See for example how __block_write_begin_int() takes
in an optional 'struct iomap *' pointer; maybe we could do something similar
with crypto?  Note, that approach would have the advantage of not requiring that
fscrypt be built-in.  Just a thought, I haven't tried writing the code yet to
see how difficult/ugly it would be...

Eric