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

From: Chandan Rajendra <chandan@linux.vnet.ibm.com>
To: Eric Biggers <ebiggers3@gmail.com>
Cc: linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-fscrypt@vger.kernel.org, tytso@mit.edu
Subject: Re: [RFC PATCH V2 00/11] Ext4 encryption support for blocksize < pagesize
Date: Thu, 22 Feb 2018 14:20:08 +0530	[thread overview]
Message-ID: <5410362.ZvmN2s610A@localhost.localdomain> (raw)
In-Reply-To: <20180221190655.GB114620@gmail.com>

On Thursday, February 22, 2018 12:36:55 AM IST Eric Biggers wrote:
> 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
> 
> 

Ok. I will take a shot at implementing this. 

-- 
chandan