Skip to content

ML-DSA: Refactor sample ops dispatch to use a vtable - #5

Closed
mdcornu wants to merge 2 commits into
masterfrom
draft/ml_dsa_shake_x4-2
Closed

ML-DSA: Refactor sample ops dispatch to use a vtable#5
mdcornu wants to merge 2 commits into
masterfrom
draft/ml_dsa_shake_x4-2

Conversation

@mdcornu

@mdcornu mdcornu commented Apr 3, 2026

Copy link
Copy Markdown
Owner

PR to address feedback from upstream OpenSSL review: openssl#30654

The initial implementation dispatched matrix_expand_A, vector_expand_S, and vector_expand_mask via global function pointers initialized with CRYPTO_ONCE.

Changes:

  • Replace the CRYPTO_ONCE based global dispatch with an OSSL_ML_DSA_SAMPLE_OPS vtable. Callers obtain the right implementation via ossl_ml_dsa_sample_ops(), which returns either the generic scalar ops or the x86-64 AVX-512VL multi-buffer ops depending on the build and CPU capabilities.
  • Move the x86-64 multi-buffer implementation into ml_dsa_sample_x86_64.inc, included at the bottom of ml_dsa_sample.c only when KECCAK1600_ASM and x86_64 are defined.

No functional change.

Updated perf is very similar (if not actually slightly better than the run_once implementaion):

CRYPTO_ONCE:

                            keygen     signs    verify keygens/s    sign/s  verify/s
               ML-DSA-44 0.000068s 0.000270s 0.000055s   14694.4    3705.4   18338.8
               ML-DSA-65 0.000142s 0.000458s 0.000085s    7034.2    2184.4   11752.5
               ML-DSA-87 0.000163s 0.000531s 0.000135s    6144.3    1882.4    7400.3

After VTABLE update:

                               keygen     signs    verify keygens/s    sign/s  verify/s
                  ML-DSA-44 0.000068s 0.000270s 0.000054s   14728.0    3703.5   18534.5
                  ML-DSA-65 0.000141s 0.000452s 0.000085s    7080.4    2212.5   11790.0
                  ML-DSA-87 0.000162s 0.000531s 0.000134s    6187.9    1883.5    7441.0

mdcornu and others added 2 commits March 31, 2026 17:13
Introduce a SHAKE x4 implementation for x86_64 AVX512VL and
integrate it into ML-DSA sampling module.

- add keccak1600x4-avx512vl assembly module
- add sha3_x4.c wrappers and new internal SHA3 x4 API declarations
- add one-time capability-based dispatch in ml_dsa_sample.c
- preserve scalar fallback for non-AVX512VL or non-ASM builds

Co-authored-by: Tomasz Kantecki <tomasz.kantecki@intel.com>
Signed-off-by: Marcel Cornu <marcel.d.cornu@intel.com>
.globl SHA3_avx512vl_capable
.type SHA3_avx512vl_capable,\@abi-omnipotent
.align 32
SHA3_avx512vl_capable:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont quite understand why we do this in assembler?

@mdcornu mdcornu Apr 7, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because we can easily check the assembler version in the perl module.

If the assembler version supports AVX512VL extensions, the SHAKE x4 functions are generated and SHA3_avx512vl_capable() implements the OPENSSL_ia32cap_P table lookup.

If not, then only stubs are generated and the SHA3_avx512vl_capable() function is hardcoded to return 0, to guard against the use of the SHAKE x4 functions when they are not available.

Stub code can be seen here:

SHA3_avx512vl_capable:

@slontis slontis left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach here looks reasonable, if we need to refactor common bits out a a later point then that is ok.

@mdcornu

mdcornu commented Apr 7, 2026

Copy link
Copy Markdown
Owner Author

The approach here looks reasonable, if we need to refactor common bits out a a later point then that is ok.

Great! Thanks for looking into it. I'll push to version to the OpenSSL PR in case there is any more feedback.

@mdcornu mdcornu closed this Apr 27, 2026
mdcornu pushed a commit that referenced this pull request Jun 10, 2026
TSAN seems to be having a problem with atomic_load_ptr and
atomic_store_ptr.  Both are, by default, __ATOMIC_RELAXED operations.

According to the tsan docs, it flags these operations as a race because,
while they are indivisible, they create no happens-before constraint,
meaning they can be reordered.

An exemplar race that is reported is:

WARNING: ThreadSanitizer: data race (pid=2139404)
  Read of size 4 at 0x723400002308 by thread T39:
    #0 EVP_MD_up_ref crypto/evp/digest.c:995 (threadstest+0x45032d) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #1 evp_md_up_ref crypto/evp/digest.c:974 (threadstest+0x450242) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #2 ossl_method_up_ref crypto/property/property.c:201 (threadstest+0x4b7a55) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #3 ossl_method_store_cache_get_locked crypto/property/property.c:941 (threadstest+0x4b9922) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #4 ossl_method_store_cache_get crypto/property/property.c:974 (threadstest+0x4b9a47) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #5 inner_evp_generic_fetch crypto/evp/evp_fetch.c:314 (threadstest+0x458186) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #6 evp_generic_fetch crypto/evp/evp_fetch.c:404 (threadstest+0x4586dc) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #7 EVP_MD_fetch crypto/evp/digest.c:985 (threadstest+0x4502d7) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #8 derive_kdk crypto/rsa/rsa_ossl.c:472 (threadstest+0x4cf738) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#9 rsa_ossl_private_decrypt crypto/rsa/rsa_ossl.c:646 (threadstest+0x4d0174) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#10 RSA_private_decrypt crypto/rsa/rsa_crpt.c:48 (threadstest+0x4c6971) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#11 rsa_decrypt providers/implementations/asymciphers/rsa_enc.c:321 (threadstest+0x51cab7) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#12 EVP_PKEY_decrypt crypto/evp/asymcipher.c:280 (threadstest+0x44a9ca) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#13 thread_shared_evp_pkey test/threadstest.c:966 (threadstest+0x404be7) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#14 thread_run test/threadstest.h:67 (threadstest+0x40132d) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)

  Previous write of size 8 at 0x723400002308 by main thread (mutexes: write M0):
    #0 memset <null> (libtsan.so.2+0x4c1eb) (BuildId: 40906101a3a1e1f1ececafafda314aee009d688a)
    #1 CRYPTO_zalloc crypto/mem.c:228 (threadstest+0x48679d) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #2 evp_md_new crypto/evp/digest.c:758 (threadstest+0x44f35e) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #3 evp_md_from_algorithm crypto/evp/digest.c:839 (threadstest+0x44f885) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #4 construct_evp_method crypto/evp/evp_fetch.c:230 (threadstest+0x457ec9) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #5 ossl_method_construct_this crypto/core_fetch.c:110 (threadstest+0x4801bf) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #6 algorithm_do_map crypto/core_algorithm.c:77 (threadstest+0x47f7a3) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #7 algorithm_do_this crypto/core_algorithm.c:122 (threadstest+0x47f987) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    #8 ossl_provider_doall_activated crypto/provider_core.c:1609 (threadstest+0x49a42a) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#9 ossl_algorithm_do_all crypto/core_algorithm.c:164 (threadstest+0x47fb14) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#10 ossl_method_construct crypto/core_fetch.c:157 (threadstest+0x4803d0) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#11 inner_evp_generic_fetch crypto/evp/evp_fetch.c:333 (threadstest+0x4583a2) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#12 evp_generic_fetch crypto/evp/evp_fetch.c:404 (threadstest+0x4586dc) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#13 EVP_MD_fetch crypto/evp/digest.c:985 (threadstest+0x4502d7) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#14 derive_kdk crypto/rsa/rsa_ossl.c:472 (threadstest+0x4cf738) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#15 rsa_ossl_private_decrypt crypto/rsa/rsa_ossl.c:646 (threadstest+0x4d0174) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#16 RSA_private_decrypt crypto/rsa/rsa_crpt.c:48 (threadstest+0x4c6971) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#17 rsa_decrypt providers/implementations/asymciphers/rsa_enc.c:321 (threadstest+0x51cab7) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)
    openssl#18 EVP_PKEY_decrypt crypto/evp/asymcipher.c:280 (threadstest+0x44a9ca) (BuildId: f34377d95e3c1d13ab9aa3204d2f1f7840d1c84a)

What tsan is saying here is that the memset in evp_md_new may get
re-ordered such that the contents of the EVP_MD may still be getting
zeroed at the time we have (a) found the EVP_MD in the method store
cache, and (b) attempted to do an up_ref on it.

This is plainly impossible, especially given that, in order to reach the
method store cache, it must be places in the method store algorithm
sparse array, which still requires the taking of the method store write
lock.  But for some reason tsan fails to see the memory fence that
creates.

It seems the simplest solution to correct this is, if we are running
under tsan, use __ATOMIC_ACQUIRE and __ATOMIC_RELEASE on
CRYPTO_atomic_[load|store]_ptr to make sure tsan sees the proper memory
ordering.

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Bob Beck <beck@openssl.org>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
MergeDate: Tue Jun  9 18:17:19 2026
(Merged from openssl#31018)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants