Ext4 encryption support for blocksize < pagesize

With block size < page size, ext4_block_write_begin() can have up to two blocks to decrypt. Hence this commit invokes fscrypt_decrypt_block() for each of those blocks.

Signed-off-by Chandan Rajendra <[email protected]>

---
 fs/ext4/inode.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

fs/ext4/inode.c +5 -4

index 69a4fd6..180dd2d 100644
@@ -1158,12 +1158,13 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
 	unsigned to = from + len;
 	struct inode *inode = page->mapping->host;
 	unsigned block_start, block_end;
-	sector_t block;
+	sector_t block, page_blk_nr;
 	int err = 0;
 	unsigned blocksize = inode->i_sb->s_blocksize;
 	unsigned bbits;
 	struct buffer_head *bh, *head, *wait[2], **wait_bh = wait;
 	bool decrypt = false;
+	int i;
 
 	BUG_ON(!PageLocked(page));
 	BUG_ON(from > PAGE_SIZE);

@@ -1224,18 +1225,30 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
 	/*
 	 * If we issued read requests, let them complete.
 	 */
-	while (wait_bh > wait) {
-		wait_on_buffer(*--wait_bh);
-		if (!buffer_uptodate(*wait_bh))
+	for (i = 0; &wait[i] < wait_bh; i++) {
+		wait_on_buffer(wait[i]);
+		if (!buffer_uptodate(wait[i]))
 			err = -EIO;
 	}

fs/ext4/inode.c +4 -0

+		for (i = 0; &wait[i] < wait_bh; i++) {
+			int err2;
+
+			--wait_bh;

Hi Chandan,

Quoted 40 lines · Chandan Rajendra

With block size < page size, ext4_block_write_begin() can have up to two blocks to decrypt. Hence this commit invokes fscrypt_decrypt_block() for each of those blocks.

Signed-off-by Chandan Rajendra <[email protected]>

---
 fs/ext4/inode.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

fs/ext4/inode.c +5 -4

index 69a4fd6..180dd2d 100644
@@ -1158,12 +1158,13 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
 	unsigned to = from + len;
 	struct inode *inode = page->mapping->host;
 	unsigned block_start, block_end;
-	sector_t block;
+	sector_t block, page_blk_nr;
 	int err = 0;
 	unsigned blocksize = inode->i_sb->s_blocksize;
 	unsigned bbits;
 	struct buffer_head *bh, *head, *wait[2], **wait_bh = wait;
 	bool decrypt = false;
+	int i;
 
 	BUG_ON(!PageLocked(page));
 	BUG_ON(from > PAGE_SIZE);

@@ -1224,18 +1225,30 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
 	/*
 	 * If we issued read requests, let them complete.
 	 */
-	while (wait_bh > wait) {
-		wait_on_buffer(*--wait_bh);
-		if (!buffer_uptodate(*wait_bh))
+	for (i = 0; &wait[i] < wait_bh; i++) {
+		wait_on_buffer(wait[i]);
+		if (!buffer_uptodate(wait[i]))
 			err = -EIO;
 	}

[...]

Quoted 11 lines

fs/ext4/inode.c +11 -0

+		for (i = 0; &wait[i] < wait_bh; i++) {
+			int err2;
+
+			--wait_bh;
+			block = page_blk_nr + (bh_offset(wait[i]) >> bbits);
+			err2 = fscrypt_decrypt_block(page->mapping->host, page,
+						wait[i]->b_size,
+						bh_offset(wait[i]),
+						block);
+			if (err2) {
+				clear_buffer_uptodate(wait[i]);

fs/ext4/inode.c static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len, · line 1251

Quoted 3 lines

fs/ext4/inode.c +3 -0

+				err = err2;
+			}
+		}

These are very confusing ways to iterate through an array, especially the second loop which is actually going in reverse order (why?). Why not just use a variable like 'nr_wait' for the number of valid buffer_head's like I had suggested? Then you can just do 'for (i = 0; i < nr_wait; i++)'.

For block size < page size, This commit adds code to encrypt all zeroed out blocks of a page.

Signed-off-by Chandan Rajendra <[email protected]>

---
 fs/crypto/bio.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

fs/crypto/bio.c +4 -4

index 378df08..4d0d14f 100644
@@ -104,10 +104,12 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 {
 	struct fscrypt_ctx *ctx;
 	struct page *ciphertext_page = NULL;
+	unsigned int page_nr_blks;
+	unsigned int offset;
+	unsigned int page_io_len;
 	struct bio *bio;
 	int ret, err = 0;
-
-	BUG_ON(inode->i_sb->s_blocksize != PAGE_SIZE);
+	int i;
 
 	ctx = fscrypt_get_ctx(inode, GFP_NOFS);
 	if (IS_ERR(ctx))

@@ -119,12 +121,23 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 		goto errout;
 	}
 
-	while (len--) {
-		err = fscrypt_do_block_crypto(inode, FS_ENCRYPT, lblk,

Quoted 31 lines · Chandan Rajendra

For block size < page size, This commit adds code to encrypt all zeroed out blocks of a page.

Signed-off-by Chandan Rajendra <[email protected]>

---
 fs/crypto/bio.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

fs/crypto/bio.c +4 -2

index 378df08..4d0d14f 100644
@@ -104,10 +104,12 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 {
 	struct fscrypt_ctx *ctx;
 	struct page *ciphertext_page = NULL;
+	unsigned int page_nr_blks;
+	unsigned int offset;
+	unsigned int page_io_len;
 	struct bio *bio;
 	int ret, err = 0;
-
-	BUG_ON(inode->i_sb->s_blocksize != PAGE_SIZE);
+	int i;
 
 	ctx = fscrypt_get_ctx(inode, GFP_NOFS);
 	if (IS_ERR(ctx))

@@ -119,12 +121,23 @@ int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 		goto errout;
 	}
 

fs/crypto/bio.c int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk, · line 129

Quoted 12 lines

fs/crypto/bio.c +6 -6

-	while (len--) {
-		err = fscrypt_do_block_crypto(inode, FS_ENCRYPT, lblk,
-					     ZERO_PAGE(0), ciphertext_page,
-					     PAGE_SIZE, 0, GFP_NOFS);
-		if (err)
-			goto errout;
+	page_nr_blks = 1 << (PAGE_SHIFT - inode->i_blkbits);
+
+	while (len) {
+		page_nr_blks = min_t(unsigned int, page_nr_blks, len);
+		page_io_len = page_nr_blks << inode->i_blkbits;
+		offset = 0;

The 'page_io_len' variable isn't needed, since 'offset == page_io_len' after the encryption loop. You can do 'bio_add_page(bio, ciphertext_page, offset, 0);'.

You are right. I will fix that up in the next iteration of the patchset.

Quoted 3 lines · Chandan Rajendra

You are right. I will fix that up in the next iteration of the patchset.

Hi Chandan,

When were you planning on sending out the next iteration of the patchset? The merge window will be opening soon, and I wasn't sure if I had missed a new iteration of your changes.ts

Also, it looks like when you renamed the *_page fscrypt functions to *_blocks, on the write side, a bounce page is still being used for each block. So so an an architecture which has 64k pages, and we are writing to a file sytem with 4k blocks, to write a 64k page, the fscrypt layer will have to allocate 16 64k bounce pages to write a single 64k page to an encrypted file. Am I missing something?

Quoted 7 lines

Also, it looks like when you renamed the *_page fscrypt functions to *_blocks, on the write side, a bounce page is still being used for each block. So so an an architecture which has 64k pages, and we are writing to a file sytem with 4k blocks, to write a 64k page, the fscrypt layer will have to allocate 16 64k bounce pages to write a single 64k page to an encrypted file. Am I missing something?

ext4_bio_write_page() invokes the new fscrypt_encrypt_block() function for each block of the page that has been marked with "Async write". For all blocks of the page that needs to be written to the disk, we pass the same bounce page as an argument to fscrypt_encrypt_block().

Quoted 8 lines · Chandan Rajendra

Quoted 7 lines

Also, it looks like when you renamed the *_page fscrypt functions to *_blocks, on the write side, a bounce page is still being used for each block. So so an an architecture which has 64k pages, and we are writing to a file sytem with 4k blocks, to write a 64k page, the fscrypt layer will have to allocate 16 64k bounce pages to write a single 64k page to an encrypted file. Am I missing something?

Quoted 4 lines

ext4_bio_write_page() invokes the new fscrypt_encrypt_block() function for each block of the page that has been marked with "Async write". For all blocks of the page that needs to be written to the disk, we pass the same bounce page as an argument to fscrypt_encrypt_block().

Thanks for the explanation. I do wonder if the proper thing to export from the fscrypt layer is fscrypt_encrypt_page(), since for all file systems, the only thing which really makes sense is to read and write a full page at a time, since we cache things at the page cache a full page a time. So instead of teaching each file system how to use fscrypt_{encrypt,decrypt}_block, maybe push that into the fscrypt layer, and implement a new fscrypt_encrypt_page() which calls fs_encrypt_block()?

Quoted 8 lines · Chandan Rajendra

Quoted 7 lines

Also, it looks like when you renamed the *_page fscrypt functions to *_blocks, on the write side, a bounce page is still being used for each block. So so an an architecture which has 64k pages, and we are writing to a file sytem with 4k blocks, to write a 64k page, the fscrypt layer will have to allocate 16 64k bounce pages to write a single 64k page to an encrypted file. Am I missing something?

Quoted 4 lines

ext4_bio_write_page() invokes the new fscrypt_encrypt_block() function for each block of the page that has been marked with "Async write". For all blocks of the page that needs to be written to the disk, we pass the same bounce page as an argument to fscrypt_encrypt_block().

Thanks for the explanation. I do wonder if the proper thing to export from the fscrypt layer is fscrypt_encrypt_page(), since for all file systems, the only thing which really makes sense is to read and write a full page at a time, since we cache things at the page cache a full page a time. So instead of teaching each file system how to use fscrypt_{encrypt,decrypt}_block, maybe push that into the fscrypt layer, and implement a new fscrypt_encrypt_page() which calls fs_encrypt_block()?

I encountered a problem when refactoring the code to get fscrypt layer to encrypt all the blocks of a page by internally calling fscrypt_encrypt_block().

It is the filesystem which knows which subset of blocks mapped by a page that needs to be encrypted.

For example, ext4_bio_write_page() marks such blocks
with "Async Write" flag and later in another pass, it encrypts and also adds these blocks to a bio.

Quoted 7 lines · Chandan Rajendra

I encountered a problem when refactoring the code to get fscrypt layer to encrypt all the blocks of a page by internally calling fscrypt_encrypt_block().

It is the filesystem which knows which subset of blocks mapped by a page that needs to be encrypted.

That's not quite correct. All blocks in a file are either always encrypted, or not. So that's not really the problem.

Quoted 4 lines

For example, ext4_bio_write_page() marks such blocks
with "Async Write" flag and later in another pass, it encrypts and also adds these blocks to a bio.

The tricky bits with ext4_bio_write_page() all are in handling the case where page_size > block_size. In that case, where there are multiple file system blocks covering a page, we need to know the on-disk block numbers are for the blocks covering the page, and the encryption is intertwined with the I/O submission path, which is file system specific -- mainly because how the completion callback and the parameters which need to be passed *into* the the callback function is file system specific.

However, none of that is needed or relevant to the encryption operation. It's an accident of code development history that fscrypt_encrypt_page was placed where it was.

That is, none of work done in the first pass (starting with the comment "In the first loop we prepare and mark buffers to submit....") is needed to be done before we call fscrypt_encrypt_page(). That call:

	data_page = fscrypt_encrypt_page(inode, page, PAGE_SIZE, 0,
					page->index, gfp_flags);

... could easily be moved to the beginning of ext4_bio_write_page().

I can do that to make the function easier to understand, but that particular cleanup is merely cosmetic. It doesn't what you would need to do order to make fscrypt_encrypt_page() iterate over the page as it calls fscrypt_encrypt_buffer().

Sorry, I wasn't clear enough with my explaination. I actually meant to say that not all blocks mapped by a page might be dirty and hence only a subset of the dirty blocks might need to be written to the disk.

This commit splits the functionality of fscrypt_encrypt_block(). The allocation of fscrypt context and cipher text page is moved to a new function fscrypt_prep_ciphertext_page().

ext4_bio_write_page() is modified to appropriately make use of the above two functions.

Signed-off-by Chandan Rajendra <[email protected]>

fs/ext4/page-io.c +20 -11

index 0a4a1e7..1e869d5 100644
@@ -419,9 +419,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	struct inode *inode = page->mapping->host;
 	unsigned block_start;
 	struct buffer_head *bh, *head;
+	u64 blk_nr;
+	gfp_t gfp_flags = GFP_NOFS;
 	int ret = 0;
 	int nr_submitted = 0;
 	int nr_to_submit = 0;
+	int blocksize = (1 << inode->i_blkbits);
 
 	BUG_ON(!PageLocked(page));
 	BUG_ON(PageWriteback(page));

@@ -475,15 +478,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		nr_to_submit++;
 	} while ((bh = bh->b_this_page) != head);
 
-	bh = head = page_buffers(page);
-
-	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) &&
-	    nr_to_submit) {
-		gfp_t gfp_flags = GFP_NOFS;
-
-	retry_encrypt:
-		data_page = fscrypt_encrypt_block(inode, page, PAGE_SIZE, 0,
-						page->index, gfp_flags);
+	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)
+		&& nr_to_submit) {
+	retry_prep_ciphertext_page:
+		data_page = fscrypt_prep_ciphertext_page(inode, page,
+							gfp_flags);
 		if (IS_ERR(data_page)) {
 			ret = PTR_ERR(data_page);
 			if (ret == -ENOMEM && wbc->sync_mode == WB_SYNC_ALL) {

@@ -492,17 +491,28 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 					congestion_wait(BLK_RW_ASYNC, HZ/50);
 				}
 				gfp_flags |= __GFP_NOFAIL;
-				goto retry_encrypt;
+				goto retry_prep_ciphertext_page;
 			}
 			data_page = NULL;
 			goto out;
 		}
 	}
 
+	blk_nr = page->index << (PAGE_SHIFT - inode->i_blkbits);
+
 	/* Now submit buffers to write */
+	bh = head = page_buffers(page);
 	do {
 		if (!buffer_async_write(bh))
 			continue;
+
+		if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)) {
+			ret = fscrypt_encrypt_block(inode, page, data_page, blocksize,
+						bh_offset(bh), blk_nr, gfp_flags);
+			if (ret)
+				break;
+		}
+
 		ret = io_submit_add_bh(io, inode,
 				       data_page ? data_page : page, bh);
 		if (ret) {

@@ -515,12 +525,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		}
 		nr_submitted++;
 		clear_buffer_dirty(bh);
-	} while ((bh = bh->b_this_page) != head);

Quoted 8 lines · Chandan Rajendra

This commit splits the functionality of fscrypt_encrypt_block(). The allocation of fscrypt context and cipher text page is moved to a new function fscrypt_prep_ciphertext_page().

ext4_bio_write_page() is modified to appropriately make use of the above two functions.

Signed-off-by Chandan Rajendra <[email protected]>

Well, this patch also modifies ext4_bio_write_page() to support the blocksize < pagesize case. The commit message makes it sound like it's just refactoring.

Quoted 75 lines

fs/ext4/page-io.c +21 -11

index 0a4a1e7..1e869d5 100644
@@ -419,9 +419,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	struct inode *inode = page->mapping->host;
 	unsigned block_start;
 	struct buffer_head *bh, *head;
+	u64 blk_nr;
+	gfp_t gfp_flags = GFP_NOFS;
 	int ret = 0;
 	int nr_submitted = 0;
 	int nr_to_submit = 0;
+	int blocksize = (1 << inode->i_blkbits);
 
 	BUG_ON(!PageLocked(page));
 	BUG_ON(PageWriteback(page));

@@ -475,15 +478,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		nr_to_submit++;
 	} while ((bh = bh->b_this_page) != head);
 
-	bh = head = page_buffers(page);
-
-	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) &&
-	    nr_to_submit) {
-		gfp_t gfp_flags = GFP_NOFS;
-
-	retry_encrypt:
-		data_page = fscrypt_encrypt_block(inode, page, PAGE_SIZE, 0,
-						page->index, gfp_flags);
+	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)
+		&& nr_to_submit) {
+	retry_prep_ciphertext_page:
+		data_page = fscrypt_prep_ciphertext_page(inode, page,
+							gfp_flags);
 		if (IS_ERR(data_page)) {
 			ret = PTR_ERR(data_page);
 			if (ret == -ENOMEM && wbc->sync_mode == WB_SYNC_ALL) {

@@ -492,17 +491,28 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 					congestion_wait(BLK_RW_ASYNC, HZ/50);
 				}
 				gfp_flags |= __GFP_NOFAIL;
-				goto retry_encrypt;
+				goto retry_prep_ciphertext_page;
 			}
 			data_page = NULL;
 			goto out;
 		}
 	}
 
+	blk_nr = page->index << (PAGE_SHIFT - inode->i_blkbits);
+
 	/* Now submit buffers to write */
+	bh = head = page_buffers(page);
 	do {
 		if (!buffer_async_write(bh))
 			continue;
+
+		if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)) {
+			ret = fscrypt_encrypt_block(inode, page, data_page, blocksize,
+						bh_offset(bh), blk_nr, gfp_flags);
+			if (ret)
+				break;
+		}
+
 		ret = io_submit_add_bh(io, inode,
 				       data_page ? data_page : page, bh);
 		if (ret) {

@@ -515,12 +525,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		}
 		nr_submitted++;
 		clear_buffer_dirty(bh);
-	} while ((bh = bh->b_this_page) != head);
+	} while (++blk_nr, (bh = bh->b_this_page) != head);
 

fs/ext4/page-io.c int ext4_bio_write_page(struct ext4_io_submit *io, · line 536

Quoted 8 lines

fs/ext4/page-io.c +1 -1

 	/* Error stopped previous loop? Clean up buffers... */
 	if (ret) {
 	out:
-		if (data_page)
+		if (data_page && bh == head)
 			fscrypt_restore_control_page(data_page);
 		printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
 		redirty_page_for_writepage(wbc, page);

I'm wondering why you didn't move the crypto stuff in ext4_bio_write_page() into a separate function like I had suggested? It's true we don't have to encrypt all the blocks in the page at once, but it would make the crypto stuff more self-contained.

Quoted 88 lines · IST Eric Biggers

Quoted 8 lines · Chandan Rajendra

This commit splits the functionality of fscrypt_encrypt_block(). The allocation of fscrypt context and cipher text page is moved to a new function fscrypt_prep_ciphertext_page().

ext4_bio_write_page() is modified to appropriately make use of the above two functions.

Signed-off-by Chandan Rajendra <[email protected]>

Well, this patch also modifies ext4_bio_write_page() to support the blocksize < pagesize case. The commit message makes it sound like it's just refactoring.

Quoted 75 lines

fs/ext4/page-io.c +21 -11

index 0a4a1e7..1e869d5 100644
@@ -419,9 +419,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 	struct inode *inode = page->mapping->host;
 	unsigned block_start;
 	struct buffer_head *bh, *head;
+	u64 blk_nr;
+	gfp_t gfp_flags = GFP_NOFS;
 	int ret = 0;
 	int nr_submitted = 0;
 	int nr_to_submit = 0;
+	int blocksize = (1 << inode->i_blkbits);
 
 	BUG_ON(!PageLocked(page));
 	BUG_ON(PageWriteback(page));

@@ -475,15 +478,11 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		nr_to_submit++;
 	} while ((bh = bh->b_this_page) != head);
 
-	bh = head = page_buffers(page);
-
-	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode) &&
-	    nr_to_submit) {
-		gfp_t gfp_flags = GFP_NOFS;
-
-	retry_encrypt:
-		data_page = fscrypt_encrypt_block(inode, page, PAGE_SIZE, 0,
-						page->index, gfp_flags);
+	if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)
+		&& nr_to_submit) {
+	retry_prep_ciphertext_page:
+		data_page = fscrypt_prep_ciphertext_page(inode, page,
+							gfp_flags);
 		if (IS_ERR(data_page)) {
 			ret = PTR_ERR(data_page);
 			if (ret == -ENOMEM && wbc->sync_mode == WB_SYNC_ALL) {

@@ -492,17 +491,28 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 					congestion_wait(BLK_RW_ASYNC, HZ/50);
 				}
 				gfp_flags |= __GFP_NOFAIL;
-				goto retry_encrypt;
+				goto retry_prep_ciphertext_page;
 			}
 			data_page = NULL;
 			goto out;
 		}
 	}
 
+	blk_nr = page->index << (PAGE_SHIFT - inode->i_blkbits);
+
 	/* Now submit buffers to write */
+	bh = head = page_buffers(page);
 	do {
 		if (!buffer_async_write(bh))
 			continue;
+
+		if (ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode)) {
+			ret = fscrypt_encrypt_block(inode, page, data_page, blocksize,
+						bh_offset(bh), blk_nr, gfp_flags);
+			if (ret)
+				break;
+		}
+
 		ret = io_submit_add_bh(io, inode,
 				       data_page ? data_page : page, bh);
 		if (ret) {

@@ -515,12 +525,12 @@ int ext4_bio_write_page(struct ext4_io_submit *io,
 		}
 		nr_submitted++;
 		clear_buffer_dirty(bh);
-	} while ((bh = bh->b_this_page) != head);
+	} while (++blk_nr, (bh = bh->b_this_page) != head);
 

Quoted 13 lines

fs/ext4/page-io.c int ext4_bio_write_page(struct ext4_io_submit *io, · line 536

Quoted 8 lines

fs/ext4/page-io.c +1 -1

 	/* Error stopped previous loop? Clean up buffers... */
 	if (ret) {
 	out:
-		if (data_page)
+		if (data_page && bh == head)
 			fscrypt_restore_control_page(data_page);
 		printk_ratelimited(KERN_ERR "%s: ret = %d\n", __func__, ret);
 		redirty_page_for_writepage(wbc, page);

I'm wondering why you didn't move the crypto stuff in ext4_bio_write_page() into a separate function like I had suggested? It's true we don't have to encrypt all the blocks in the page at once, but it would make the crypto stuff more self-contained.

Eric, Are you suggesting that the entire block of code that has invocations to fscrypt_prep_ciphertext_page() and fscrypt_encrypt_block() be moved to a separate function that gets defined in fscrypt module?

If yes, In Ext4, We have the invocation of io_submit_add_bh() being interleaved with calls to fscrypt_encrypt_block().

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".

Hi Chandan,

Quoted 37 lines · Chandan Rajendra

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.

Quoted 2 lines

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.

Quoted 43 lines · IST Eric Biggers

Hi Chandan,

Quoted 37 lines · Chandan Rajendra

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.

Quoted 2 lines

Thanks to Eric Biggers for providing review comments for "RFC V1".

Quoted 8 lines

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().

Hi Chandan,

Quoted 58 lines · Chandan Rajendra

Quoted 43 lines · IST Eric Biggers

Hi Chandan,

Quoted 37 lines · Chandan Rajendra

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.

Quoted 2 lines

Thanks to Eric Biggers for providing review comments for "RFC V1".

Quoted 8 lines

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].

Quoted 5 lines

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