[PATCH] crypto: run qcrypto_pbkdf2_count_iters in a new thread

[PATCH] crypto: run qcrypto_pbkdf2_count_iters in a new thread

[Tiago Pasqualini]

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(-)

diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 8d198c152c..4c4e1e3cd3 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/thread.h"
 #include "qapi/error.h"
 #include "crypto/pbkdf.h"
 #ifndef _WIN32
@@ -85,12 +86,17 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,
 #endif
 }
 
-uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
-                                    const uint8_t *key, size_t nkey,
-                                    const uint8_t *salt, size_t nsalt,
-                                    size_t nout,
-                                    Error **errp)
+static void *threaded_qcrypto_pbkdf2_count_iters(void *data)
 {
+    CountItersData *iters_data = (CountItersData *) data;
+    QCryptoHashAlgorithm hash = iters_data->hash;
+    const uint8_t *key = iters_data->key;
+    size_t nkey = iters_data->nkey;
+    const uint8_t *salt = iters_data->salt;
+    size_t nsalt = iters_data->nsalt;
+    size_t nout = iters_data->nout;
+    Error **errp = iters_data->errp;
+
     uint64_t ret = -1;
     g_autofree uint8_t *out = g_new(uint8_t, nout);
     uint64_t iterations = (1 << 15);
@@ -114,7 +120,10 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
 
         delta_ms = end_ms - start_ms;
 
-        if (delta_ms > 500) {
+        if (delta_ms == 0) { /* sanity check */
+            error_setg(errp, "Unable to get accurate CPU usage");
+            goto cleanup;
+        } else if (delta_ms > 500) {
             break;
         } else if (delta_ms < 100) {
             iterations = iterations * 10;
@@ -129,5 +138,24 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
 
  cleanup:
     memset(out, 0, nout);
-    return ret;
+    iters_data->iterations = ret;
+    return NULL;
+}
+
+uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
+                                    const uint8_t *key, size_t nkey,
+                                    const uint8_t *salt, size_t nsalt,
+                                    size_t nout,
+                                    Error **errp)
+{
+    CountItersData data = {
+        hash, key, nkey, salt, nsalt, nout, errp
+    };
+    QemuThread thread;
+
+    qemu_thread_create(&thread, "pbkdf2", threaded_qcrypto_pbkdf2_count_iters,
+                       &data, QEMU_THREAD_JOINABLE);
+    qemu_thread_join(&thread);
+
+    return data.iterations;
 }
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;
+} CountItersData;
 #endif /* QCRYPTO_PBKDF_H */
-- 
2.43.0

Re: [PATCH] crypto: run qcrypto_pbkdf2_count_iters in a new thread

[Tiago Pasqualini]

Hi folks,

Sorry for the ping, but any thoughts on this? I mainly implemented
what was discussed in the upstream bug[0].

Let me know what you think or any suggestions for this.

Thank you,
Tiago

[0] https://gitlab.com/qemu-project/qemu/-/issues/2398

On Tue, Aug 13, 2024 at 10:19 AM Tiago Pasqualini
<tiago.pasqualini@canonical.com> 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(-)
>
> diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
> index 8d198c152c..4c4e1e3cd3 100644
> --- a/crypto/pbkdf.c
> +++ b/crypto/pbkdf.c
> @@ -19,6 +19,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/thread.h"
>  #include "qapi/error.h"
>  #include "crypto/pbkdf.h"
>  #ifndef _WIN32
> @@ -85,12 +86,17 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,
>  #endif
>  }
>
> -uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
> -                                    const uint8_t *key, size_t nkey,
> -                                    const uint8_t *salt, size_t nsalt,
> -                                    size_t nout,
> -                                    Error **errp)
> +static void *threaded_qcrypto_pbkdf2_count_iters(void *data)
>  {
> +    CountItersData *iters_data = (CountItersData *) data;
> +    QCryptoHashAlgorithm hash = iters_data->hash;
> +    const uint8_t *key = iters_data->key;
> +    size_t nkey = iters_data->nkey;
> +    const uint8_t *salt = iters_data->salt;
> +    size_t nsalt = iters_data->nsalt;
> +    size_t nout = iters_data->nout;
> +    Error **errp = iters_data->errp;
> +
>      uint64_t ret = -1;
>      g_autofree uint8_t *out = g_new(uint8_t, nout);
>      uint64_t iterations = (1 << 15);
> @@ -114,7 +120,10 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
>
>          delta_ms = end_ms - start_ms;
>
> -        if (delta_ms > 500) {
> +        if (delta_ms == 0) { /* sanity check */
> +            error_setg(errp, "Unable to get accurate CPU usage");
> +            goto cleanup;
> +        } else if (delta_ms > 500) {
>              break;
>          } else if (delta_ms < 100) {
>              iterations = iterations * 10;
> @@ -129,5 +138,24 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
>
>   cleanup:
>      memset(out, 0, nout);
> -    return ret;
> +    iters_data->iterations = ret;
> +    return NULL;
> +}
> +
> +uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
> +                                    const uint8_t *key, size_t nkey,
> +                                    const uint8_t *salt, size_t nsalt,
> +                                    size_t nout,
> +                                    Error **errp)
> +{
> +    CountItersData data = {
> +        hash, key, nkey, salt, nsalt, nout, errp
> +    };
> +    QemuThread thread;
> +
> +    qemu_thread_create(&thread, "pbkdf2", threaded_qcrypto_pbkdf2_count_iters,
> +                       &data, QEMU_THREAD_JOINABLE);
> +    qemu_thread_join(&thread);
> +
> +    return data.iterations;
>  }
> 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;
> +} CountItersData;
>  #endif /* QCRYPTO_PBKDF_H */
> --
> 2.43.0
>

Re: [PATCH] crypto: run qcrypto_pbkdf2_count_iters in a new thread

[Daniel P. Berrangé]

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 :|