On Fri, Jan 12, 2018 at 07:49:43AM +0100, Stephan Mueller wrote:
> Am Donnerstag, 11. Januar 2018, 20:56:56 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@xxxxxxxxxxxx>
> > ---
> > crypto/Kconfig | 11 ++++++++
> > crypto/ablkcipher.c | 9 +++++++
> > crypto/acompress.c | 9 +++++++
> > crypto/aead.c | 10 ++++++++
> > crypto/ahash.c | 8 ++++++
> > crypto/akcipher.c | 13 ++++++++++
> > crypto/algapi.c | 6 +++++
> > crypto/blkcipher.c | 9 +++++++
> > crypto/crypto_user.c | 28 +++++++++++++++++++++
> > crypto/kpp.c | 7 ++++++
> > crypto/rng.c | 8 ++++++
> > crypto/scompress.c | 9 +++++++
> > crypto/shash.c | 5 ++++
> > crypto/skcipher.c | 9 +++++++
> > include/crypto/acompress.h | 22 ++++++++++++++++
> > include/crypto/aead.h | 22 ++++++++++++++++
> > include/crypto/akcipher.h | 42 +++++++++++++++++++++++++++++++
> > include/crypto/hash.h | 21 ++++++++++++++++
> > include/crypto/kpp.h | 28 +++++++++++++++++++++
> > include/crypto/rng.h | 17 +++++++++++++
> > include/crypto/skcipher.h | 22 ++++++++++++++++
> > include/linux/crypto.h | 56
> > +++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/cryptouser.h |
> > 34 +++++++++++++++++++++++++
> > 23 files changed, 405 insertions(+)
> >
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index 971d558494c3..3b88fba14b59 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -1780,6 +1780,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
> >
[...]
> > * crypto_acomp_compress() -- Invoke asynchronous compress operation
> > *
> > @@ -247,6 +267,7 @@ static inline int crypto_acomp_compress(struct acomp_req
> > *req) {
> > struct crypto_acomp *tfm = crypto_acomp_reqtfm(req);
> >
> > + crypto_stat_compress(req);
> > return tfm->compress(req);
>
> In general: should'nt the statistics increment only happen if the associated
> operation was successful?
>
I will do it.
This bring also a possibility to get errors counters.
> > /**
> > * crypto_ahash_finup() - update and finalize message digest
> > * @req: reference to the ahash_request handle that holds all information
> > @@ -519,6 +538,8 @@ static inline int crypto_ahash_init(struct ahash_request
> > *req) */
> > static inline int crypto_ahash_update(struct ahash_request *req)
> > {
> > +
> > + crypto_stat_ahash_update(req);
> > return crypto_ahash_reqtfm(req)->update(req);
>
> In case you roll another update: please remove the blank line.
Ok
> > diff --git a/include/uapi/linux/cryptouser.h
> > b/include/uapi/linux/cryptouser.h index 19bf0ca6d635..15e51ccb3679 100644
> > --- a/include/uapi/linux/cryptouser.h
> > +++ b/include/uapi/linux/cryptouser.h
> > @@ -73,6 +73,8 @@ struct crypto_report_hash {
> > char type[CRYPTO_MAX_NAME];
> > unsigned int blocksize;
> > unsigned int digestsize;
> > + __u64 stat_hash;
>
> Why do you use __u64? The atomic_t variable is an int, i.e. 32 bit. Thus I
> would think that __u32 would suffice?
You are right, I will downgrade to __u32
But I think I will set len stats to atomic64/__u64 and keep count on atomic_t/__u32.
>
>
> > + __u64 stat_hash_tlen;
> > };
>
> What I am slightly unsure here is: how should user space detect whether these
> additional parameters are part of the NETLINK_USER API or not? I use that
> interface in my libkcapi whose binary may be used on multiple different kernel
> versions. How should that library operate if one kernel has these parameters
> and another does not?
>
Userspace could check for kernel version and know if stat are present or not.
Another way is to add a new netlink request.
Thanks for your review.
Regards
Corentin Labbe
