| Message ID | 20240813131928.842265-1-tiago.pasqualini@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | crypto: run qcrypto_pbkdf2_count_iters in a new thread | expand |
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 >
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
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 */
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(-)