linux-kernel - Re: [PATCH v2] fscrypt: improve filename encryption and decryption performance

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: <20250704051441.GA4199@sol>
Date: Thu, 3 Jul 2025 22:14:41 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Yuwen Chen <ywen.chen@...mail.com>
Cc: davem@...emloft.net, herbert@...dor.apana.org.au, jaegeuk@...nel.org,
	linux-crypto@...r.kernel.org, linux-fscrypt@...r.kernel.org,
	linux-kernel@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH v2] fscrypt: improve filename encryption and decryption
 performance

On Fri, Jul 04, 2025 at 12:13:22PM +0800, Yuwen Chen wrote:
> With the CONFIG_UNICODE configuration enabled, the fname_decrypt
> and fscrypt_fname_encrypt functions may be called very frequently.
> Since filenames are generally short, the frequent invocation of
> memory allocation and release operations by these two functions
> will lead to very poor performance.
> 
> Use the following program to conduct a file-creation test in
> the private program directory(/data/media/0/Android/data/*)
> of Android.
> 
> int main(int argc, char **argv)
> {
>     size_t fcnt = 0;
>     char path[PATH_MAX];
>     char buf[4096] = {0};
>     int i, fd;
> 
>     if (argc < 2)
>         return - EINVAL;
> 
>     fcnt = atoi(argv[1]);
>     for (i = 0; i < fcnt; i++) {
>         snprintf(path, sizeof(path), "./%d", i);
>         fd = open(path, O_RDWR | O_CREAT, 0600);
>         if (fd < 0)
>             return - 1;
>         write(fd, buf, sizeof(buf));
>         close(fd);
>     }
>     return 0;
> }
> 
> The test platform is Snapdragon 8s Gen4, with a kernel version
> of v6.6 and a userdebug version.
> 
> Before this submission was merged, when creating 2000 files,
> the performance test results are as follows:
> $ time /data/file_creater 2000
> 0m14.83s real     0m00.00s user     0m14.30s system
> 0m15.61s real     0m00.00s user     0m15.04s system
> 0m14.72s real     0m00.01s user     0m14.18s system
> 
> After this submission was merged, the performance is as follows:
> $ time /data/file_creater 2000
> 0m07.64s real     0m00.00s user     0m07.37s system
> 0m07.66s real     0m00.00s user     0m07.40s system
> 0m08.67s real     0m00.01s user     0m08.35s system
> 
> Signed-off-by: Yuwen Chen <ywen.chen@...mail.com>
> ---
>  fs/crypto/fname.c         | 22 ++++++++++++++--------
>  include/crypto/skcipher.h |  9 +++++++++
>  2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 010f9c0a4c2f1..168b2a88fa23b 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -92,14 +92,20 @@ static inline bool fscrypt_is_dot_dotdot(const struct qstr *str)
>  int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
>  			  u8 *out, unsigned int olen)
>  {
> -	struct skcipher_request *req = NULL;
>  	DECLARE_CRYPTO_WAIT(wait);
>  	const struct fscrypt_inode_info *ci = inode->i_crypt_info;
>  	struct crypto_skcipher *tfm = ci->ci_enc_key.tfm;
> +	SKCIPHER_REQUEST_ON_STACK(req, tfm, MAX_SKCIPHER_REQSIZE);
>  	union fscrypt_iv iv;
>  	struct scatterlist sg;
>  	int res;
>  
> +	/*
> +	 * When the size of the statically - allocated skcipher_request
> +	 * structure is insufficient, remind us to make modifications.
> +	 */
> +	BUG_ON(MAX_SKCIPHER_REQSIZE < crypto_skcipher_reqsize(tfm));
> +
>  	/*
>  	 * Copy the filename to the output buffer for encrypting in-place and
>  	 * pad it with the needed number of NUL bytes.
> @@ -124,7 +130,6 @@ int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
>  
>  	/* Do the encryption */
>  	res = crypto_wait_req(crypto_skcipher_encrypt(req), &wait);
> -	skcipher_request_free(req);
>  	if (res < 0) {
>  		fscrypt_err(inode, "Filename encryption failed: %d", res);
>  		return res;

I'm guessing you have some debugging options enabled in your kconfig.  Usually
the allocations aren't quite *that* expensive.  That being said, it's always
been really annoying that they have to be there.

Unfortunately, as far as I know, you actually can't just allocate the
skcipher_request on the stack like that, since the legacy crypto API assumes
that the request memory is DMA-able.  On-stack requests also might not be
properly aligned (see
https://lore.kernel.org/all/CA+55aFxJOzMim_d-O2E2yip8JWo0NdYs_72sNwFKSkTjy8q0Sw@mail.gmail.com/
-- may be outdated, but I haven't heard otherwise).

The problem is really that the legacy crypto API (crypto_skcipher in this case)
was never really designed for efficient CPU-based crypto in the first place.
The correct solution is to add simple library APIs for the algorithms to
lib/crypto/, then update fscrypt to use that instead of crypto_skcipher.

I plan to do that, but I'm first focusing on other related things, such as doing
the same for fsverity.

- Eric