[PATCH v2] f2fs: read COW data with the original inode during atomic write

[PATCH v2] f2fs: read COW data with the original inode during atomic write

[Mikhail Lobanov]

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: stable@vger.kernel.org
Signed-off-by: Mikhail Lobanov <m.lobanov@rosa.ru>
Reviewed-by: Chao Yu <chao@kernel.org>
---
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

Re: [f2fs-dev] [PATCH v2] f2fs: read COW data with the original inode during atomic write

[Jaegeuk Kim]

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: stable@vger.kernel.org
> Signed-off-by: Mikhail Lobanov <m.lobanov@rosa.ru>
> Reviewed-by: Chao Yu <chao@kernel.org>
> ---
> 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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v2] f2fs: read COW data with the original inode during atomic write

[Mikhail Lobanov]

Hi Jaegeuk,

Looks good to me, thanks.

Mikhail

Re: [f2fs-dev] [PATCH v2] f2fs: read COW data with the original inode during atomic write

[Chao Yu]

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 <chao@kernel.org>

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: stable@vger.kernel.org
>> Signed-off-by: Mikhail Lobanov <m.lobanov@rosa.ru>
>> Reviewed-by: Chao Yu <chao@kernel.org>
>> ---
>> 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
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel