Thanks, I have made further clean-ups. Could you please check this?
https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev
On 06/15, Mikhail Lobanov via Linux-f2fs-devel wrote:
> When updating an atomic-write file, f2fs_write_begin() may read the
> previously written data back from the COW inode:
> prepare_atomic_write_begin() locates the block in the COW inode and sets
> use_cow, and the read bio is then built with the COW inode:
>
> f2fs_submit_page_read(use_cow ? F2FS_I(inode)->cow_inode : inode,
> ...);
>
> and f2fs_grab_read_bio() decides whether to schedule fs-layer decryption
> (STEP_DECRYPT) for the bio based on that inode via
> fscrypt_inode_uses_fs_layer_crypto().
>
> However, the folio being filled belongs to the original inode
> (folio->mapping->host == inode), and the data stored in the COW block was
> encrypted (or left as plaintext) using the original inode's context, not
> the COW inode's -- see f2fs_encrypt_one_page(), which keys off
> fio->page->mapping->host. fscrypt_decrypt_pagecache_blocks() likewise
> operates on folio->mapping->host.
>
> The COW inode is created as a tmpfile in the parent directory and inherits
> its encryption policy from there. With test_dummy_encryption the newly
> created COW inode gets the dummy policy and becomes encrypted, while a
> pre-existing regular file -- created before the policy applied, e.g.
> already present in the on-disk image -- stays unencrypted. The read
> path then sets STEP_DECRYPT based on the encrypted COW inode and calls
> fscrypt_decrypt_pagecache_blocks() on a folio whose host (the unencrypted
> original inode) has a NULL ->i_crypt_info, dereferencing it:
>
> Oops: general protection fault, probably for non-canonical address ...
> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> RIP: 0010:fscrypt_decrypt_pagecache_blocks+0xa0/0x310
> Workqueue: f2fs_post_read_wq f2fs_post_read_work
> Call Trace:
> fscrypt_decrypt_bio+0x1eb/0x340
> f2fs_post_read_work+0xba/0x140
> process_one_work+0x91c/0x1a40
> worker_thread+0x677/0xe90
> kthread+0x2bc/0x3a0
>
> The COW inode is only needed to locate the on-disk block, and that block
> address is already resolved into @blkaddr by prepare_atomic_write_begin()
> via __find_data_block(cow_inode, ...); f2fs_submit_page_read() then reads
> from that physical @blkaddr directly, so the inode argument only selects
> the post-read crypto context, not which block is fetched. Reading with
> @inode therefore returns the same (latest, not-yet-committed) COW data,
> while making both the fs-layer decryption decision and the inline crypto
> path use the correct (original inode's) key.
>
> With the COW inode no longer used at the read site, the use_cow flag has no
> remaining consumer; drop it from f2fs_write_begin() and
> prepare_atomic_write_begin().
>
> Fixes: 591fc34e1f98 ("f2fs: use cow inode data when updating atomic write")
> Cc: [email protected]
> Signed-off-by: Mikhail Lobanov <[email protected]>
> Reviewed-by: Chao Yu <[email protected]>
> ---
> v2: drop the now-unused use_cow flag from f2fs_write_begin() and
> prepare_atomic_write_begin() (Chao Yu); no functional change beyond
> v1. Carried Chao's Reviewed-by as the cleanup was his request.
> Rebased on current mainline (f2fs_submit_page_read() now takes a
> fsverity_info argument and returns void).
>
> fs/f2fs/data.c | 17 ++++++++++++-----
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 8d4f1e75dee3..9016272b68c7 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3822,7 +3822,7 @@ static int __reserve_data_block(struct inode *inode,
> pgoff_t index,
>
> static int prepare_atomic_write_begin(struct f2fs_sb_info *sbi,
> struct folio *folio, loff_t pos, unsigned int len,
> - block_t *blk_addr, bool *node_changed, bool *use_cow)
> + block_t *blk_addr, bool *node_changed)
> {
> struct inode *inode = folio->mapping->host;
> struct inode *cow_inode = F2FS_I(inode)->cow_inode;
> @@ -3839,7 +3839,6 @@ static int prepare_atomic_write_begin(struct
> f2fs_sb_info *sbi,
> if (err) {
> return err;
> } else if (*blk_addr != NULL_ADDR) {
> - *use_cow = true;
> return 0;
> }
>
> @@ -3873,7 +3872,6 @@ static int f2fs_write_begin(const struct kiocb *iocb,
> struct folio *folio;
> pgoff_t index = pos >> PAGE_SHIFT;
> bool need_balance = false;
> - bool use_cow = false;
> block_t blkaddr = NULL_ADDR;
> int err = 0;
>
> @@ -3936,7 +3934,7 @@ static int f2fs_write_begin(const struct kiocb *iocb,
>
> if (f2fs_is_atomic_file(inode))
> err = prepare_atomic_write_begin(sbi, folio, pos, len,
> - &blkaddr, &need_balance, &use_cow);
> + &blkaddr, &need_balance);
> else
> err = prepare_write_begin(sbi, folio, pos, len,
> &blkaddr, &need_balance);
> @@ -3976,8 +3974,15 @@ static int f2fs_write_begin(const struct kiocb *iocb,
> err = -EFSCORRUPTED;
> goto put_folio;
> }
> - f2fs_submit_page_read(use_cow ? F2FS_I(inode)->cow_inode :
> - inode,
> + /*
> + * Although the block may be stored in the COW inode, the folio
> + * belongs to @inode and its data was encrypted (or not) using
> + * @inode's context (see f2fs_encrypt_one_page()). Read with
> + * @inode so the post-read decryption decision matches the
> + * folio's owner; otherwise an unencrypted @inode whose COW
> inode
> + * is encrypted hits a NULL ->i_crypt_info on decryption.
> + */
> + f2fs_submit_page_read(inode,
> NULL, /* can't write to fsverity files */
> folio, blkaddr, 0, true);
>
>
> --
> 2.34.1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
