Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > From: Eric Biggers <ebiggers@xxxxxxxxxx> > > Remove support for the "Crypto usage statistics" feature > (CONFIG_CRYPTO_STATS). This feature does not appear to have ever been > used, and it is harmful because it significantly reduces performance and > is a large maintenance burden. > > Covering each of these points in detail: > > 1. Feature is not being used > > Since these generic crypto statistics are only readable using netlink, > it's fairly straightforward to look for programs that use them. I'm > unable to find any evidence that any such programs exist. For example, > Debian Code Search returns no hits except the kernel header and kernel > code itself and translations of the kernel header: > https://codesearch.debian.net/search?q=CRYPTOCFGA_STAT&literal=1&perpkg=1 > > The patch series that added this feature in 2018 > (https://lore.kernel.org/linux-crypto/1537351855-16618-1-git-send-email-clabbe@xxxxxxxxxxxx/) > said "The goal is to have an ifconfig for crypto device." This doesn't > appear to have happened. > > It's not clear that there is real demand for crypto statistics. Just > because the kernel provides other types of statistics such as I/O and > networking statistics and some people find those useful does not mean > that crypto statistics are useful too. > > Further evidence that programs are not using CONFIG_CRYPTO_STATS is that > it was able to be disabled in RHEL and Fedora as a bug fix > (https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/merge_requests/2947). > > Even further evidence comes from the fact that there are and have been > bugs in how the stats work, but they were never reported. For example, > before Linux v6.7 hash stats were double-counted in most cases. > > There has also never been any documentation for this feature, so it > might be hard to use even if someone wanted to. > > 2. CONFIG_CRYPTO_STATS significantly reduces performance > > Enabling CONFIG_CRYPTO_STATS significantly reduces the performance of > the crypto API, even if no program ever retrieves the statistics. This > primarily affects systems with a large number of CPUs. For example, > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2039576 reported > that Lustre client encryption performance improved from 21.7GB/s to > 48.2GB/s by disabling CONFIG_CRYPTO_STATS. > > It can be argued that this means that CONFIG_CRYPTO_STATS should be > optimized with per-cpu counters similar to many of the networking > counters. But no one has done this in 5+ years. This is consistent > with the fact that the feature appears to be unused, so there seems to > be little interest in improving it as opposed to just disabling it. > > It can be argued that because CONFIG_CRYPTO_STATS is off by default, > performance doesn't matter. But Linux distros tend to error on the side > of enabling options. The option is enabled in Ubuntu and Arch Linux, > and until recently was enabled in RHEL and Fedora (see above). So, even > just having the option available is harmful to users. > > 3. CONFIG_CRYPTO_STATS is a large maintenance burden > > There are over 1000 lines of code associated with CONFIG_CRYPTO_STATS, > spread among 32 files. It significantly complicates much of the > implementation of the crypto API. After the initial submission, many > fixes and refactorings have consumed effort of multiple people to keep > this feature "working". We should be spending this effort elsewhere. > > Acked-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > Acked-by: Corentin Labbe <clabbe@xxxxxxxxxxxx> > Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx> > --- > > Changed in v2: > - Keep struct comp_alg_common, as per the request at > https://lore.kernel.org/linux-crypto/ZfERP1LK8q+CsoSo@xxxxxxxxxxxxxxxxxxx/, > even though the struct becomes trivial with the stats removed. > > - Use consistent comments in include/uapi/linux/cryptouser.h, > and add one to CRYPTO_MSG_GETSTAT which was missing one. > > - In crypto/compress.h, remove the forward declaration of sk_buff > because the function prototype that needed it is being removed. > > - In crypto_shash_digest(), don't read desc->tfm redundantly. > > arch/s390/configs/debug_defconfig | 1 - > arch/s390/configs/defconfig | 1 - > crypto/Kconfig | 20 --- > crypto/Makefile | 2 - > crypto/acompress.c | 33 ---- > crypto/aead.c | 84 +-------- > crypto/ahash.c | 63 +------ > crypto/akcipher.c | 31 ---- > crypto/compress.h | 3 - > crypto/{crypto_user_base.c => crypto_user.c} | 10 +- > crypto/crypto_user_stat.c | 176 ------------------- > crypto/hash.h | 30 ---- > crypto/kpp.c | 30 ---- > crypto/lskcipher.c | 73 +------- > crypto/rng.c | 44 +---- > crypto/scompress.c | 3 - > crypto/shash.c | 75 +------- > crypto/sig.c | 13 -- > crypto/skcipher.c | 86 +-------- > crypto/skcipher.h | 10 -- > include/crypto/acompress.h | 73 +------- > include/crypto/aead.h | 21 --- > include/crypto/akcipher.h | 78 +------- > include/crypto/algapi.h | 3 - > include/crypto/hash.h | 22 --- > include/crypto/internal/acompress.h | 1 - > include/crypto/internal/cryptouser.h | 16 -- > include/crypto/internal/scompress.h | 1 - > include/crypto/kpp.h | 58 +----- > include/crypto/rng.h | 51 +----- > include/crypto/skcipher.h | 25 --- > include/uapi/linux/cryptouser.h | 30 ++-- > 32 files changed, 71 insertions(+), 1096 deletions(-) > rename crypto/{crypto_user_base.c => crypto_user.c} (98%) > delete mode 100644 crypto/crypto_user_stat.c > delete mode 100644 include/crypto/internal/cryptouser.h Patch applied. Thanks. -- Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
