[3/4] f2fs: add inline encryption support

Wire up f2fs to support inline encryption via the helper functions which
fs/crypto/ now provides.  This includes:

- Adding a mount option 'inlinecrypt' which enables inline encryption
  on encrypted files where it can be used.

- Setting the bio_crypt_ctx on bios that will be submitted to an
  inline-encrypted file.

- Not adding logically discontiguous data to bios that will be submitted
  to an inline-encrypted file.

- Not doing filesystem-layer crypto on inline-encrypted files.

This patch includes a fix for a race during IPU by
Sahitya Tummala <stummala@codeaurora.org>

Co-developed-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Satya Tangirala <satyat@google.com>
---
 Documentation/filesystems/f2fs.rst |  7 ++-
 fs/f2fs/compress.c                 |  2 +-
 fs/f2fs/data.c                     | 81 ++++++++++++++++++++++++------
 fs/f2fs/super.c                    | 32 ++++++++++++
 4 files changed, 104 insertions(+), 18 deletions(-)

On 06/17, Satya Tangirala wrote:
> Wire up f2fs to support inline encryption via the helper functions which
> fs/crypto/ now provides.  This includes:
> 
> - Adding a mount option 'inlinecrypt' which enables inline encryption
>   on encrypted files where it can be used.
> 
> - Setting the bio_crypt_ctx on bios that will be submitted to an
>   inline-encrypted file.
> 
> - Not adding logically discontiguous data to bios that will be submitted
>   to an inline-encrypted file.
> 
> - Not doing filesystem-layer crypto on inline-encrypted files.
> 
> This patch includes a fix for a race during IPU by
> Sahitya Tummala <stummala@codeaurora.org>
> 
> Co-developed-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>

Acked-by: Jaegeuk Kim <jaegeuk@kernel.org>

> ---
>  Documentation/filesystems/f2fs.rst |  7 ++-
>  fs/f2fs/compress.c                 |  2 +-
>  fs/f2fs/data.c                     | 81 ++++++++++++++++++++++++------
>  fs/f2fs/super.c                    | 32 ++++++++++++
>  4 files changed, 104 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst
> index 099d45ac8d8f..4dc36143ff82 100644
> --- a/Documentation/filesystems/f2fs.rst
> +++ b/Documentation/filesystems/f2fs.rst
> @@ -258,7 +258,12 @@ compress_extension=%s  Support adding specified extension, so that f2fs can enab
>                         on compression extension list and enable compression on
>                         these file by default rather than to enable it via ioctl.
>                         For other files, we can still enable compression via ioctl.
> -====================== ============================================================
> +inlinecrypt
> +                       Encrypt/decrypt the contents of encrypted files using the
> +                       blk-crypto framework rather than filesystem-layer encryption.
> +                       This allows the use of inline encryption hardware. The on-disk
> +                       format is unaffected. For more details, see
> +                       Documentation/block/inline-encryption.rst.
>  
>  Debugfs Entries
>  ===============
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 1e02a8c106b0..29e50fbe7eca 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1086,7 +1086,7 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>  		.submitted = false,
>  		.io_type = io_type,
>  		.io_wbc = wbc,
> -		.encrypted = f2fs_encrypted_file(cc->inode),
> +		.encrypted = fscrypt_inode_uses_fs_layer_crypto(cc->inode),
>  	};
>  	struct dnode_of_data dn;
>  	struct node_info ni;
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 326c63879ddc..6955ea6fa1b6 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -14,6 +14,7 @@
>  #include <linux/pagevec.h>
>  #include <linux/blkdev.h>
>  #include <linux/bio.h>
> +#include <linux/blk-crypto.h>
>  #include <linux/swap.h>
>  #include <linux/prefetch.h>
>  #include <linux/uio.h>
> @@ -459,6 +460,33 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages)
>  	return bio;
>  }
>  
> +static void f2fs_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
> +				  pgoff_t first_idx,
> +				  const struct f2fs_io_info *fio,
> +				  gfp_t gfp_mask)
> +{
> +	/*
> +	 * The f2fs garbage collector sets ->encrypted_page when it wants to
> +	 * read/write raw data without encryption.
> +	 */
> +	if (!fio || !fio->encrypted_page)
> +		fscrypt_set_bio_crypt_ctx(bio, inode, first_idx, gfp_mask);
> +}
> +
> +static bool f2fs_crypt_mergeable_bio(struct bio *bio, const struct inode *inode,
> +				     pgoff_t next_idx,
> +				     const struct f2fs_io_info *fio)
> +{
> +	/*
> +	 * The f2fs garbage collector sets ->encrypted_page when it wants to
> +	 * read/write raw data without encryption.
> +	 */
> +	if (fio && fio->encrypted_page)
> +		return !bio_has_crypt_ctx(bio);
> +
> +	return fscrypt_mergeable_bio(bio, inode, next_idx);
> +}
> +
>  static inline void __submit_bio(struct f2fs_sb_info *sbi,
>  				struct bio *bio, enum page_type type)
>  {
> @@ -684,6 +712,9 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>  	/* Allocate a new bio */
>  	bio = __bio_alloc(fio, 1);
>  
> +	f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
> +			       fio->page->index, fio, GFP_NOIO);
> +
>  	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
>  		bio_put(bio);
>  		return -EFAULT;
> @@ -763,9 +794,10 @@ static void del_bio_entry(struct bio_entry *be)
>  	kmem_cache_free(bio_entry_slab, be);
>  }
>  
> -static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> +static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
>  							struct page *page)
>  {
> +	struct f2fs_sb_info *sbi = fio->sbi;
>  	enum temp_type temp;
>  	bool found = false;
>  	int ret = -EAGAIN;
> @@ -782,13 +814,18 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
>  
>  			found = true;
>  
> -			if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> -							PAGE_SIZE) {
> +			if (page_is_mergeable(sbi, *bio, *fio->last_block,
> +					fio->new_blkaddr) &&
> +			    f2fs_crypt_mergeable_bio(*bio,
> +					fio->page->mapping->host,
> +					fio->page->index, fio) &&
> +			    bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> +					PAGE_SIZE) {
>  				ret = 0;
>  				break;
>  			}
>  
> -			/* bio is full */
> +			 /* page can't be merged into bio; submit the bio */
>  			del_bio_entry(be);
>  			__submit_bio(sbi, *bio, DATA);
>  			break;
> @@ -873,18 +910,17 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
>  	trace_f2fs_submit_page_bio(page, fio);
>  	f2fs_trace_ios(fio, 0);
>  
> -	if (bio && !page_is_mergeable(fio->sbi, bio, *fio->last_block,
> -						fio->new_blkaddr))
> -		f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);
>  alloc_new:
>  	if (!bio) {
>  		bio = __bio_alloc(fio, BIO_MAX_PAGES);
>  		__attach_io_flag(fio);
> +		f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
> +				       fio->page->index, fio, GFP_NOIO);
>  		bio_set_op_attrs(bio, fio->op, fio->op_flags);
>  
>  		add_bio_entry(fio->sbi, bio, page, fio->temp);
>  	} else {
> -		if (add_ipu_page(fio->sbi, &bio, page))
> +		if (add_ipu_page(fio, &bio, page))
>  			goto alloc_new;
>  	}
>  
> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>  
>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>  
> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
> -			io->last_block_in_bio, fio->new_blkaddr))
> +	if (io->bio &&
> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
> +			      fio->new_blkaddr) ||
> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
> +				       fio->page->index, fio)))
>  		__submit_merged_bio(io);
>  alloc_new:
>  	if (io->bio == NULL) {
> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>  			goto skip;
>  		}
>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
> +				       fio->page->index, fio, GFP_NOIO);
>  		io->fio = *fio;
>  	}
>  
> @@ -993,11 +1034,14 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
>  								for_write);
>  	if (!bio)
>  		return ERR_PTR(-ENOMEM);
> +
> +	f2fs_set_bio_crypt_ctx(bio, inode, first_idx, NULL, GFP_NOFS);
> +
>  	f2fs_target_device(sbi, blkaddr, bio);
>  	bio->bi_end_io = f2fs_read_end_io;
>  	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
>  
> -	if (f2fs_encrypted_file(inode))
> +	if (fscrypt_inode_uses_fs_layer_crypto(inode))
>  		post_read_steps |= 1 << STEP_DECRYPT;
>  	if (f2fs_compressed_file(inode))
>  		post_read_steps |= 1 << STEP_DECOMPRESS_NOWQ;
> @@ -2073,8 +2117,9 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>  	 * This page will go to BIO.  Do we need to send this
>  	 * BIO off first?
>  	 */
> -	if (bio && !page_is_mergeable(F2FS_I_SB(inode), bio,
> -				*last_block_in_bio, block_nr)) {
> +	if (bio && (!page_is_mergeable(F2FS_I_SB(inode), bio,
> +				       *last_block_in_bio, block_nr) ||
> +		    !f2fs_crypt_mergeable_bio(bio, inode, page->index, NULL))) {
>  submit_and_realloc:
>  		__submit_bio(F2FS_I_SB(inode), bio, DATA);
>  		bio = NULL;
> @@ -2204,8 +2249,9 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>  		blkaddr = data_blkaddr(dn.inode, dn.node_page,
>  						dn.ofs_in_node + i + 1);
>  
> -		if (bio && !page_is_mergeable(sbi, bio,
> -					*last_block_in_bio, blkaddr)) {
> +		if (bio && (!page_is_mergeable(sbi, bio,
> +					*last_block_in_bio, blkaddr) ||
> +		    !f2fs_crypt_mergeable_bio(bio, inode, page->index, NULL))) {
>  submit_and_realloc:
>  			__submit_bio(sbi, bio, DATA);
>  			bio = NULL;
> @@ -2421,6 +2467,9 @@ int f2fs_encrypt_one_page(struct f2fs_io_info *fio)
>  	/* wait for GCed page writeback via META_MAPPING */
>  	f2fs_wait_on_block_writeback(inode, fio->old_blkaddr);
>  
> +	if (fscrypt_inode_uses_inline_crypto(inode))
> +		return 0;
> +
>  retry_encrypt:
>  	fio->encrypted_page = fscrypt_encrypt_pagecache_blocks(page,
>  					PAGE_SIZE, 0, gfp_flags);
> @@ -2594,7 +2643,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  			f2fs_unlock_op(fio->sbi);
>  		err = f2fs_inplace_write_data(fio);
>  		if (err) {
> -			if (f2fs_encrypted_file(inode))
> +			if (fscrypt_inode_uses_fs_layer_crypto(inode))
>  				fscrypt_finalize_bounce_page(&fio->encrypted_page);
>  			if (PageWriteback(page))
>  				end_page_writeback(page);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 20e56b0fa46a..3621969b2665 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -138,6 +138,7 @@ enum {
>  	Opt_alloc,
>  	Opt_fsync,
>  	Opt_test_dummy_encryption,
> +	Opt_inlinecrypt,
>  	Opt_checkpoint_disable,
>  	Opt_checkpoint_disable_cap,
>  	Opt_checkpoint_disable_cap_perc,
> @@ -204,6 +205,7 @@ static match_table_t f2fs_tokens = {
>  	{Opt_fsync, "fsync_mode=%s"},
>  	{Opt_test_dummy_encryption, "test_dummy_encryption=%s"},
>  	{Opt_test_dummy_encryption, "test_dummy_encryption"},
> +	{Opt_inlinecrypt, "inlinecrypt"},
>  	{Opt_checkpoint_disable, "checkpoint=disable"},
>  	{Opt_checkpoint_disable_cap, "checkpoint=disable:%u"},
>  	{Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"},
> @@ -833,6 +835,13 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>  			if (ret)
>  				return ret;
>  			break;
> +		case Opt_inlinecrypt:
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> +			sb->s_flags |= SB_INLINECRYPT;
> +#else
> +			f2fs_info(sbi, "inline encryption not supported");
> +#endif
> +			break;
>  		case Opt_checkpoint_disable_cap_perc:
>  			if (args->from && match_int(args, &arg))
>  				return -EINVAL;
> @@ -1624,6 +1633,8 @@ static void default_options(struct f2fs_sb_info *sbi)
>  	F2FS_OPTION(sbi).compress_ext_cnt = 0;
>  	F2FS_OPTION(sbi).bggc_mode = BGGC_MODE_ON;
>  
> +	sbi->sb->s_flags &= ~SB_INLINECRYPT;
> +
>  	set_opt(sbi, INLINE_XATTR);
>  	set_opt(sbi, INLINE_DATA);
>  	set_opt(sbi, INLINE_DENTRY);
> @@ -2470,6 +2481,25 @@ static void f2fs_get_ino_and_lblk_bits(struct super_block *sb,
>  	*lblk_bits_ret = 8 * sizeof(block_t);
>  }
>  
> +static int f2fs_get_num_devices(struct super_block *sb)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_SB(sb);
> +
> +	if (f2fs_is_multi_device(sbi))
> +		return sbi->s_ndevs;
> +	return 1;
> +}
> +
> +static void f2fs_get_devices(struct super_block *sb,
> +			     struct request_queue **devs)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_SB(sb);
> +	int i;
> +
> +	for (i = 0; i < sbi->s_ndevs; i++)
> +		devs[i] = bdev_get_queue(FDEV(i).bdev);
> +}
> +
>  static const struct fscrypt_operations f2fs_cryptops = {
>  	.key_prefix		= "f2fs:",
>  	.get_context		= f2fs_get_context,
> @@ -2479,6 +2509,8 @@ static const struct fscrypt_operations f2fs_cryptops = {
>  	.max_namelen		= F2FS_NAME_LEN,
>  	.has_stable_inodes	= f2fs_has_stable_inodes,
>  	.get_ino_and_lblk_bits	= f2fs_get_ino_and_lblk_bits,
> +	.get_num_devices	= f2fs_get_num_devices,
> +	.get_devices		= f2fs_get_devices,
>  };
>  #endif
>  
> -- 
> 2.27.0.290.gba653c62da-goog

On 2020/6/17 15:57, Satya Tangirala wrote:
> Wire up f2fs to support inline encryption via the helper functions which
> fs/crypto/ now provides.  This includes:
> 
> - Adding a mount option 'inlinecrypt' which enables inline encryption
>   on encrypted files where it can be used.
> 
> - Setting the bio_crypt_ctx on bios that will be submitted to an
>   inline-encrypted file.
> 
> - Not adding logically discontiguous data to bios that will be submitted
>   to an inline-encrypted file.
> 
> - Not doing filesystem-layer crypto on inline-encrypted files.
> 
> This patch includes a fix for a race during IPU by
> Sahitya Tummala <stummala@codeaurora.org>
> 
> Co-developed-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  Documentation/filesystems/f2fs.rst |  7 ++-
>  fs/f2fs/compress.c                 |  2 +-
>  fs/f2fs/data.c                     | 81 ++++++++++++++++++++++++------
>  fs/f2fs/super.c                    | 32 ++++++++++++
>  4 files changed, 104 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst
> index 099d45ac8d8f..4dc36143ff82 100644
> --- a/Documentation/filesystems/f2fs.rst
> +++ b/Documentation/filesystems/f2fs.rst
> @@ -258,7 +258,12 @@ compress_extension=%s  Support adding specified extension, so that f2fs can enab
>                         on compression extension list and enable compression on
>                         these file by default rather than to enable it via ioctl.
>                         For other files, we can still enable compression via ioctl.
> -====================== ============================================================
> +inlinecrypt
> +                       Encrypt/decrypt the contents of encrypted files using the
> +                       blk-crypto framework rather than filesystem-layer encryption.
> +                       This allows the use of inline encryption hardware. The on-disk
> +                       format is unaffected. For more details, see
> +                       Documentation/block/inline-encryption.rst.
>  
>  Debugfs Entries
>  ===============
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 1e02a8c106b0..29e50fbe7eca 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1086,7 +1086,7 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>  		.submitted = false,
>  		.io_type = io_type,
>  		.io_wbc = wbc,
> -		.encrypted = f2fs_encrypted_file(cc->inode),
> +		.encrypted = fscrypt_inode_uses_fs_layer_crypto(cc->inode),
>  	};
>  	struct dnode_of_data dn;
>  	struct node_info ni;
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 326c63879ddc..6955ea6fa1b6 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -14,6 +14,7 @@
>  #include <linux/pagevec.h>
>  #include <linux/blkdev.h>
>  #include <linux/bio.h>
> +#include <linux/blk-crypto.h>
>  #include <linux/swap.h>
>  #include <linux/prefetch.h>
>  #include <linux/uio.h>
> @@ -459,6 +460,33 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages)
>  	return bio;
>  }
>  
> +static void f2fs_set_bio_crypt_ctx(struct bio *bio, const struct inode *inode,
> +				  pgoff_t first_idx,
> +				  const struct f2fs_io_info *fio,
> +				  gfp_t gfp_mask)
> +{
> +	/*
> +	 * The f2fs garbage collector sets ->encrypted_page when it wants to
> +	 * read/write raw data without encryption.
> +	 */
> +	if (!fio || !fio->encrypted_page)
> +		fscrypt_set_bio_crypt_ctx(bio, inode, first_idx, gfp_mask);
> +}
> +
> +static bool f2fs_crypt_mergeable_bio(struct bio *bio, const struct inode *inode,
> +				     pgoff_t next_idx,
> +				     const struct f2fs_io_info *fio)
> +{
> +	/*
> +	 * The f2fs garbage collector sets ->encrypted_page when it wants to
> +	 * read/write raw data without encryption.
> +	 */
> +	if (fio && fio->encrypted_page)
> +		return !bio_has_crypt_ctx(bio);
> +
> +	return fscrypt_mergeable_bio(bio, inode, next_idx);
> +}
> +
>  static inline void __submit_bio(struct f2fs_sb_info *sbi,
>  				struct bio *bio, enum page_type type)
>  {
> @@ -684,6 +712,9 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>  	/* Allocate a new bio */
>  	bio = __bio_alloc(fio, 1);
>  
> +	f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
> +			       fio->page->index, fio, GFP_NOIO);
> +
>  	if (bio_add_page(bio, page, PAGE_SIZE, 0) < PAGE_SIZE) {
>  		bio_put(bio);
>  		return -EFAULT;
> @@ -763,9 +794,10 @@ static void del_bio_entry(struct bio_entry *be)
>  	kmem_cache_free(bio_entry_slab, be);
>  }
>  
> -static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> +static int add_ipu_page(struct f2fs_io_info *fio, struct bio **bio,
>  							struct page *page)
>  {
> +	struct f2fs_sb_info *sbi = fio->sbi;
>  	enum temp_type temp;
>  	bool found = false;
>  	int ret = -EAGAIN;
> @@ -782,13 +814,18 @@ static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
>  
>  			found = true;
>  
> -			if (bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> -							PAGE_SIZE) {
> +			if (page_is_mergeable(sbi, *bio, *fio->last_block,
> +					fio->new_blkaddr) &&
> +			    f2fs_crypt_mergeable_bio(*bio,
> +					fio->page->mapping->host,
> +					fio->page->index, fio) &&
> +			    bio_add_page(*bio, page, PAGE_SIZE, 0) ==
> +					PAGE_SIZE) {
>  				ret = 0;
>  				break;
>  			}
>  
> -			/* bio is full */
> +			 /* page can't be merged into bio; submit the bio */

One more unneeded space before '/'.

>  			del_bio_entry(be);
>  			__submit_bio(sbi, *bio, DATA);
>  			break;
> @@ -873,18 +910,17 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
>  	trace_f2fs_submit_page_bio(page, fio);
>  	f2fs_trace_ios(fio, 0);
>  
> -	if (bio && !page_is_mergeable(fio->sbi, bio, *fio->last_block,
> -						fio->new_blkaddr))
> -		f2fs_submit_merged_ipu_write(fio->sbi, &bio, NULL);

I prefer to keep this condition for non-inlinecrypt case to avoid unneeded lock
contention in add_ipu_page().

>  alloc_new:
>  	if (!bio) {
>  		bio = __bio_alloc(fio, BIO_MAX_PAGES);
>  		__attach_io_flag(fio);
> +		f2fs_set_bio_crypt_ctx(bio, fio->page->mapping->host,
> +				       fio->page->index, fio, GFP_NOIO);
>  		bio_set_op_attrs(bio, fio->op, fio->op_flags);
>  
>  		add_bio_entry(fio->sbi, bio, page, fio->temp);
>  	} else {
> -		if (add_ipu_page(fio->sbi, &bio, page))
> +		if (add_ipu_page(fio, &bio, page))
>  			goto alloc_new;
>  	}
>  
> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>  
>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>  
> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
> -			io->last_block_in_bio, fio->new_blkaddr))
> +	if (io->bio &&
> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
> +			      fio->new_blkaddr) ||
> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
> +				       fio->page->index, fio)))

bio_page->index, fio)))

>  		__submit_merged_bio(io);
>  alloc_new:
>  	if (io->bio == NULL) {
> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>  			goto skip;
>  		}
>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
> +				       fio->page->index, fio, GFP_NOIO);

bio_page->index, fio, GFP_NOIO);

Thanks,

>  		io->fio = *fio;
>  	}
>  
> @@ -993,11 +1034,14 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
>  								for_write);
>  	if (!bio)
>  		return ERR_PTR(-ENOMEM);
> +
> +	f2fs_set_bio_crypt_ctx(bio, inode, first_idx, NULL, GFP_NOFS);
> +
>  	f2fs_target_device(sbi, blkaddr, bio);
>  	bio->bi_end_io = f2fs_read_end_io;
>  	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
>  
> -	if (f2fs_encrypted_file(inode))
> +	if (fscrypt_inode_uses_fs_layer_crypto(inode))
>  		post_read_steps |= 1 << STEP_DECRYPT;
>  	if (f2fs_compressed_file(inode))
>  		post_read_steps |= 1 << STEP_DECOMPRESS_NOWQ;
> @@ -2073,8 +2117,9 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>  	 * This page will go to BIO.  Do we need to send this
>  	 * BIO off first?
>  	 */
> -	if (bio && !page_is_mergeable(F2FS_I_SB(inode), bio,
> -				*last_block_in_bio, block_nr)) {
> +	if (bio && (!page_is_mergeable(F2FS_I_SB(inode), bio,
> +				       *last_block_in_bio, block_nr) ||
> +		    !f2fs_crypt_mergeable_bio(bio, inode, page->index, NULL))) {
>  submit_and_realloc:
>  		__submit_bio(F2FS_I_SB(inode), bio, DATA);
>  		bio = NULL;
> @@ -2204,8 +2249,9 @@ int f2fs_read_multi_pages(struct compress_ctx *cc, struct bio **bio_ret,
>  		blkaddr = data_blkaddr(dn.inode, dn.node_page,
>  						dn.ofs_in_node + i + 1);
>  
> -		if (bio && !page_is_mergeable(sbi, bio,
> -					*last_block_in_bio, blkaddr)) {
> +		if (bio && (!page_is_mergeable(sbi, bio,
> +					*last_block_in_bio, blkaddr) ||
> +		    !f2fs_crypt_mergeable_bio(bio, inode, page->index, NULL))) {
>  submit_and_realloc:
>  			__submit_bio(sbi, bio, DATA);
>  			bio = NULL;
> @@ -2421,6 +2467,9 @@ int f2fs_encrypt_one_page(struct f2fs_io_info *fio)
>  	/* wait for GCed page writeback via META_MAPPING */
>  	f2fs_wait_on_block_writeback(inode, fio->old_blkaddr);
>  
> +	if (fscrypt_inode_uses_inline_crypto(inode))
> +		return 0;
> +
>  retry_encrypt:
>  	fio->encrypted_page = fscrypt_encrypt_pagecache_blocks(page,
>  					PAGE_SIZE, 0, gfp_flags);
> @@ -2594,7 +2643,7 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>  			f2fs_unlock_op(fio->sbi);
>  		err = f2fs_inplace_write_data(fio);
>  		if (err) {
> -			if (f2fs_encrypted_file(inode))
> +			if (fscrypt_inode_uses_fs_layer_crypto(inode))
>  				fscrypt_finalize_bounce_page(&fio->encrypted_page);
>  			if (PageWriteback(page))
>  				end_page_writeback(page);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 20e56b0fa46a..3621969b2665 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -138,6 +138,7 @@ enum {
>  	Opt_alloc,
>  	Opt_fsync,
>  	Opt_test_dummy_encryption,
> +	Opt_inlinecrypt,
>  	Opt_checkpoint_disable,
>  	Opt_checkpoint_disable_cap,
>  	Opt_checkpoint_disable_cap_perc,
> @@ -204,6 +205,7 @@ static match_table_t f2fs_tokens = {
>  	{Opt_fsync, "fsync_mode=%s"},
>  	{Opt_test_dummy_encryption, "test_dummy_encryption=%s"},
>  	{Opt_test_dummy_encryption, "test_dummy_encryption"},
> +	{Opt_inlinecrypt, "inlinecrypt"},
>  	{Opt_checkpoint_disable, "checkpoint=disable"},
>  	{Opt_checkpoint_disable_cap, "checkpoint=disable:%u"},
>  	{Opt_checkpoint_disable_cap_perc, "checkpoint=disable:%u%%"},
> @@ -833,6 +835,13 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>  			if (ret)
>  				return ret;
>  			break;
> +		case Opt_inlinecrypt:
> +#ifdef CONFIG_FS_ENCRYPTION_INLINE_CRYPT
> +			sb->s_flags |= SB_INLINECRYPT;
> +#else
> +			f2fs_info(sbi, "inline encryption not supported");
> +#endif
> +			break;
>  		case Opt_checkpoint_disable_cap_perc:
>  			if (args->from && match_int(args, &arg))
>  				return -EINVAL;
> @@ -1624,6 +1633,8 @@ static void default_options(struct f2fs_sb_info *sbi)
>  	F2FS_OPTION(sbi).compress_ext_cnt = 0;
>  	F2FS_OPTION(sbi).bggc_mode = BGGC_MODE_ON;
>  
> +	sbi->sb->s_flags &= ~SB_INLINECRYPT;
> +
>  	set_opt(sbi, INLINE_XATTR);
>  	set_opt(sbi, INLINE_DATA);
>  	set_opt(sbi, INLINE_DENTRY);
> @@ -2470,6 +2481,25 @@ static void f2fs_get_ino_and_lblk_bits(struct super_block *sb,
>  	*lblk_bits_ret = 8 * sizeof(block_t);
>  }
>  
> +static int f2fs_get_num_devices(struct super_block *sb)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_SB(sb);
> +
> +	if (f2fs_is_multi_device(sbi))
> +		return sbi->s_ndevs;
> +	return 1;
> +}
> +
> +static void f2fs_get_devices(struct super_block *sb,
> +			     struct request_queue **devs)
> +{
> +	struct f2fs_sb_info *sbi = F2FS_SB(sb);
> +	int i;
> +
> +	for (i = 0; i < sbi->s_ndevs; i++)
> +		devs[i] = bdev_get_queue(FDEV(i).bdev);
> +}
> +
>  static const struct fscrypt_operations f2fs_cryptops = {
>  	.key_prefix		= "f2fs:",
>  	.get_context		= f2fs_get_context,
> @@ -2479,6 +2509,8 @@ static const struct fscrypt_operations f2fs_cryptops = {
>  	.max_namelen		= F2FS_NAME_LEN,
>  	.has_stable_inodes	= f2fs_has_stable_inodes,
>  	.get_ino_and_lblk_bits	= f2fs_get_ino_and_lblk_bits,
> +	.get_num_devices	= f2fs_get_num_devices,
> +	.get_devices		= f2fs_get_devices,
>  };
>  #endif
>  
>

Hi Chao,

On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
> > @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> >  
> >  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> >  
> > -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
> > -			io->last_block_in_bio, fio->new_blkaddr))
> > +	if (io->bio &&
> > +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
> > +			      fio->new_blkaddr) ||
> > +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
> > +				       fio->page->index, fio)))
> 
> bio_page->index, fio)))
> 
> >  		__submit_merged_bio(io);
> >  alloc_new:
> >  	if (io->bio == NULL) {
> > @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> >  			goto skip;
> >  		}
> >  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
> > +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
> > +				       fio->page->index, fio, GFP_NOIO);
> 
> bio_page->index, fio, GFP_NOIO);
> 

We're using ->mapping->host and ->index.  Ordinarily that would mean the page
needs to be a pagecache page.  But bio_page can also be a compressed page or a
bounce page containing fs-layer encrypted contents.

Is your suggestion to keep using fio->page->mapping->host (since encrypted pages
don't have a mapping), but start using bio_page->index (since f2fs apparently
*does* set ->index for compressed pages, and if the file uses fs-layer
encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?

Does this mean the code is currently broken for compression + inline encryption
because it's using the wrong ->index?  I think the answer is no, since
f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
pages along with the compressed pages.  In that case, your suggestion would be a
cleanup rather than a fix?

It would be helpful if there was an f2fs mount option to auto-enable compression
on all files (similar to how test_dummy_encryption auto-enables encryption on
all files) so that it could be tested more easily.

- Eric

On 06/18, Eric Biggers wrote:
> Hi Chao,
> 
> On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
> > > @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> > >  
> > >  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> > >  
> > > -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
> > > -			io->last_block_in_bio, fio->new_blkaddr))
> > > +	if (io->bio &&
> > > +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
> > > +			      fio->new_blkaddr) ||
> > > +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
> > > +				       fio->page->index, fio)))
> > 
> > bio_page->index, fio)))
> > 
> > >  		__submit_merged_bio(io);
> > >  alloc_new:
> > >  	if (io->bio == NULL) {
> > > @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> > >  			goto skip;
> > >  		}
> > >  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
> > > +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
> > > +				       fio->page->index, fio, GFP_NOIO);
> > 
> > bio_page->index, fio, GFP_NOIO);
> > 
> 
> We're using ->mapping->host and ->index.  Ordinarily that would mean the page
> needs to be a pagecache page.  But bio_page can also be a compressed page or a
> bounce page containing fs-layer encrypted contents.
> 
> Is your suggestion to keep using fio->page->mapping->host (since encrypted pages
> don't have a mapping), but start using bio_page->index (since f2fs apparently
> *does* set ->index for compressed pages, and if the file uses fs-layer
> encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?
> 
> Does this mean the code is currently broken for compression + inline encryption
> because it's using the wrong ->index?  I think the answer is no, since
> f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
> pages along with the compressed pages.  In that case, your suggestion would be a
> cleanup rather than a fix?
> 
> It would be helpful if there was an f2fs mount option to auto-enable compression
> on all files (similar to how test_dummy_encryption auto-enables encryption on
> all files) so that it could be tested more easily.

Eric, you can use "-o compress_extension=*".

> 
> - Eric

On Thu, Jun 18, 2020 at 12:28:04PM -0700, Jaegeuk Kim wrote:
> > 
> > It would be helpful if there was an f2fs mount option to auto-enable compression
> > on all files (similar to how test_dummy_encryption auto-enables encryption on
> > all files) so that it could be tested more easily.
> 
> Eric, you can use "-o compress_extension=*".
> 

Okay, great!  This isn't mentioned in the documentation:

compress_extension=%s  Support adding specified extension, so that f2fs can enable
                       compression on those corresponding files, e.g. if all files
                       with '.ext' has high compression rate, we can set the '.ext'
                       on compression extension list and enable compression on
                       these file by default rather than to enable it via ioctl.
                       For other files, we can still enable compression via ioctl.

On Wed, Jun 17, 2020 at 07:57:31AM +0000, Satya Tangirala wrote:
> Wire up f2fs to support inline encryption via the helper functions which
> fs/crypto/ now provides.  This includes:
> 
> - Adding a mount option 'inlinecrypt' which enables inline encryption
>   on encrypted files where it can be used.
> 
> - Setting the bio_crypt_ctx on bios that will be submitted to an
>   inline-encrypted file.
> 
> - Not adding logically discontiguous data to bios that will be submitted
>   to an inline-encrypted file.
> 
> - Not doing filesystem-layer crypto on inline-encrypted files.
> 
> This patch includes a fix for a race during IPU by
> Sahitya Tummala <stummala@codeaurora.org>
> 
> Co-developed-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  Documentation/filesystems/f2fs.rst |  7 ++-
>  fs/f2fs/compress.c                 |  2 +-
>  fs/f2fs/data.c                     | 81 ++++++++++++++++++++++++------
>  fs/f2fs/super.c                    | 32 ++++++++++++
>  4 files changed, 104 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/filesystems/f2fs.rst b/Documentation/filesystems/f2fs.rst
> index 099d45ac8d8f..4dc36143ff82 100644
> --- a/Documentation/filesystems/f2fs.rst
> +++ b/Documentation/filesystems/f2fs.rst
> @@ -258,7 +258,12 @@ compress_extension=%s  Support adding specified extension, so that f2fs can enab
>                         on compression extension list and enable compression on
>                         these file by default rather than to enable it via ioctl.
>                         For other files, we can still enable compression via ioctl.
> -====================== ============================================================

The above line being deleted marks the end of a table, so it shouldn't be
deleted (it should go after the part below).

> +inlinecrypt
> +                       Encrypt/decrypt the contents of encrypted files using the
> +                       blk-crypto framework rather than filesystem-layer encryption.
> +                       This allows the use of inline encryption hardware. The on-disk
> +                       format is unaffected. For more details, see
> +                       Documentation/block/inline-encryption.rst.

Like I commented on one of the commit messages -- this doesn't make it clear
what happens in cases where blk-crypto can't be used.  Maybe just replace:
"Encrypt/decrypt" => "When possible, encrypt/decrypt".

Likewise for the ext4 documentation for this same mount option.

- Eric

Hi Eric,

On 2020/6/19 2:13, Eric Biggers wrote:
> Hi Chao,
> 
> On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
>>> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>  
>>>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>>>  
>>> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
>>> -			io->last_block_in_bio, fio->new_blkaddr))
>>> +	if (io->bio &&
>>> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
>>> +			      fio->new_blkaddr) ||
>>> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
>>> +				       fio->page->index, fio)))
>>
>> bio_page->index, fio)))
>>
>>>  		__submit_merged_bio(io);
>>>  alloc_new:
>>>  	if (io->bio == NULL) {
>>> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>  			goto skip;
>>>  		}
>>>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
>>> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
>>> +				       fio->page->index, fio, GFP_NOIO);
>>
>> bio_page->index, fio, GFP_NOIO);
>>
> 
> We're using ->mapping->host and ->index.  Ordinarily that would mean the page
> needs to be a pagecache page.  But bio_page can also be a compressed page or a
> bounce page containing fs-layer encrypted contents.

I'm concerning about compression + inlinecrypt case.

> 
> Is your suggestion to keep using fio->page->mapping->host (since encrypted pages

Yup,

> don't have a mapping), but start using bio_page->index (since f2fs apparently

I meant that we need to use bio_page->index as tweak value in write path to
keep consistent as we did in read path, otherwise we may read the wrong
decrypted data later to incorrect tweak value.

- f2fs_read_multi_pages (only comes from compression inode)
 - f2fs_alloc_dic
  - f2fs_set_compressed_page(page, cc->inode,
					start_idx + i + 1, dic);
                                        ^^^^^^^^^^^^^^^^^
  - dic->cpages[i] = page;
 - for ()
     struct page *page = dic->cpages[i];
     if (!bio)
       - f2fs_grab_read_bio(..., page->index,..)
        - f2fs_set_bio_crypt_ctx(..., first_idx, ..)   /* first_idx == cpage->index */

You can see that cpage->index was set to page->index + 1, that's why we need
to use one of cpage->index/page->index as tweak value all the time rather than
using both index mixed in read/write path.

But note that for fs-layer encryption, we have used cpage->index as tweak value,
so here I suggest we can keep consistent to use cpage->index in inlinecrypt case.

> *does* set ->index for compressed pages, and if the file uses fs-layer
> encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?
> 
> Does this mean the code is currently broken for compression + inline encryption
> because it's using the wrong ->index?  I think the answer is no, since

I guess it's broken now for compression + inlinecrypt case.

> f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
> pages along with the compressed pages.  In that case, your suggestion would be a
> cleanup rather than a fix?

That's a fix.

> 
> It would be helpful if there was an f2fs mount option to auto-enable compression
> on all files (similar to how test_dummy_encryption auto-enables encryption on
> all files) so that it could be tested more easily.

Agreed.

Previously I changed mkfs to allow to add compression flag to root inode for
compression test. :P

Thanks,

> 
> - Eric
> .
>

On 2020/6/19 3:28, Jaegeuk Kim wrote:
> On 06/18, Eric Biggers wrote:
>> Hi Chao,
>>
>> On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
>>>> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>>  
>>>>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>>>>  
>>>> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
>>>> -			io->last_block_in_bio, fio->new_blkaddr))
>>>> +	if (io->bio &&
>>>> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
>>>> +			      fio->new_blkaddr) ||
>>>> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
>>>> +				       fio->page->index, fio)))
>>>
>>> bio_page->index, fio)))
>>>
>>>>  		__submit_merged_bio(io);
>>>>  alloc_new:
>>>>  	if (io->bio == NULL) {
>>>> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>>  			goto skip;
>>>>  		}
>>>>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
>>>> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
>>>> +				       fio->page->index, fio, GFP_NOIO);
>>>
>>> bio_page->index, fio, GFP_NOIO);
>>>
>>
>> We're using ->mapping->host and ->index.  Ordinarily that would mean the page
>> needs to be a pagecache page.  But bio_page can also be a compressed page or a
>> bounce page containing fs-layer encrypted contents.
>>
>> Is your suggestion to keep using fio->page->mapping->host (since encrypted pages
>> don't have a mapping), but start using bio_page->index (since f2fs apparently
>> *does* set ->index for compressed pages, and if the file uses fs-layer
>> encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?
>>
>> Does this mean the code is currently broken for compression + inline encryption
>> because it's using the wrong ->index?  I think the answer is no, since
>> f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
>> pages along with the compressed pages.  In that case, your suggestion would be a
>> cleanup rather than a fix?
>>
>> It would be helpful if there was an f2fs mount option to auto-enable compression
>> on all files (similar to how test_dummy_encryption auto-enables encryption on
>> all files) so that it could be tested more easily.
> 
> Eric, you can use "-o compress_extension=*".

Cool, we should update documentation when merge that patch...

> 
>>
>> - Eric
> .
>

On Fri, Jun 19, 2020 at 10:39:34AM +0800, Chao Yu wrote:
> Hi Eric,
> 
> On 2020/6/19 2:13, Eric Biggers wrote:
> > Hi Chao,
> > 
> > On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
> >>> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> >>>  
> >>>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
> >>>  
> >>> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
> >>> -			io->last_block_in_bio, fio->new_blkaddr))
> >>> +	if (io->bio &&
> >>> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
> >>> +			      fio->new_blkaddr) ||
> >>> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
> >>> +				       fio->page->index, fio)))
> >>
> >> bio_page->index, fio)))
> >>
> >>>  		__submit_merged_bio(io);
> >>>  alloc_new:
> >>>  	if (io->bio == NULL) {
> >>> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
> >>>  			goto skip;
> >>>  		}
> >>>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
> >>> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
> >>> +				       fio->page->index, fio, GFP_NOIO);
> >>
> >> bio_page->index, fio, GFP_NOIO);
> >>
> > 
> > We're using ->mapping->host and ->index.  Ordinarily that would mean the page
> > needs to be a pagecache page.  But bio_page can also be a compressed page or a
> > bounce page containing fs-layer encrypted contents.
> 
> I'm concerning about compression + inlinecrypt case.
> 
> > 
> > Is your suggestion to keep using fio->page->mapping->host (since encrypted pages
> 
> Yup,
> 
> > don't have a mapping), but start using bio_page->index (since f2fs apparently
> 
> I meant that we need to use bio_page->index as tweak value in write path to
> keep consistent as we did in read path, otherwise we may read the wrong
> decrypted data later to incorrect tweak value.
> 
> - f2fs_read_multi_pages (only comes from compression inode)
>  - f2fs_alloc_dic
>   - f2fs_set_compressed_page(page, cc->inode,
> 					start_idx + i + 1, dic);
>                                         ^^^^^^^^^^^^^^^^^
>   - dic->cpages[i] = page;
>  - for ()
>      struct page *page = dic->cpages[i];
>      if (!bio)
>        - f2fs_grab_read_bio(..., page->index,..)
>         - f2fs_set_bio_crypt_ctx(..., first_idx, ..)   /* first_idx == cpage->index */
> 
> You can see that cpage->index was set to page->index + 1, that's why we need
> to use one of cpage->index/page->index as tweak value all the time rather than
> using both index mixed in read/write path.
> 
> But note that for fs-layer encryption, we have used cpage->index as tweak value,
> so here I suggest we can keep consistent to use cpage->index in inlinecrypt case.

Yes, inlinecrypt mustn't change the ciphertext that gets written to disk.

> 
> > *does* set ->index for compressed pages, and if the file uses fs-layer
> > encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?
> > 
> > Does this mean the code is currently broken for compression + inline encryption
> > because it's using the wrong ->index?  I think the answer is no, since
> 
> I guess it's broken now for compression + inlinecrypt case.
> 
> > f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
> > pages along with the compressed pages.  In that case, your suggestion would be a
> > cleanup rather than a fix?
> 
> That's a fix.
> 

FWIW, I tested this, and it actually works both before and after your suggested
change.  The reason is that f2fs_write_compressed_pages() actually passes the
pagecache pages sequentially starting at 'start_idx_of_cluster(cc) + 1' for the
length of the compressed cluster.  That matches the '+ 1' adjustment elsewhere,
so we have fio->page->index == bio_page->index.

I personally think the way the f2fs compression code works is really confusing.
Compressed pages don't have a 1:1 correspondence to pagecache pages, so there
should *not* be a pagecache page passed around when writing a compressed page.

Anyway, here's the test script I used in case anyone else wants to use it.  But
we really need to write a proper f2fs compression + encryption test for xfstests
which decrypts and decompresses a file in userspace and verifies we get back the
original data.  (There are already ciphertext verification tests, but they don't
cover compression.)  Note that this test is needed even for the filesystem-layer
encryption which is currently supported.

#!/bin/bash

set -e

DEV=/dev/vdb

umount /mnt &> /dev/null || true
mkfs.f2fs -f -O encrypt,compression,extra_attr $DEV
head -c 1000000 /dev/zero > /tmp/testdata

for opt1 in '-o inlinecrypt' ''; do
        mount $DEV /mnt $opt1
        rm -rf /mnt/.fscrypt /mnt/dir
        fscrypt setup /mnt
        mkdir /mnt/dir
        chattr +c /mnt/dir
        echo hunter2 | fscrypt encrypt /mnt/dir --quiet --source=custom_passphrase --name=secret
        cp /tmp/testdata /mnt/dir/file
        umount /mnt
        for opt2 in '-o inlinecrypt' ''; do
                mount $DEV /mnt $opt2
                echo hunter2 | fscrypt unlock /mnt/dir --quiet
                cmp /mnt/dir/file /tmp/testdata
                umount /mnt
        done
done

On 2020/6/19 12:20, Eric Biggers wrote:
> On Fri, Jun 19, 2020 at 10:39:34AM +0800, Chao Yu wrote:
>> Hi Eric,
>>
>> On 2020/6/19 2:13, Eric Biggers wrote:
>>> Hi Chao,
>>>
>>> On Thu, Jun 18, 2020 at 06:06:02PM +0800, Chao Yu wrote:
>>>>> @@ -936,8 +972,11 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>>>  
>>>>>  	inc_page_count(sbi, WB_DATA_TYPE(bio_page));
>>>>>  
>>>>> -	if (io->bio && !io_is_mergeable(sbi, io->bio, io, fio,
>>>>> -			io->last_block_in_bio, fio->new_blkaddr))
>>>>> +	if (io->bio &&
>>>>> +	    (!io_is_mergeable(sbi, io->bio, io, fio, io->last_block_in_bio,
>>>>> +			      fio->new_blkaddr) ||
>>>>> +	     !f2fs_crypt_mergeable_bio(io->bio, fio->page->mapping->host,
>>>>> +				       fio->page->index, fio)))
>>>>
>>>> bio_page->index, fio)))
>>>>
>>>>>  		__submit_merged_bio(io);
>>>>>  alloc_new:
>>>>>  	if (io->bio == NULL) {
>>>>> @@ -949,6 +988,8 @@ void f2fs_submit_page_write(struct f2fs_io_info *fio)
>>>>>  			goto skip;
>>>>>  		}
>>>>>  		io->bio = __bio_alloc(fio, BIO_MAX_PAGES);
>>>>> +		f2fs_set_bio_crypt_ctx(io->bio, fio->page->mapping->host,
>>>>> +				       fio->page->index, fio, GFP_NOIO);
>>>>
>>>> bio_page->index, fio, GFP_NOIO);
>>>>
>>>
>>> We're using ->mapping->host and ->index.  Ordinarily that would mean the page
>>> needs to be a pagecache page.  But bio_page can also be a compressed page or a
>>> bounce page containing fs-layer encrypted contents.
>>
>> I'm concerning about compression + inlinecrypt case.
>>
>>>
>>> Is your suggestion to keep using fio->page->mapping->host (since encrypted pages
>>
>> Yup,
>>
>>> don't have a mapping), but start using bio_page->index (since f2fs apparently
>>
>> I meant that we need to use bio_page->index as tweak value in write path to
>> keep consistent as we did in read path, otherwise we may read the wrong
>> decrypted data later to incorrect tweak value.
>>
>> - f2fs_read_multi_pages (only comes from compression inode)
>>  - f2fs_alloc_dic
>>   - f2fs_set_compressed_page(page, cc->inode,
>> 					start_idx + i + 1, dic);
>>                                         ^^^^^^^^^^^^^^^^^
>>   - dic->cpages[i] = page;
>>  - for ()
>>      struct page *page = dic->cpages[i];
>>      if (!bio)
>>        - f2fs_grab_read_bio(..., page->index,..)
>>         - f2fs_set_bio_crypt_ctx(..., first_idx, ..)   /* first_idx == cpage->index */
>>
>> You can see that cpage->index was set to page->index + 1, that's why we need
>> to use one of cpage->index/page->index as tweak value all the time rather than
>> using both index mixed in read/write path.
>>
>> But note that for fs-layer encryption, we have used cpage->index as tweak value,
>> so here I suggest we can keep consistent to use cpage->index in inlinecrypt case.
> 
> Yes, inlinecrypt mustn't change the ciphertext that gets written to disk.
> 
>>
>>> *does* set ->index for compressed pages, and if the file uses fs-layer
>>> encryption then f2fs_set_bio_crypt_ctx() won't use the index anyway)?
>>>
>>> Does this mean the code is currently broken for compression + inline encryption
>>> because it's using the wrong ->index?  I think the answer is no, since
>>
>> I guess it's broken now for compression + inlinecrypt case.
>>
>>> f2fs_write_compressed_pages() will still pass the first 'nr_cpages' pagecache
>>> pages along with the compressed pages.  In that case, your suggestion would be a
>>> cleanup rather than a fix?
>>
>> That's a fix.
>>
> 
> FWIW, I tested this, and it actually works both before and after your suggested
> change.  The reason is that f2fs_write_compressed_pages() actually passes the
> pagecache pages sequentially starting at 'start_idx_of_cluster(cc) + 1' for the
> length of the compressed cluster.  That matches the '+ 1' adjustment elsewhere,
> so we have fio->page->index == bio_page->index.

I've checked the code, yes, that's correct.

> 
> I personally think the way the f2fs compression code works is really confusing.
> Compressed pages don't have a 1:1 correspondence to pagecache pages, so there
> should *not* be a pagecache page passed around when writing a compressed page.

The only place we always use fio->page is:
- f2fs_submit_page_write
 - trace_f2fs_submit_page_write(fio->page,..)
  - f2fs__submit_page_bio
   __entry->dev		= page_file_mapping(page)->host->i_sb->s_dev;
   __entry->ino		= page_file_mapping(page)->host->i_ino;

For compression case, we can get rid of using this parameter because bio_page
(fio->compressed_page) has correct mapping info, however for fs-layer encryption
case, bio_page (fio->encrypted_page, allocated by fscrypt_alloc_bounce_page())
has not correct mapping info.

> 
> Anyway, here's the test script I used in case anyone else wants to use it.  But
> we really need to write a proper f2fs compression + encryption test for xfstests
> which decrypts and decompresses a file in userspace and verifies we get back the
> original data.  (There are already ciphertext verification tests, but they don't
> cover compression.)  Note that this test is needed even for the filesystem-layer
> encryption which is currently supported.

Yes, let me check how to make this testcase a bit later.

> 
> #!/bin/bash
> 
> set -e
> 
> DEV=/dev/vdb
> 
> umount /mnt &> /dev/null || true
> mkfs.f2fs -f -O encrypt,compression,extra_attr $DEV
> head -c 1000000 /dev/zero > /tmp/testdata
> 
> for opt1 in '-o inlinecrypt' ''; do
>         mount $DEV /mnt $opt1
>         rm -rf /mnt/.fscrypt /mnt/dir
>         fscrypt setup /mnt
>         mkdir /mnt/dir
>         chattr +c /mnt/dir
>         echo hunter2 | fscrypt encrypt /mnt/dir --quiet --source=custom_passphrase --name=secret
>         cp /tmp/testdata /mnt/dir/file
>         umount /mnt
>         for opt2 in '-o inlinecrypt' ''; do
>                 mount $DEV /mnt $opt2
>                 echo hunter2 | fscrypt unlock /mnt/dir --quiet
>                 cmp /mnt/dir/file /tmp/testdata
>                 umount /mnt
>         done
> done
> .
>