Re: [PATCH 2/10] crypto: aead - Count error stats differently

Eric Biggers
2023-02-16T05:35:41Z

On Wed, Feb 15, 2023 at 05:25:09PM +0800, Herbert Xu wrote:
> int crypto_aead_encrypt(struct aead_request *req)
> {
> struct crypto_aead *aead = crypto_aead_reqtfm(req);
> - struct crypto_alg *alg = aead->base.__crt_alg;
> + struct aead_alg *alg = crypto_aead_alg(aead);
> unsigned int cryptlen = req->cryptlen;

The cryptlen local variable is no longer needed. Just use req->cryptlen below.

> + struct crypto_istat_aead *istat;
> int ret;
>
> - crypto_stats_get(alg);
> + istat = aead_get_stat(alg);
> +
> + if (IS_ENABLED(CONFIG_CRYPTO_STATS)) {
> + atomic64_inc(&istat->encrypt_cnt);
> + atomic64_add(cryptlen, &istat->encrypt_tlen);
> + }
> +

This could just check whether istat is NULL:

istat = aead_get_stat(alg);
if (istat) {
atomic64_inc(&istat->encrypt_cnt);
atomic64_add(req->cryptlen, &istat->encrypt_tlen);
}

That's simpler, and it makes it clearer that the pointer is not dereferenced
when it is NULL.

Note that aead_get_stat() is an inline function, so the stats code will still be
optimized out when !CONFIG_CRYPTO_STATS.

> +static inline int crypto_aead_errstat(struct crypto_istat_aead *istat, int err)
> +{
> + if (!IS_ENABLED(CONFIG_CRYPTO_STATS))
> + return err;
> +
> + if (err && err != -EINPROGRESS && err != -EBUSY)
> + atomic64_inc(&istat->err_cnt);
> +
> + return err;
> +}
> +
[...]
> if (crypto_aead_get_flags(aead) & CRYPTO_TFM_NEED_KEY)
> ret = -ENOKEY;
> else
> - ret = crypto_aead_alg(aead)->encrypt(req);
> - crypto_stats_aead_encrypt(cryptlen, alg, ret);
> - return ret;
> + ret = alg->encrypt(req);
> +
> + return crypto_aead_errstat(istat, ret);

Similarly, istat != NULL could be used instead of CONFIG_CRYPTO_STATS.

IMO, this would also be easier to read if the stats increment was just coded
directly, like it is above, without the crypto_aead_errstat() function:

if (crypto_aead_get_flags(aead) & CRYPTO_TFM_NEED_KEY)
ret = -ENOKEY;
else
ret = alg->encrypt(req);

if (istat && ret && ret != -EINPROGRESS && ret != -EBUSY)
atomic64_inc(&istat->err_cnt);

return ret;

Similarly for all the other algorithm types.

- Eric