| 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
| ||
|
Message-ID: <20241104160136.GI21832@frogsfrogsfrogs>
Date: Mon, 4 Nov 2024 08:01:36 -0800
From: "Darrick J. Wong" <djwong@...nel.org>
To: Eric Biggers <ebiggers@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-crypto@...r.kernel.org,
linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
linux-mips@...r.kernel.org, linux-riscv@...ts.infradead.org,
linux-s390@...r.kernel.org, linux-scsi@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, loongarch@...ts.linux.dev,
sparclinux@...r.kernel.org, x86@...nel.org,
Ard Biesheuvel <ardb@...nel.org>
Subject: Re: [PATCH v3 16/18] jbd2: switch to using the crc32c library
On Sun, Nov 03, 2024 at 02:31:52PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@...gle.com>
>
> Now that the crc32c() library function directly takes advantage of
> architecture-specific optimizations, it is unnecessary to go through the
> crypto API. Just use crc32c(). This is much simpler, and it improves
> performance due to eliminating the crypto API overhead.
>
> Reviewed-by: Ard Biesheuvel <ardb@...nel.org>
> Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> ---
> fs/jbd2/Kconfig | 2 --
> fs/jbd2/journal.c | 25 ++-----------------------
> include/linux/jbd2.h | 31 +++----------------------------
> 3 files changed, 5 insertions(+), 53 deletions(-)
>
> diff --git a/fs/jbd2/Kconfig b/fs/jbd2/Kconfig
> index 4ad2c67f93f1..9c19e1512101 100644
> --- a/fs/jbd2/Kconfig
> +++ b/fs/jbd2/Kconfig
> @@ -1,11 +1,9 @@
> # SPDX-License-Identifier: GPL-2.0-only
> config JBD2
> tristate
> select CRC32
> - select CRYPTO
> - select CRYPTO_CRC32C
> help
> This is a generic journaling layer for block devices that support
> both 32-bit and 64-bit block numbers. It is currently used by
> the ext4 and OCFS2 filesystems, but it could also be used to add
> journal support to other file systems or block devices such
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 97f487c3d8fc..56cea5a738a7 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1373,24 +1373,16 @@ static int journal_check_superblock(journal_t *journal)
> printk(KERN_ERR "JBD2: Can't enable checksumming v1 and v2/3 "
> "at the same time!\n");
> return err;
> }
>
> - /* Load the checksum driver */
> if (jbd2_journal_has_csum_v2or3_feature(journal)) {
> if (sb->s_checksum_type != JBD2_CRC32C_CHKSUM) {
> printk(KERN_ERR "JBD2: Unknown checksum type\n");
> return err;
> }
>
> - journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
> - if (IS_ERR(journal->j_chksum_driver)) {
> - printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> - err = PTR_ERR(journal->j_chksum_driver);
> - journal->j_chksum_driver = NULL;
> - return err;
> - }
> /* Check superblock checksum */
> if (sb->s_checksum != jbd2_superblock_csum(journal, sb)) {
> printk(KERN_ERR "JBD2: journal checksum error\n");
> err = -EFSBADCRC;
> return err;
> @@ -1611,12 +1603,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
>
> return journal;
>
> err_cleanup:
> percpu_counter_destroy(&journal->j_checkpoint_jh_count);
> - if (journal->j_chksum_driver)
> - crypto_free_shash(journal->j_chksum_driver);
> kfree(journal->j_wbuf);
> jbd2_journal_destroy_revoke(journal);
> journal_fail_superblock(journal);
> kfree(journal);
> return ERR_PTR(err);
> @@ -2194,12 +2184,10 @@ int jbd2_journal_destroy(journal_t *journal)
> if (journal->j_proc_entry)
> jbd2_stats_proc_exit(journal);
> iput(journal->j_inode);
> if (journal->j_revoke)
> jbd2_journal_destroy_revoke(journal);
> - if (journal->j_chksum_driver)
> - crypto_free_shash(journal->j_chksum_driver);
> kfree(journal->j_fc_wbuf);
> kfree(journal->j_wbuf);
> kfree(journal);
>
> return err;
> @@ -2340,23 +2328,14 @@ int jbd2_journal_set_features(journal_t *journal, unsigned long compat,
> pr_err("JBD2: Cannot enable fast commits.\n");
> return 0;
> }
> }
>
> - /* Load the checksum driver if necessary */
> - if ((journal->j_chksum_driver == NULL) &&
> - INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3)) {
> - journal->j_chksum_driver = crypto_alloc_shash("crc32c", 0, 0);
> - if (IS_ERR(journal->j_chksum_driver)) {
> - printk(KERN_ERR "JBD2: Cannot load crc32c driver.\n");
> - journal->j_chksum_driver = NULL;
> - return 0;
> - }
> - /* Precompute checksum seed for all metadata */
> + /* Precompute checksum seed for all metadata */
> + if (INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3))
> journal->j_csum_seed = jbd2_chksum(journal, ~0, sb->s_uuid,
> sizeof(sb->s_uuid));
> - }
>
> lock_buffer(journal->j_sb_buffer);
>
> /* If enabling v3 checksums, update superblock */
> if (INCOMPAT_FEATURE_ON(JBD2_FEATURE_INCOMPAT_CSUM_V3)) {
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 8aef9bb6ad57..33d25a3d15f1 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -26,11 +26,11 @@
> #include <linux/mutex.h>
> #include <linux/timer.h>
> #include <linux/slab.h>
> #include <linux/bit_spinlock.h>
> #include <linux/blkdev.h>
> -#include <crypto/hash.h>
> +#include <linux/crc32c.h>
> #endif
>
> #define journal_oom_retry 1
>
> /*
> @@ -1239,17 +1239,10 @@ struct journal_s
> * An opaque pointer to fs-private information. ext3 puts its
> * superblock pointer here.
> */
> void *j_private;
>
> - /**
> - * @j_chksum_driver:
> - *
> - * Reference to checksum algorithm driver via cryptoapi.
> - */
> - struct crypto_shash *j_chksum_driver;
> -
> /**
> * @j_csum_seed:
> *
> * Precomputed journal UUID checksum for seeding other checksums.
> */
> @@ -1748,14 +1741,11 @@ static inline bool jbd2_journal_has_csum_v2or3_feature(journal_t *j)
> return jbd2_has_feature_csum2(j) || jbd2_has_feature_csum3(j);
> }
>
> static inline int jbd2_journal_has_csum_v2or3(journal_t *journal)
> {
> - WARN_ON_ONCE(jbd2_journal_has_csum_v2or3_feature(journal) &&
> - journal->j_chksum_driver == NULL);
> -
> - return journal->j_chksum_driver != NULL;
> + return jbd2_journal_has_csum_v2or3_feature(journal);
Same comment as my last reply about removing trivial helpers, but
otherwise
Reviewed-by: Darrick J. Wong <djwong@...nel.org>
If you push this for 6.13 I'd be happy that the shash junk finally went
away. The onstack struct desc thing in the _chksum() functions was by
far the most sketchy part of the ext4/jbd2 metadata csum project. :)
--D
> }
>
> static inline int jbd2_journal_get_num_fc_blks(journal_superblock_t *jsb)
> {
> int num_fc_blocks = be32_to_cpu(jsb->s_num_fc_blks);
> @@ -1794,26 +1784,11 @@ static inline unsigned long jbd2_log_space_left(journal_t *journal)
> #define JBD_MAX_CHECKSUM_SIZE 4
>
> static inline u32 jbd2_chksum(journal_t *journal, u32 crc,
> const void *address, unsigned int length)
> {
> - struct {
> - struct shash_desc shash;
> - char ctx[JBD_MAX_CHECKSUM_SIZE];
> - } desc;
> - int err;
> -
> - BUG_ON(crypto_shash_descsize(journal->j_chksum_driver) >
> - JBD_MAX_CHECKSUM_SIZE);
> -
> - desc.shash.tfm = journal->j_chksum_driver;
> - *(u32 *)desc.ctx = crc;
> -
> - err = crypto_shash_update(&desc.shash, address, length);
> - BUG_ON(err);
> -
> - return *(u32 *)desc.ctx;
> + return crc32c(crc, address, length);
> }
>
> /* Return most recent uncommitted transaction */
> static inline tid_t jbd2_get_latest_transaction(journal_t *journal)
> {
> --
> 2.47.0
>
>
Powered by blists - more mailing lists