linux-kernel - Re: [PATCH RFC 2/3] crypto: Implement a generic crypto statistics

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: <3836147.1hXCrOrRkh@tauon.chronox.de>
Date:   Thu, 21 Dec 2017 07:38:35 +0100
From:   Stephan Mueller <smueller@...onox.de>
To:     Corentin Labbe <clabbe@...libre.com>
Cc:     davem@...emloft.net, herbert@...dor.apana.org.au,
        nhorman@...driver.com, linux-crypto@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 2/3] crypto: Implement a generic crypto statistics

Am Mittwoch, 20. Dezember 2017, 21:09:26 CET schrieb Corentin Labbe:

Hi Corentin,

> This patch implement a generic way to get statistics about all crypto
> usages.
> 
> Signed-off-by: Corentin Labbe <clabbe@...libre.com>
> ---
>  crypto/Kconfig             |  11 +++
>  crypto/ahash.c             |  18 +++++
>  crypto/algapi.c            | 186
> +++++++++++++++++++++++++++++++++++++++++++++ crypto/rng.c               | 
>  3 +
>  include/crypto/acompress.h |  10 +++
>  include/crypto/akcipher.h  |  12 +++
>  include/crypto/kpp.h       |   9 +++
>  include/crypto/rng.h       |   5 ++
>  include/crypto/skcipher.h  |   8 ++
>  include/linux/crypto.h     |  22 ++++++
>  10 files changed, 284 insertions(+)
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index d6e9b60fc063..69f1822a026b 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -1781,6 +1781,17 @@ config CRYPTO_USER_API_AEAD
>  	  This option enables the user-spaces interface for AEAD
>  	  cipher algorithms.
> 
> +config CRYPTO_STATS
> +	bool "Crypto usage statistics for User-space"
> +	help
> +	  This option enables the gathering of crypto stats.
> +	  This will collect:
> +	  - encrypt/decrypt size and numbers of symmeric operations
> +	  - compress/decompress size and numbers of compress operations
> +	  - size and numbers of hash operations
> +	  - encrypt/decrypt/sign/verify numbers for asymmetric operations
> +	  - generate/seed numbers for rng operations
> +
>  config CRYPTO_HASH_INFO
>  	bool
> 
> diff --git a/crypto/ahash.c b/crypto/ahash.c
> index 3a35d67de7d9..93b56892f1b8 100644
> --- a/crypto/ahash.c
> +++ b/crypto/ahash.c
> @@ -356,18 +356,36 @@ static int crypto_ahash_op(struct ahash_request *req,
> 
>  int crypto_ahash_final(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> +	tfm->base.__crt_alg->enc_cnt++;
> +	tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif
>  	return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->final);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_final);
> 
>  int crypto_ahash_finup(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> +	tfm->base.__crt_alg->enc_cnt++;
> +	tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif
>  	return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->finup);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_finup);
> 
>  int crypto_ahash_digest(struct ahash_request *req)
>  {
> +#ifdef CONFIG_CRYPTO_STATS
> +	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> +
> +	tfm->base.__crt_alg->enc_cnt++;
> +	tfm->base.__crt_alg->enc_tlen += req->nbytes;
> +#endif

This is ugly. May I ask that one inlne function is made that has these ifdefs 
instead of adding them throughout multiple functions? This comment would apply 
to the other instrumentation code below as well. The issue is that these 
ifdefs should not be spread around through the code.

Besides, I would think that the use of normal integers is not thread-safe. 
What about using atomic_t?

>  	return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->digest);
>  }
>  EXPORT_SYMBOL_GPL(crypto_ahash_digest);
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index b8f6122f37e9..4fca4576af78 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -20,11 +20,158 @@
>  #include <linux/rtnetlink.h>
>  #include <linux/slab.h>
>  #include <linux/string.h>
> +#include <linux/kobject.h>
> 
>  #include "internal.h"
> 
>  static LIST_HEAD(crypto_template_list);
> 
> +#ifdef CONFIG_CRYPTO_STATS

Instead of ifdefing in the code, may I suggest adding a new file that is 
compiled / not compiled via the Makefile?

> +static struct kobject *crypto_root_kobj;
> +
> +static struct kobj_type dynamic_kobj_ktype = {
> +	.sysfs_ops      = &kobj_sysfs_ops,
> +};
> +
> +static ssize_t fcrypto_priority(struct kobject *kobj,
> +				struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%d\n", alg->cra_priority);
> +}
> +
> +static ssize_t fcrypto_stat_enc_cnt(struct kobject *kobj,
> +				    struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%lu\n", alg->enc_cnt);
> +}
> +
> +static ssize_t fcrypto_stat_enc_tlen(struct kobject *kobj,
> +				     struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%lu\n", alg->enc_tlen);
> +}
> +
> +static ssize_t fcrypto_stat_dec_cnt(struct kobject *kobj,
> +				    struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%lu\n", alg->dec_cnt);
> +}
> +
> +static ssize_t fcrypto_stat_dec_tlen(struct kobject *kobj,
> +				     struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%lu\n", alg->dec_tlen);
> +}
> +
> +static ssize_t fcrypto_stat_verify_cnt(struct kobject *kobj,
> +				       struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%lu\n", alg->verify_cnt);
> +}
> +
> +static ssize_t fcrypto_stat_sign_cnt(struct kobject *kobj,
> +				     struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	return snprintf(buf, 9, "%lu\n", alg->sign_cnt);
> +}
> +
> +static ssize_t fcrypto_stat_type(struct kobject *kobj,
> +				 struct kobj_attribute *attr, char *buf)
> +{
> +	struct crypto_alg *alg;
> +	u32 type;
> +
> +	alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> +	type = (alg->cra_flags & CRYPTO_ALG_TYPE_MASK);
> +	if (type == CRYPTO_ALG_TYPE_ABLKCIPHER ||
> +	    type == CRYPTO_ALG_TYPE_SKCIPHER ||
> +	    type == CRYPTO_ALG_TYPE_CIPHER ||
> +	    type == CRYPTO_ALG_TYPE_BLKCIPHER
> +		)
> +		return snprintf(buf, 9, "cipher\n");

"skcipher" ?

Ciao
Stephan