From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:38270 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752790AbeBVIsn (ORCPT ); Thu, 22 Feb 2018 03:48:43 -0500 Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w1M8cVbM074760 for ; Thu, 22 Feb 2018 03:48:42 -0500 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0a-001b2d01.pphosted.com with ESMTP id 2g9syh1rfc-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 22 Feb 2018 03:48:42 -0500 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 22 Feb 2018 08:48:40 -0000 From: Chandan Rajendra To: Eric Biggers 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 In-Reply-To: <20180221190655.GB114620@gmail.com> References: <20180212094347.22071-1-chandan@linux.vnet.ibm.com> <3754767.QgBE46qNIq@localhost.localdomain> <20180221190655.GB114620@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Message-Id: <5410362.ZvmN2s610A@localhost.localdomain> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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