Refactor benchmark to mitigate memory problems #194

andresag01 · 2018-08-13T20:56:57Z

This PR entirely refactors the benchmark example to fix the memory usage issues in Mbed OS (particularly stack overflows). The idea is to run the benchmark for each module in a completely different (noinline) function. Also, some objects are moved to either to the global section or are allocated dynamically.

This PR also includes changes to:

Remove \r\n in favour of \n
Remove the todo struct which was a remainder from when the program was command line (Remove todo from the benchmark application #170)
Check for errors at every stage (benchmark application: TIME_AND_TSC doesn't check return code #172)
Enable more test cases by manually defining more MBEDTLS_* macros in a user config
Consistently use mbedtls_printf (benchmark application uses printf instead of mbedtls_printf #185)
Remove use of memory_buffer_alloc module (MBEDTLS_MEMORY_BUFFER_ALLOC_C feature in benchmark is problematic #171)

Also addresses: #52, #29, #15 and #184

NOTE: This PR is currently under development. I have checked that it works in K64F, but I am waiting for the CI to complete to check whether any other memory issues are detected in the other targets. Regardless, the main structure of the change is in place and can be reviewed.

RonEld · 2018-08-14T05:45:06Z

@andresag01 Before I do a full review, I have a question and a couple of comments:

Refactor benchmark example to reduce stack usage #116 Should have handled most of the memory usage issue. Wasn't this the case
The todo struct is already removed in Remove todo from the benchmark application #170
The usage of the memory_buffer_alloc is already removed in Remove MBEDTLS_MEMORY_BUFFER_ALLOC_C usages #190

Since these last two PRs already exist, I think it would be much simpler to review if you revert your changes related to the todo struct and memory_buffer_alloc

k-stachowiak

I have added some comments to the code. I'd like to finish the review after @andresag01 has addressed @RonEld 's comment, so that I review the right changes.

k-stachowiak · 2018-08-14T12:09:40Z

benchmark/main.cpp

+
+/* Clear some memory that was used to prepare the context */
+#if defined(MBEDTLS_ECP_C)
+void ecp_clear_precomputed(mbedtls_ecp_group *grp) {


I don't know, which coding style is supposed to be used in the examples repository, but both Mbed TLS and Mbed OS expect function braces to be in a separate line

Correct, I am trying to use the Mbed OS style, but missed a lot of cases. I will just run astyle to format everything automatically

k-stachowiak · 2018-08-14T12:14:53Z

benchmark/main.cpp

-    else                                            \
-    {                                               \
-        mbedtls_printf( "%6lu ms/" TYPE, ms );      \
-        MEMORY_MEASURE_PRINT( sizeof( TYPE ) + 1 ); \


It is nuclear to me, why the memory usage printing was removed. Is it due to the changed allocation schemes? Can it be stated that the values acquired from mbedtls_memory_buffer_alloc_max_get( ... ) aren't useful now?

MEMORY_MEASURE_PRINT is dependent on memory_buffer_alloc. However, the way it was used here causes memory issues, so I am removing it. There is also a ticket to do that...

k-stachowiak · 2018-08-14T14:28:51Z

benchmark/main.cpp

-            mbedtls_ecdh_gen_public( &ecdh.grp, &ecdh.d, &ecdh.Qp, myrand, NULL ) != 0 )
-        {
-            return( 1 );
+        ret =mbedtls_ecp_copy( &ecdh.Qp, &ecdh.Q );


A space is missing here

k-stachowiak · 2018-08-14T14:40:41Z

benchmark/main.cpp

+    unsigned long i;                                                        \
+    Timeout t;                                                              \
+                                                                            \
+    mbedtls_printf(HEADER_FORMAT, TITLE);                                   \


The code formatting is not consistent with the previous implementation.

It is not because I am trying to use the Mbed OS style as this is an Mbed OS example

andresag01 · 2018-08-14T17:44:35Z

@RonEld:

#116 Should have handled most of the memory usage issue. Wasn't this the case

I think that #116 was a good step, but looking at the assembly source for GCC the stack allocation was still very high. Also, I investigated further and some primitive overflow the stack even if they are the only benchmark in the function. So I decided to split everything and tag functions with noinline in the hope that this takes care of the problem once and for all. Also, I moved one or two of the most memory consuming contexts to dynamic allocation as it would not work otherwise.

The todo struct is already removed in Remove todo from the benchmark application #170

The usage of the memory_buffer_alloc is already removed in Remove MBEDTLS_MEMORY_BUFFER_ALLOC_C usages #190

My original plan was to just fix the worst memory issues, but then I ended up making so many changes that I just decided to include fixes for those two issues as well here. In fact, this PR is essentially a rewrite of the application rather than a "fix". I included changes for issues such as todo and memory_buffer_alloc because it was simple to do it here and would be more time consuming if we had to resolve conflicts twice across three PRs.

RonEld · 2018-08-15T07:08:21Z

@andresag01

Also, I investigated further and some primitive overflow the stack even if they are the only benchmark in the function.

Yes, Mbed-TLS/mbedtls#1937 should resolve the overflow in the SHA functions

andresag01 · 2018-08-15T07:43:01Z

@RonEld: Actually I did not have an issue with the hash primitives. However, something like Blowfish is more problematic.

RonEld

General comments:

In main(), we shouldn't exit on error. I know this is the current behaviour, but since we are rewriting, let's just go to next test
In main(), There is a combination of BENCHMARK_FUNC_CALL and calling helper functions. I think there should be helper functions for all.
I think a helper function for every algorithm is a little too much, but not a blocker
We should surround the whole helper functions with the relevant #if defined
Please return the implementation of PRINT_ERROR and call this MACRO upon failure, for better error code readability
I am currently workiong on a feature not available PR (in the mbedtls repository), but it would be nice if the existing FEATURE NOT SUPPORTED error codes would be already integrated here

RonEld · 2018-08-15T07:56:40Z

benchmark/main.cpp

+    for (i = 1, alarmed = 0, t.attach(alarm, 1.0); !alarmed; i++)           \
+    {                                                                       \
+        if ((ret = (CODE)) != 0) {                                          \
+            mbedtls_printf("%s returned -0x%04X\n", #CODE, -ret);           \


It should be like the previous implementation of PRINT_ERROR, in case MBEDTLS_ERROR_C is defined, call mbedtls_strerror for better readability of the error code

RonEld · 2018-08-15T07:57:28Z

benchmark/main.cpp

-#endif
+MBED_NOINLINE static int benchmark_arc4() {
+    int ret = 0;
+#if defined(MBEDTLS_ARC4_C)


why not surround the whole function with #if defined(MBEDTLS_ARC4_C) ?

Because then I would have to duplicate the preprocessor check here and in the main() function.

I know, but it would still result in a smaller code, and not calling empty functions. It depends on the compiler, if it can optimize these functions out

I think code size here is not really a problem. When the feature is not defined as a macro then the compiler (GCC) will generate the following code for an empty function:

0000181c <benchmark_havege()>: 181c: 2000 movs r0, #0 181e: 4770 bx lr

Then there will be the following call in the main:

36aa: f7fe f8b7 bl 181c <benchmark_havege()> 36ae: b9b8 cbnz r0, 36e0 <main+0x37c>

Also, I checked and when multiple functions are empty, then the compiler will reuse one of the empty functions instead of emitting multiple ones to reduce code size. In any case, the overhead is 2.5 words (10 bytes) which take about 8-10 clock cycles to execute in a Cortex-M0...

Have you tried all toolchains? ARM, GCC_ARM and IAR?

I tried one version of GCC_ARM only. However, I do not expect the code to be too different with the others. After all, if a feature is not enabled, its corresponding function will be empty and there is only one assignment and a return in there. Then in main there is only a call to that function and a check which map to the instructions above (or something similar to that).

I agree there shouldn't be much difference, personally, I prefer not to have a call to empty functions, but that's a matter of style, it's not a blocker for me

RonEld · 2018-08-15T07:57:52Z

benchmark/main.cpp

-    if( todo->md5 )
-        TIME_AND_TSC( "MD5", mbedtls_md5( buf, BUFSIZE, tmp ) );
-#endif
+MBED_NOINLINE static int benchmark_des3() {


why not surround the whole function with #if defined(MBEDTLS_DES_C) && defined(MBEDTLS_CIPHER_MODE_CBC) ?

Please refer to my other comment

RonEld · 2018-08-15T07:58:20Z

benchmark/main.cpp

-    // Please refer to https://github.com/ARMmbed/mbedtls/issues/1200 for more information.
-    (void)ctx;
-    memset( tmp, 0xBB, sizeof( tmp ) );
+MBED_NOINLINE static int benchmark_des() {


why not surround the whole function with #if defined(MBEDTLS_DES_C) && defined(MBEDTLS_CIPHER_MODE_CBC) ?

RonEld · 2018-08-15T07:58:48Z

benchmark/main.cpp


-        memset( buf, 0, sizeof( buf ) );
-        memset( tmp, 0, sizeof( tmp ) );
+MBED_NOINLINE static int benchmark_des3_cmac() {


why not surround the whole function with #if defined(MBEDTLS_DES_C) && defined(MBEDTLS_CIPHER_MODE_CBC) && defined(MBEDTLS_CMAC_C) ?

RonEld · 2018-08-15T08:02:14Z

benchmark/main.cpp

-            mbedtls_gcm_free( &gcm );
+        ret = mbedtls_gcm_setkey(&gcm, MBEDTLS_CIPHER_ID_AES, tmp, keysize);
+        if (ret != 0) {
+            mbedtls_printf("mbedtls_gcm_setkey() returned -0x%04X\n", -ret);


Please skip in case of feature not supported

I think AES has the feature unavailable macro error code, but as far as I know GCM does not have it yet. I think the PR that adds it is: Mbed-TLS/mbedtls#1716. Is this correct? If so, then I will not add the check because the symbol MBEDTLS_ERR_GCM_FEATURE_UNAVAILABLE does not exist yet. Or is it possible that this function returns MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE?

The problem here is, GCM can return either MBEDTLS_AES_FEATURE_UNAVAILABLE, if no alternative GCM exists, or the undefined yet MBEDTLS_GCM_FEATURE_UNAVAILABLE, if there is an alternative gcm.

Thanks for clarifying. I have added the checks for the AES feature unavailable only.

RonEld · 2018-08-15T08:02:36Z

benchmark/main.cpp

+
+MBED_NOINLINE static int benchmark_aes_ccm() {
+    int ret = 0;
+#if defined(MBEDTLS_AES_C) && defined(MBEDTLS_CCM_C)


why not surround the whole function with #if defined(MBEDTLS_AES_C) && defined(MBEDTLS_CCM_C)?

RonEld · 2018-08-15T08:03:00Z

benchmark/main.cpp

+
+MBED_NOINLINE static int benchmark_aes_cmac() {
+    int ret = 0;
+#if defined(MBEDTLS_AES_C) && defined(MBEDTLS_CMAC_C)


why not surround the whole function with #if defined(MBEDTLS_AES_C) && defined(MBEDTLS_CMAC_C)?

RonEld · 2018-08-15T08:03:38Z

benchmark/main.cpp

+    return ret;
+}
+
+MBED_NOINLINE static int benchmark_camellia() {


why not surround the whole function with #if defined(MBEDTLS_CAMELLIA_C) && defined(MBEDTLS_CIPHER_MODE_CBC)?

RonEld · 2018-08-15T08:09:29Z

benchmark/main.cpp

        }

-        TIME_PUBLIC(  "ECDHE-Curve25519"


I know this is the original behaviour, but if we are alreadey doing a rewrite, please don't exit on failure, we should continue to next test.

RonEld · 2018-08-15T08:39:57Z

@andresag01 The FEATURE_NOT_SUPPORTED support is nice to have, but I will work on it once Mbed-TLS/mbedtls#1716 is merged, so this isn't a blocker for this PR

andresag01 · 2018-08-15T09:51:14Z

@RonEld: I believe I have addressed all (or at least most of) your comments.

andresag01 · 2018-08-15T10:03:54Z

I have tested the changes in K64F with GCC_ARM and it seems to work fine. However, when I try it with ARMCC I get the following errors:

mbedtls_hmac_drbg_seed() returned -0x5180
  !  MD - Failed to allocate memory
mbedtls_pk_parse_key() returned -0x1180
  !  PEM - Failed to allocate memory
mbedtls_mpi_read_string() returned -0x0010
  !  BIGNUM - Memory allocation failed
mbedtls_ecdsa_genkey() returned -0xFFFFFFFF
  !  UNKNOWN ERROR CODE (0001)
mbedtls_ecdh_make_public() returned -0x0010
  !  BIGNUM - Memory allocation failed
mbedtls_ecp_group_load() returned -0x0010
  !  BIGNUM - Memory allocation failed

It now seems that we are running out of heap, but I am not very sure why it succeeds in GCC_ARM if the size of data + bss is larger than with ARMCC, so I would expect the heap space to be smaller. I will take a look at that as soon as I get the change.

RonEld

I haven't specifically mentioned CCM and CMAC as possible to get MBEDTLS_AES_FEATURE_NOT_SUPPORTED.
Sorry for not mentioning it specifically before

RonEld · 2018-08-15T10:29:20Z

benchmark/main.cpp

+        memset(tmp, 0, sizeof(tmp));
+
+        ret = mbedtls_ccm_setkey(&ccm, MBEDTLS_CIPHER_ID_AES, tmp, keysize);
+        if (ret != 0) {


CCM could also return MBEDTLS_AES_FEATURE_NOT_SUPPORTED in case no alternative CCM

RonEld · 2018-08-15T10:30:17Z

benchmark/main.cpp

+            goto exit;
+        }
+
+        BENCHMARK_FUNC_CALL(title,


CMAC could also return MBEDTLS_AES_FEATURE_NOT_SUPPORTED in case no alternative CMAC.
This is problematic because of the MACRO though

Is this still a problem?

Unfortunately, the macro does make it a bit awkward to add the extra checks. I did add this line of code to check for MBEDTLS_AES_FEATURE_NOT_SUPPORTED on calls like this.

I agree that the code is not ideal, but my main motivation for this refactor was to patch up the reliability issues. So I tried to minimize the development time by no changing all these macros. Perhaps we should consider replacing them when it is needed to check more feature not supported errors.

RonEld · 2018-08-16T14:00:13Z

Note this PR also fixes an issue of failure in hmac_drbg , when MBEDTLS_THREADING_C is defined, as described in Mbed-TLS/mbedtls#1758 (review)

andresag01 · 2018-08-16T17:27:41Z

@RonEld: Please note that this fix is already included in this PR

andresag01 · 2018-08-16T17:30:38Z

The failures currently reported by the Ci are unrelated to this change. There seems to be an issue with the CI

andresag01 · 2018-08-22T08:51:56Z

@RonEld: I added the extra AES unavailable checks to CCM, CMAC and CTR_DRBG

Patater · 2018-09-03T14:13:40Z

benchmark/main.cpp

+
+static unsigned char buf[BUFSIZE];
+static unsigned char tmp[200];
+static char title[TITLE_LEN];


This is a lot of static memory use. Why not use heap or stack instead?

Do we need tmp and buf at the same time, or could the users of what's currently buf use tmp instead?

tmp and buf are used at the same time. buf is the buffer of length 1024 that is the input \ output of the current operation.
tmp is an additional buffer used for the current operation, such as key, iv, signature etc..
I think, however, that the size of tmp should be decreased significantly. I am not sure htat more than 32 bytes is really used

This is a lot of static memory use. Why not use heap or stack instead?

I would like to avoid putting these into the stack because that would increase the likely of stack overflows, which is one of the motivations for this PR. These are currently as globals because there are shared by all functions, and in any case they were allocated statically in the original benchmark, so I just left it like that. I could move it to the heap if you feel strongly about it, I don't mind either way...

Do we need tmp and buf at the same time, or could the users of what's currently buf use tmp instead?

As @RonEld explained, we do need both buf and tmp. I will try to reduce the buffer sizes though.

Have you tried reducing the tmp buffer size? From my brief look, it is used for small buffers, such as keys and ivs.
I think that 150 bytes can also be reduced. Couldn't 64 bytes be enough?

Patater · 2018-09-03T14:14:04Z

benchmark/main.cpp

        use_len = len;
-        if( use_len > sizeof(int) )
+        if(use_len > sizeof(int)) {


Add a space after if.

I think this review comment is based on an old commit. Please note that later on there is a commit that fixes these style problems by running astyle with the Mbed OS configuration file.

Patater · 2018-09-03T14:16:38Z

benchmark/main.cpp

-#else
-#define ecp_clear_precomputed( g )
-#endif
+MBED_NOINLINE static int benchmark_arc4() {


Why "NOINLINE"? If you allow the compiler to inline this, and it ends up as an empty function (by configuration choices), there shouldn't be any effect (no stub function made that does nothing).

The NOINLINE is because without it the compiler will inline functions into main() with high optimization levels and dramatically increase the stack size causing the application to fail...

In a later commit I changed location of the preprocessor checks to surround the entire function and the call in main, so there will not be an empty function added when the feature is disabled.

Note that @RonEld had a similar comment, so I changed this. As I discussed with him, I prefer the other approach because it decreases the number of preprocessor checks with only negligible effect in code size and runtime. Please find below my answer to @RonEld's original comment:

I think code size here is not really a problem. When the feature is not defined as a macro then the compiler (GCC) will generate the following code for an empty function:

0000181c <benchmark_havege()>: 181c: 2000 movs r0, #0 181e: 4770 bx lr

Then there will be the following call in the main:

36aa: f7fe f8b7 bl 181c <benchmark_havege()> 36ae: b9b8 cbnz r0, 36e0 <main+0x37c>

Also, I checked and when multiple functions are empty, then the compiler will reuse one of the empty functions instead of emitting multiple ones to reduce code size. In any case, the overhead is 2.5 words (10 bytes) which take about 8-10 clock cycles to execute in a Cortex-M0...

Too bad the compiler doesn't realize it doesn't have enough stack available to inline functions (or not). OK.

Patater · 2018-09-03T14:22:07Z

benchmark/main.cpp

-            mbedtls_ecdh_gen_public( &ecdh.grp, &ecdh.d, &ecdh.Qp, myrand, NULL ) != 0 )
-        {
-            return( 1 );
+        ret =mbedtls_ecp_copy( &ecdh.Qp, &ecdh.Q );


This isn't Mbed OS style...

I think this review comment is based on an old commit. Please note that later on there is a commit that fixes these style problems by running astyle with the Mbed OS configuration file.

Patater · 2018-09-03T14:25:44Z

benchmark/main.cpp

-#define PRINT_ERROR                                            \
-        mbedtls_strerror( ret, ( char * )tmp, sizeof( tmp ) ); \
-        mbedtls_printf( "FAILED: %s\r\n", tmp );
+#define BENCHMARK_FUNC_CALL(TITLE, CODE)                                    \


For additional code savings, and easier maintainability (no giant macros), this could be made into a static function that accepts a function pointer and a string or two. This would mean wrapping a function call you want to benchmark with another function that doesn't take any parameters.

I agree, and perhaps adding a list of the optional valid return codes that the function can return, such as the different FEATURE_UNAVAILABLE errors ( TBD for the implementation of the return codes)

@Patater, @RonEld: I understand your concerns, but there were a lot of other issues to fix with this application. While code size is important, the main problems I had to look into was the RAM problems. To save time I decided to not do something like what you are suggesting and preserve the main structure of each test case, but inside separate functions.

How strongly about this? Should we just leave it like this for now?

Did fixing the RAM issues increase the code footprint? If not, I think we can pursue additional code savings later.

Unfortunately the code in the latest version of this PR is not really comparable with the original version of the application because a lot more code has been added to improve reliability (for example, all the error checks). However, I am fairly certain that if we removed this, the code would have increased because there are a lot more function prologues and epilogues, but I expect the amount of code regarding these macros to be relatively similar.

RonEld

I have added a couple of observation on alternative CCM and GCM. No action required.

However, I believe that returning the MBEDTLS_AES_FEATURE_UNAVAILABLE on 256 bit key will cause the benchmark to stop operation, instead of skipping the test.

Also, ctr_drbg uses 256 bit keys (until Mbed-TLS/mbedtls#1902 is merged) so AES Feature unavailable for ctr_drbg code is not a valid error and should not have special treatment, IMHO.

Another suggestion \ question was about reusing the title buffer

RonEld · 2018-09-03T14:16:52Z

benchmark/main.cpp

@@ -156,7 +156,11 @@ do {                                                                        \
                                                                            \
    for (i = 1, alarmed = 0, t.attach(alarm, 1.0); !alarmed; i++)           \
    {                                                                       \
-        if ((ret = (CODE)) != 0) {                                          \
+        ret = CODE;                                                         \
+        if (ret == MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE) {                   \


Have you tested this with changing the AES code to return MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE on 256 bit key? I think this will cause the benchmark to stop instead of skipping the test.

In addition, in future Unavailable errors, this will probably not work when we don't loop and overwrite the ret in next call, but this is not part of this PR

Have you tested this with changing the AES code to return MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE on 256 bit key? I think this will cause the benchmark to stop instead of skipping the test.

I did test it and it seems to work.

In addition, in future Unavailable errors, this will probably not work when we don't loop and overwrite the ret in next call, but this is not part of this PR

Not sure what you mean. Please bear in mind that this piece of code is inside a while(0) loop. The break statement exits that while(0) (which is in the macro and will always be there. I dont think it really matters whether the whole macro itself is used within a loop or not.

what I mean is, for AES256, for example:
Let's say that AES256_CBC will return the MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE , so the set key will succeed, but the mbedtls_aes_crypt_cbc will return the error code. ret is now the error MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE , and we finish the loop in line 435 causing the function benchmark_aes_cbc() to return an error code, changing the exit_code to MBEDTLS_EXIT_FAILURE. We will not exit the test, but I wouldn't consider this a failure.

As for other modules, which will have a future NOT SUPPORTED error, it's basically the same, the specific benchmark test function will return an error code, even if we don't really consider a FEATURE UNAVAILABLE error a real error code

Ok, I see what you mean. I will fix that.

RonEld · 2018-09-03T14:18:56Z

benchmark/main.cpp

@@ -519,6 +527,8 @@ MBED_NOINLINE static int benchmark_aes_gcm()

        ret = mbedtls_gcm_setkey(&gcm, MBEDTLS_CIPHER_ID_AES, tmp, keysize);
        if (ret == MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE) {
+            /* Do not consider this as a failure */


Note: Obviously this will work only for AES feature unavailable, and not in case an alternative gcm implementation returns the uncreated yet MBEDTLS_GCM_FEATURE_UNAVAILABLE

This is not a review comment, but an notice for future implementation

RonEld · 2018-09-03T14:19:09Z

benchmark/main.cpp

@@ -560,7 +570,11 @@ MBED_NOINLINE static int benchmark_aes_ccm()
        memset(tmp, 0, sizeof(tmp));

        ret = mbedtls_ccm_setkey(&ccm, MBEDTLS_CIPHER_ID_AES, tmp, keysize);
-        if (ret != 0) {
+        if (ret == MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE) {


Note: Obviously this will work only for AES feature unavailable, and not in case an alternative ccm implementation returns the uncreated yet MBEDTLS_CCM_FEATURE_UNAVAILABLE

This is not a review comment, but an notice for future implementation

RonEld · 2018-09-03T14:22:33Z

benchmark/main.cpp

@@ -747,6 +747,7 @@ MBED_NOINLINE static int benchmark_ctr_drbg()
 {
    int ret = 0;
    const char *nopr_title = "CTR_DRBG (NOPR)";
+    const char *pr_title = "CTR_DRBG (PR)";


why not use the existing title buffer, and just use the snprintf to write to it the relevant title?

I am not sure what is the point of using title and snprintf(). There is nothing to format here, these two variables are just a short hand to not have to write the string twice in the function body.

RonEld · 2018-09-03T14:24:10Z

benchmark/main.cpp

@@ -755,6 +756,7 @@ MBED_NOINLINE static int benchmark_ctr_drbg()
    if (ret == MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE) {


MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE is not a recoverable error, for ctr_drbg, so I don't think it requires special treatment for this error

I am not sure what you mean. The special treatment is only so that the console printout shows up nicely in the terminal. Perhaps there is something I am missing?...

I think that in all the other MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE we should print and skip the specific test, as it's a reasonable error to return.
For ctr_drbg, it means that ctr_drbg will fail, as it uses AES256 only, unless configured to 128 bit, as introduced in Mbed-TLS/mbedtls#1902
So, this error is not a valid error for ctr_drbg, IMHO.
If it's only for the printout, well, the PRINT_ERROR will handle this, no?

I still do not understand what the change request is here. We are already printing out the test and the string "Feature Unavailable" when AES is not available. This is equivalent to skip because we are not letting the benchmark application failed for that. I do not see the point of printing the actual error string corresponding to MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE and this would be inconsistent with the checks in other parts of this application

Because, when we skip a test, it's a valid error, and the return code of the benchmark should be success.
On ctr_drbg, on the other hand, when MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE is returned, it is a real error, and we should fail the test, and developers should be aware that they will need to modify thei ctr_drbg to use 128 bit key size(after Mbed-TLS/mbedtls#1902 is merged)

Ok, I see what you mean, but I do not agree with it. If ctr_drbg does not run because it has no AES that is not a bug or a real failure according to what we have done in the benchmark application. Its just that the primitive is unavailable under the current configuration. Isnt this comparable to saying that AES-256 is not available so its an actual error because the user should change to something like AES-128?

No, because for ctr_drbg, the usage of AES256 is embedded within the algorithm and not configurable, and not under the user's discretion.
After it will be configurable to 128, I still think that it should be an error, and not just skipped, because it's an algorithm using AES, and not just a primitive AES, but I won't insist on it, after it will be configurable

It's not configurable to use AES-128, yet. After it is configurable, we can make this an error. Is any further action needed here?

I think that we should leave it as it is until the new feature is merged. @RonEld, would you still like to pursue the change? I am happy to make the change if you think its necessary.

Correct, this is not configurable. This is why It is a failure error, that should cause the CI to fail, and we should not have special treatment for this specific error, in case of ctr drbg

andresag01 · 2018-10-08T20:49:45Z

Rebased the changes to sync with latest merged to master.

Review comments have had responses.

simonbutcher · 2018-10-15T14:47:46Z

@RonEld - Can you please re-review? We need to get this merged as the fixes will restore us to working on some broken targets.

andresag01 · 2018-10-17T10:07:38Z

@sbutcher-arm: This PR already has 2 approvals, so removing the 'needs review' tag.

simonbutcher · 2018-10-18T09:28:59Z

retest

simonbutcher · 2018-10-18T10:51:28Z

So this PR seems to fix the tests previously failing on the following targets:

build-benchmark-K64F-ARM
build-benchmark-NUCLEO_F429ZI-ARM
build-benchmark-KW24D-ARM

The top two were memory problems with ECDSA, and the final one build-benchmark-KW24D-ARM failed with ECDHE.

However, we still have failing targets:

build-tls-client-UBLOX_EVK_ODIN_W2-IAR
build-tls-client-UBLOX_EVK_ODIN_W2-GCC_ARM
build-tls-client-UBLOX_EVK_ODIN_W2-ARM
build-tls-client-NUCLEO_F429ZI-GCC_ARM
build-benchmark-KW24D-IAR

The KW24D just doesn't have enough memory for the example, so we'll need to work out a way of excluding that test. The others are network problems, and I suspect build-tls-client-NUCLEO_F429ZI-GCC_ARM was just a transient error, because the test is fragile to network problems.

So this PR does fix what it said it did and is good to merge, but unfortunately reveals that some failing tests were hiding other failing tests. So that means it's good to merge - but we have more bugs to fix on some targets. I'll raise those separately.

simonbutcher · 2018-10-18T10:52:21Z

retest

simonbutcher · 2018-10-18T10:52:42Z

Retesting to see if the issue with build-tls-client-NUCLEO_F429ZI-GCC_ARM goes away on a second go.

andresag01 added the enhancement label Aug 13, 2018

andresag01 requested review from k-stachowiak and RonEld August 13, 2018 20:56

andresag01 added the runCI label Aug 13, 2018

k-stachowiak reviewed Aug 14, 2018

View reviewed changes

This was referenced Aug 15, 2018

Remove todo struct #187

Closed

Remove MBEDTLS_MEMORY_BUFFER_ALLOC_C usages #190

Closed

RonEld mentioned this pull request Aug 15, 2018

Check code error code in TIME_AND_TSC #191

Closed

RonEld suggested changes Aug 15, 2018

View reviewed changes

andresag01 removed the runCI label Aug 15, 2018

andresag01 added the runCI label Aug 15, 2018

RonEld suggested changes Aug 15, 2018

View reviewed changes

RonEld mentioned this pull request Aug 15, 2018

use mbedtls_printf instead of printf #186

Closed

andresag01 mentioned this pull request Aug 22, 2018

Revert addition of ecdsa test #189

Closed

simonbutcher requested a review from Patater September 2, 2018 21:46

Patater previously requested changes Sep 3, 2018

View reviewed changes

RonEld suggested changes Sep 3, 2018

View reviewed changes

andresag01 added runCI and removed runCI labels Sep 4, 2018

Andres Amaya Garcia and others added 4 commits October 8, 2018 21:46

benchmark: Fix typo in arguments to sizeof()

31ebb42

benchmark: reduce size of tmp buffer to save memory

54ea62b

benchmark: Dont return error on feature unavailable

52ffc01

benchmark: Make CTR_DRBG fail on feature unavailable

c1a1abe

andresag01 force-pushed the iotssl-2430-benchmark-refactor branch from d0858c1 to c1a1abe Compare October 8, 2018 20:48

RonEld approved these changes Oct 15, 2018

View reviewed changes

andresag01 removed the needs: review label Oct 17, 2018

simonbutcher added the needs: CI label Oct 18, 2018

simonbutcher added ready for merge and removed needs: CI labels Oct 18, 2018

simonbutcher merged commit 0e4f64e into ARMmbed:master Oct 18, 2018

This was referenced Oct 18, 2018

[OOB_5_4] Benchmark program hangs at "RSA-2048" #52

Closed

benchmark example does not finish correctly in NCS36510 dev board #29

Closed

Assertation failed in the benchmark example #15

Closed

Benchmark example fails on K64F with ARM C Compiler #184

Closed

andresag01 mentioned this pull request Oct 31, 2018

Memory allocation failure in benchmark example IAR + KW24D #138

Open

		@@ -755,6 +756,7 @@ MBED_NOINLINE static int benchmark_ctr_drbg()
		if (ret == MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE) {

Refactor benchmark to mitigate memory problems #194

Refactor benchmark to mitigate memory problems #194

Conversation

andresag01 commented Aug 13, 2018 • edited Loading

RonEld commented Aug 14, 2018

k-stachowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Aug 14, 2018 • edited Loading

RonEld commented Aug 15, 2018 • edited Loading

andresag01 commented Aug 15, 2018

RonEld left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Aug 15, 2018 • edited Loading

andresag01 commented Aug 15, 2018

andresag01 commented Aug 15, 2018

RonEld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld commented Aug 16, 2018 • edited Loading

andresag01 commented Aug 16, 2018

andresag01 commented Aug 16, 2018

andresag01 commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RonEld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 commented Aug 13, 2018 •

edited

Loading

andresag01 commented Aug 14, 2018 •

edited

Loading

RonEld commented Aug 15, 2018 •

edited

Loading

RonEld left a comment •

edited

Loading

RonEld commented Aug 15, 2018 •

edited

Loading

RonEld commented Aug 16, 2018 •

edited

Loading

andresag01 Sep 4, 2018 •

edited

Loading