| Message ID | 20180212094347.22071-1-chandan@linux.vnet.ibm.com |
|---|---|
| Headers | show |
| Series | Ext4 encryption support for blocksize < pagesize | expand |
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. (Just a thought: if we did make fscrypt non-modular, we could potentially limit what gets pulled in from the crypto API by dropping the 'selects' of the specific crypto algorithms, since they aren't hard dependencies. Also the options like CRYPTO_AES and CRYPTO_XTS that are currently being selected actually refer to the generic implementations of those algorithms, which usually aren't the ones people want to be using since they can be very slow compared to architecture-specific versions.) It would also be easier to review the patch that moves ext4_mpage_readpages() to fs/crypto/ if you split out the behavior change (allowing blocksize < pagesize) into a separate patch. Switching ext4 to use it can be separate from adding it as well. Note that the situation with ext4_block_write_begin() vs __block_write_begin() is very similar to ext4_mpage_readpages() vs mpage_readpages(). So I think whatever you are doing with readpages (e.g. moving it to fs/crypto/, or adding a callback to the generic version) should also be done with __block_write_begin(). In general, it would also be helpful if you kept comments up to date. It makes it difficult to read the code when I see fscrypt_encrypt_block(), but the comment says fscrypt_encrypt_page() and the documented behavior is very different from what it actually does. fscrypt_prep_ciphertext_page() also doesn't have a comment at all. Updating f2fs and ubifs is important as well so that you can see whether the changes actually work for them or not. - Eric
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(). > > (Just a thought: if we did make fscrypt non-modular, we could potentially limit > what gets pulled in from the crypto API by dropping the 'selects' of the > specific crypto algorithms, since they aren't hard dependencies. Also the > options like CRYPTO_AES and CRYPTO_XTS that are currently being selected > actually refer to the generic implementations of those algorithms, which usually > aren't the ones people want to be using since they can be very slow compared to > architecture-specific versions.) > > It would also be easier to review the patch that moves ext4_mpage_readpages() to > fs/crypto/ if you split out the behavior change (allowing blocksize < pagesize) > into a separate patch. Switching ext4 to use it can be separate from adding it > as well. > I will implement this in the next version of the patchset. > Note that the situation with ext4_block_write_begin() vs __block_write_begin() > is very similar to ext4_mpage_readpages() vs mpage_readpages(). So I think > whatever you are doing with readpages (e.g. moving it to fs/crypto/, or adding a > callback to the generic version) should also be done with __block_write_begin(). I will implement this in the next version of the patchset. > > In general, it would also be helpful if you kept comments up to date. It makes > it difficult to read the code when I see fscrypt_encrypt_block(), but the > comment says fscrypt_encrypt_page() and the documented behavior is very > different from what it actually does. fscrypt_prep_ciphertext_page() also > doesn't have a comment at all. I will update the comments. > > Updating f2fs and ubifs is important as well so that you can see whether the > changes actually work for them or not. Yes, I have started making the changes for F2FS and will make sure to get it working for UBIFS before posting the next version of the patchset. Eric, Thanks once again for the review comments.
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
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.
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". Chandan Rajendra (11): ext4: Clear BH_Uptodate flag on decryption error fs/buffer.c: Export end_buffer_async_read and create_page_buffers fs/crypto/: Rename functions to indicate that they operate on FS blocks completion_pages: Decrypt all contiguous blocks in a page ext4: Decrypt all boundary blocks when doing buffered write ext4: Decrypt the block that needs to be partially zeroed fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page Enable reading encrypted files in blocksize less than pagesize setup fscrypt: Move completion_pages to crypto/readpage.c Enable writing encrypted files in blocksize less than pagesize setup ext4: Enable encryption for blocksize less than page size Documentation/filesystems/fscrypt.rst | 14 +- fs/buffer.c | 6 +- fs/crypto/Makefile | 2 +- fs/crypto/bio.c | 77 +++--- fs/crypto/crypto.c | 91 +++--- fs/crypto/fscrypt_private.h | 5 +- fs/crypto/readpage.c | 506 ++++++++++++++++++++++++++++++++++ fs/ext4/Makefile | 2 +- fs/ext4/ext4.h | 5 - fs/ext4/inode.c | 53 +++- fs/ext4/page-io.c | 34 ++- fs/ext4/readpage.c | 294 -------------------- fs/ext4/super.c | 7 - include/linux/buffer_head.h | 3 + include/linux/fscrypt.h | 1 + include/linux/fscrypt_notsupp.h | 23 +- include/linux/fscrypt_supp.h | 20 +- 17 files changed, 700 insertions(+), 443 deletions(-) create mode 100644 fs/crypto/readpage.c delete mode 100644 fs/ext4/readpage.c