qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Tiago Pasqualini <tiago.pasqualini@canonical.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH] crypto: run qcrypto_pbkdf2_count_iters in a new thread
Date: Fri, 30 Aug 2024 12:08:18 +0100	[thread overview]
Message-ID: <ZtGoIvroHBwF53oF@redhat.com> (raw)
In-Reply-To: <20240813131928.842265-1-tiago.pasqualini@canonical.com>

On Tue, Aug 13, 2024 at 10:19:28AM -0300, Tiago Pasqualini wrote:
> CPU time accounting in the kernel has been demonstrated to have a
> sawtooth pattern[1][2]. This can cause the getrusage system call to
> not be as accurate as we are expecting, which can cause this calculation
> to stall.
> 
> The kernel discussions shows that this inaccuracy happens when CPU time
> gets big enough, so this patch changes qcrypto_pbkdf2_count_iters to run
> in a fresh thread to avoid this inaccuracy. It also adds a sanity check
> to fail the process if CPU time is not accounted.
> 
> [1] https://lore.kernel.org/lkml/159231011694.16989.16351419333851309713.tip-bot2@tip-bot2/
> [2] https://lore.kernel.org/lkml/20221226031010.4079885-1-maxing.lan@bytedance.com/t/#m1c7f2fdc0ea742776a70fd1aa2a2e414c437f534
> 
> Resolves: #2398
> Signed-off-by: Tiago Pasqualini <tiago.pasqualini@canonical.com>
> ---
>  crypto/pbkdf.c         | 42 +++++++++++++++++++++++++++++++++++-------
>  include/crypto/pbkdf.h | 10 ++++++++++
>  2 files changed, 45 insertions(+), 7 deletions(-)

Mostly looks good, but one minor issue...

> diff --git a/include/crypto/pbkdf.h b/include/crypto/pbkdf.h
> index 2c31a44a27..b3757003e4 100644
> --- a/include/crypto/pbkdf.h
> +++ b/include/crypto/pbkdf.h
> @@ -153,4 +153,14 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
>                                      size_t nout,
>                                      Error **errp);
>  
> +typedef struct CountItersData {
> +    QCryptoHashAlgorithm hash;
> +    const uint8_t *key;
> +    size_t nkey;
> +    const uint8_t *salt;
> +    size_t nsalt;
> +    size_t nout;
> +    Error **errp;
> +    uint64_t iterations;

Super fussy here, but lets make 'Error **errp' the very
last item in the struct.

> +} CountItersData;
>  #endif /* QCRYPTO_PBKDF_H */

...this should remain in the pbkdf.c file, since it is not intended to
be part of the public API.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|



      parent reply	other threads:[~2024-08-30 11:08 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-13 13:19 [PATCH] crypto: run qcrypto_pbkdf2_count_iters in a new thread Tiago Pasqualini
2024-08-23 17:13 ` Tiago Pasqualini
2024-08-30 11:08 ` Daniel P. Berrangé [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZtGoIvroHBwF53oF@redhat.com \
    --to=berrange@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=tiago.pasqualini@canonical.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).