linux-ext4 - Re: [PATCH 1/2] ext4: hoist ext4_block_write_begin and replace the __block_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240829000346.GC558903@google.com>
Date: Thu, 29 Aug 2024 00:03:46 +0000
From: Eric Biggers <ebiggers@...nel.org>
To: zhangshida <starzhangzsd@...il.com>
Cc: tytso@....edu, adilger.kernel@...ger.ca, jack@...e.com,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	zhangshida@...inos.cn, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH 1/2] ext4: hoist ext4_block_write_begin and replace the
 __block_write_begin

On Fri, Aug 23, 2024 at 09:33:28AM +0800, zhangshida wrote:
> From: Shida Zhang <zhangshida@...inos.cn>
> 
> Using __block_write_begin() make it inconvenient to journal the
> user data dirty process. We can't tell the block layer maintainer,
> ‘Hey, we want to trace the dirty user data in ext4, can we add some
> special code for ext4 in __block_write_begin?’:P
> 
> So use ext4_block_write_begin() instead.
> 
> The two functions are basically doing the same thing except for the
> fscrypt related code. Narrow the scope of CONFIG_FS_ENCRYPTION so as
> to allow ext4_block_write_begin() to function like __block_write_begin
> when the config is disabled.
> And hoist the ext4_block_write_begin so that it can be used in other
> files.
> 
> Suggested-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Shida Zhang <zhangshida@...inos.cn>
> ---
>  fs/ext4/ext4.h   |  2 ++
>  fs/ext4/inline.c | 10 +++++-----
>  fs/ext4/inode.c  | 23 ++++++-----------------
>  3 files changed, 13 insertions(+), 22 deletions(-)

Thanks for cleaning this up.

There are still some comments in fs/ext4/inode.c that reference
__block_write_begin.  Can you update them too?

One more thing below.

> @@ -1119,7 +1118,9 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
>  	}
>  	if (unlikely(err)) {
>  		folio_zero_new_buffers(folio, from, to);
> -	} else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
> +	}
> +#ifdef CONFIG_FS_ENCRYPTION
> +	else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
>  		for (i = 0; i < nr_wait; i++) {
>  			int err2;
>  
> @@ -1131,10 +1132,10 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
>  			}
>  		}
>  	}
> +#endif

This #ifdef isn't necessary since fscrypt_inode_uses_fs_layer_crypto() returns
false (and it's known at compile time) when !CONFIG_FS_ENCRYPTION.

- Eric