[MDEV-20386] Replace inline asm with compiler-builtin intrinsic functions Created: 2019-08-19  Updated: 2021-01-02  Resolved: 2021-01-01

Status: Closed
Project: MariaDB Server
Component/s: Encryption, Storage Engine - InnoDB
Affects Version/s: 10.2, 10.3, 10.4, 10.5
Fix Version/s: 10.5.9, 10.6.0

Type: Bug Priority: Major
Reporter: Marko Mäkelä Assignee: Marko Mäkelä
Resolution: Fixed Votes: 0
Labels: MSAN, portability

Issue Links:
PartOf
is part of MDEV-20377 Make WITH_MSAN more usable Closed
Relates
relates to MDEV-23663 HAVE_INTEL_RDRAND is not enabled in t... Closed
relates to MDEV-24514 WITH_MSAN is disabling WOLFSSL_AESNI ... Closed
relates to MDEV-23900 x86_32 fail to build on innodb crc32q Closed

 Description   

As noted in MDEV-20377, inline assembler will cause MemorySanitizer to report false positives for uninitialized values. It would be more portable to use the compiler built-in functions not only on Windows, but on all platforms.

Here is a crude patch for WolfSSL:

diff --git a/wolfcrypt/src/random.c b/wolfcrypt/src/random.c
 
index 6b0d5dafc..817619d16 100644
 
--- a/wolfcrypt/src/random.c
 
+++ b/wolfcrypt/src/random.c
 
@@ -173,7 +173,7 @@ int wc_RNG_GenerateByte(WC_RNG* rng, byte* b)
 
     static int wc_GenerateRand_IntelRD(OS_Seed* os, byte* output, word32 sz);
 
     #endif
 
 
-#ifdef USE_WINDOWS_API
 
+#if 1 /*def USE_WINDOWS_API */
 
     #include <immintrin.h>
 
 #endif /* USE_WINDOWS_API */
 
 #endif
 
@@ -1282,7 +1282,7 @@ int wc_FreeNetRandom(void)
 
 
 #ifdef HAVE_INTEL_RDSEED
 
 
-#ifndef USE_WINDOWS_API
 
+#if 0/*ndef USE_WINDOWS_API*/
 
 
     /* return 0 on success */
 
     static WC_INLINE int IntelRDseed64(word64* seed)
 
@@ -1302,7 +1302,7 @@ int wc_FreeNetRandom(void)
 
     {
 
         int ok;
 
 
-        ok = _rdseed64_step(seed);
 
+        ok = _rdseed64_step((unsigned long long*) seed);
 
         return (ok) ? 0 : -1;
 
     }

and another for InnoDB:

diff --git a/storage/innobase/ut/ut0crc32.cc b/storage/innobase/ut/ut0crc32.cc
 
index 4a6447c1dcf..58273e9058e 100644
 
--- a/storage/innobase/ut/ut0crc32.cc
 
+++ b/storage/innobase/ut/ut0crc32.cc
 
@@ -219,6 +219,8 @@ ut_crc32_8_hw(
 
 {
 
 #ifdef _MSC_VER
 
 	*crc = _mm_crc32_u8(*crc, (*data)[0]);
 
+#elif 1
 
+	*crc = __builtin_ia32_crc32qi(*crc, (*data)[0]);
 
 #else
 
 	asm("crc32b %1, %0"
 
 	    /* output operands */
 
@@ -251,6 +253,8 @@ ut_crc32_64_low_hw(
 
 #else
 
 #error Not Supported processors type.
 
 #endif
 
+#elif 1
 
+	crc_64bit = __builtin_ia32_crc32di(crc_64bit, data);
 
 #else
 
 	asm("crc32q %1, %0"
 
 	    /* output operands */

Both these patches will require some compiler switches to enable the instructions to be emitted. For clang 8.0.1, -march=native did the trick for me, but that is obviously unacceptable. According to diagnostic messages from the compiler, the features might be called "rdseed" and "SSE 4.2".

 Comments   
Comment by Vladislav Vaintroub [ 2019-08-19 ]

You may file a bug against wolfssl/wolfcrypt. we do not change WolfSSL code

Comment by Marko Mäkelä [ 2019-08-19 ]

Very well, I can file a separate bug against WolfSSL. Meanwhile, can you please look at the InnoDB part?

This is blocking the use of MSAN, which is an important tool for quality assurance. It promises to have only 3× CPU overhead, which is much lower than Valgrind. Hence, "won’t fix" is not an acceptable resolution.

Comment by Marko Mäkelä [ 2019-08-19 ]

Replace inline asm with compiler built-in intrinsic functions #2415 was filed against WolfSSL.

Comment by Vladislav Vaintroub [ 2020-03-18 ]

I'm closing this since this is not in our code, and since the bug was already files against WolfSSL.

Comment by Marko Mäkelä [ 2020-03-25 ]

I am reopening this, because MDEV-20377 (enable MemorySanitizer) requires that the problem be addressed.

Comment by Marko Mäkelä [ 2020-08-31 ]

The WolfSSL problem was worked around by disabling all acceleration in WITH_MSAN=ON builds.

The InnoDB CRC-32C implementation now uses Intel intrinsic functions. Only in GCC 4 we will have to use __builtin_ functions, because the header <nmmintrin.h> is not compatible with that compiler.

Comment by Marko Mäkelä [ 2020-09-01 ]

I submitted WolfSSL pull request #3268 to use the Intel intrinsic functions for RDRAND and RDSEED whenever applicable and made a test build.

Comment by Marko Mäkelä [ 2020-09-25 ]

My pull request was merged to WolfSSL, and we can actually enable the rdrand and rdseed instructions in WITH_MSAN builds. But, we still must disable the hand-written assembler code until some instrumentation is added to WolfSSL. That should involve invoking __msan_check_mem_is_initialized() on the input and __msan_unpoison() on the output of the assembler code.

Comment by Marko Mäkelä [ 2020-10-03 ]

I submitted another test build with the newest WolfSSL.

Comment by Daniel Black [ 2020-10-06 ]

note: -march flags/test still required (10.3-d99f787244ab82f658b3f4a6c9877289e6385e04

clang++-10.0.1

 
[719/1511] Building CXX object storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o
 
FAILED: storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o 
ccache /usr/bin/clang++  -DBTR_CUR_ADAPT -DBTR_CUR_HASH_ADAPT -DCOMPILER_HINTS -DDBUG_TRACE -DHAVE_CONFIG_H -DHAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE=1 -DHAVE_IB_LINUX_FUTEX=1 -DHAVE_LZ4=1 -DHAVE_LZ4_COMPRESS_DEFAULT=1 -DHAVE_LZMA=1 -DHAVE_NANOSLEEP=1 -DHAVE_SCHED_GETCPU=1 -DHAVE_SNAPPY=1 -DLINUX_NATIVE_AIO=1 -DMUTEX_EVENT -DWITH_INNODB_DISALLOW_WRITES -D_FILE_OFFSET_BITS=64 -Iinclude -I/home/dan/repos/mariadb-server-10.3/storage/innobase/include -I/home/dan/repos/mariadb-server-10.3/storage/innobase/handler -I/home/dan/repos/mariadb-server-10.3/libbinlogevents/include -I/home/dan/repos/mariadb-server-10.3/include -I/home/dan/repos/mariadb-server-10.3/sql -fstack-protector --param=ssp-buffer-size=4 -fno-rtti -O2 -g -DNDEBUG -fsanitize=memory -fsanitize-memory-track-origins -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DDBUG_OFF -Wall -Wdeclaration-after-statement -Wextra -Wformat-security -Wno-init-self -Wno-null-conversion -Wno-unused-parameter -Wno-unused-private-field -Woverloaded-virtual -Wnon-virtual-dtor -Wvla -Wwrite-strings   -DUNIV_LINUX -D_GNU_SOURCE=1  -fvisibility=hidden -MD -MT storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o -MF storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o.d -o storage/innobase/CMakeFiles/innobase.dir/ut/ut0crc32.cc.o -c /home/dan/repos/mariadb-server-10.3/storage/innobase/ut/ut0crc32.cc
 
/home/dan/repos/mariadb-server-10.3/storage/innobase/ut/ut0crc32.cc:202:9: error: '__builtin_ia32_crc32qi' needs target feature sse4.2
 
        *crc = __builtin_ia32_crc32qi(*crc, (*data)[0]);
 
               ^
 
/home/dan/repos/mariadb-server-10.3/storage/innobase/ut/ut0crc32.cc:236:14: error: '__builtin_ia32_crc32di' needs target feature sse4.2
 
        crc_64bit = __builtin_ia32_crc32di(crc_64bit, data);
 
                    ^
 
2 errors generated.

Comment by Marko Mäkelä [ 2020-10-22 ]

danblack, I did not claim to fix earlier versions than 10.5. The main motivation of this is to be able to use WITH_MSAN without any -march. There have been a lot of fixes in 10.5 to the CRC-32 and CRC-32C code that we did not port to earlier versions. Does the 10.3 build fail on a platform where clang is the default compiler, such as FreeBSD or Mac OS X?

Comment by Daniel Black [ 2020-10-23 ]

pushed https://travis-ci.org/github/grooverdan/mariadb-server/builds/738179878 as OSX test with MSAN enabled.

Comment by Marko Mäkelä [ 2020-12-10 ]

danblack, I updated the bb-10.5-MDEV-20386 branch again. I am anticipating a WolfSSL release soon. (In our main branches, we use release tags for the submodule, not ‘random’ commits.)

Comment by Marko Mäkelä [ 2021-01-01 ]

Another test with WolfSSL v4.6.0-stable

Comment by Marko Mäkelä [ 2021-01-01 ]

MDEV-24514 will enable the hand-written assembler code (WOLFSSL_AESNI) when MemorySanitizer is enabled.

Generated at Thu Feb 08 08:59:04 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.