On 6/16/26 00:03, Jaegeuk Kim wrote: > 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
The merged one looks good to me. Reviewed-by: Chao Yu <[email protected]> Thanks, > > 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
