Refactor benchmark example to reduce stack usage #116

RonEld · 2017-09-19T10:50:37Z

Seperate the tests to different hepler functions, according to
their module
Enable ECDSA tests

Tested on K64F and NRF52840_DK

k-stachowiak

I checked what I could at my knowledge level:

missed options that have no implementation
resources not being released
I found no issue of that sort.
I have one usability related suggestion.

k-stachowiak · 2017-10-10T09:31:11Z

benchmark/main.cpp

-    else
+#if defined(MBEDTLS_RSA_C) && \
+    defined(MBEDTLS_PEM_PARSE_C) && defined(MBEDTLS_PK_PARSE_C)
+    if( todo->rsa )


This program may benefit from printing warnings if the user requests a given test via CLI but it is compiled out. Currently it skips it silently.

@k-stachowiak Perhaps, but this PR is only to refactor the code into different test functions, to reduce stack size

AndrzejKurek · 2017-10-19T12:44:41Z

Changes look fine to me, but just as a note for future pull requests - it would be nice to split it into two commits - first, moving the code around without changes, and second - introducing these changes.
Doing it as it is done here makes it very hard to review the changes, as github reports only random chunks removed and added at various lines, while they are only moved.

Patater · 2018-05-09T11:22:51Z

This PR needs rebasing, as benchmark has undergone some changes.

Rewrite after a rebase with many conflicts

RonEld · 2018-05-13T14:56:01Z

There were too many conflicts, so I have rewritten the PR.
Note that it is only moving the tests to different functions, so no other changes \ reordering

1. update copyright year. 2. Enable ecdsa. 3. Change `hlen` parameter in the ecdsa test to byte size, as this is what the API expects. 4. Remove unused variables.

Add the platform context to the test functions, as these are the functions handling the cryptographic modules.

RonEld · 2018-05-13T15:24:04Z

@AndrzejKurek I have rewritten the PR, as there were too many conflicts in rebase. Please re-review.
I have also added support for the platform context.

@k-stachowiak could you please specify where there are resources not being freed? If you found some memory leaks, I think it would be a good idea to open a separate issue, as I didn't really modify code, except the ecdsa code that didn't work on my HW accelerator.

Have the test functions return error code, to avoid resource leak of the platform context, and for cleaner exit.

AndrzejKurek · 2018-05-14T10:39:26Z

benchmark/main.cpp

@@ -145,7 +145,7 @@
 /*
 * Uncomment this line to enable ECDSA benchmark.
 */
-//#define ENABLE_ECDSA
+#define ENABLE_ECDSA


Should this be enabled? It is more than just reducing stack size, its a functional change.

The reason it was disabled was because ECDSA caused stack overflow.
Now, it can be returned.
I think we can remove the whole ENABLE_ECDSA part, once this will be tested on numerous targets

This change breaks the example for the ARM Compiler on K64F! We should have tested that before merging. It's especially obvious to test because it's what the CI tests!

cc: @mazimkhan

I have tested it on K64F and on NRF52840.
If CI is still broken, we can return this change.

AndrzejKurek · 2018-05-14T10:52:04Z

benchmark/main.cpp

@@ -819,7 +777,7 @@ static int benchmark( int argc, char *argv[], mbedtls_platform_context* ctx )
            mbedtls_snprintf( title, sizeof( title ), "ECDSA-%s",
                                              curve_info->name );
            TIME_PUBLIC( title, "sign",
-                    ret = mbedtls_ecdsa_write_signature( &ecdsa, MBEDTLS_MD_SHA256, buf, curve_info->bit_size,
+                    ret = mbedtls_ecdsa_write_signature( &ecdsa, MBEDTLS_MD_SHA256, buf, ( curve_info->bit_size + 7 ) / 8,


Was the previous buffer size simply inaccurate, or we don't have so much space? If this amount only plays a role in checking whether the buffer is big enough, why don't we pass the real buffer size?

The parameter is hlen not buffer length. So, sizeof(buf) is inaccurate.
The previous buffer size was sent as bits, not bytes, but the API expects length in bytes.
SInce this is a benchmark application, it didn't verify with expected result, and the SW implementation succeeded. I noticed this issue, when a HW accelerator expects only specific standard lengths, so it returned an error

AndrzejKurek

Besides the malloc_buf allocation that's not freed, and probably should be raised as a bug, this PR is ok.

RonEld · 2018-05-14T11:09:18Z

@AndrzejKurek Thanks for the review. I did consider adding a free to the malloc_buf allocation, but since it's not defined on Mbed OS(at least no the targets I checked), and it's a functional change, I didn't add it

andresag01

This PR is mostly a straight forward split of the previous long benchmark function to reduce stack usage. And I am happy with these changes. However, I pointed out several other (mostly preexisting issues) that would be good to fix, but I do not think they are a blocker because they do were already there.

Also pointed out a simple style issue, but this is not a blocker either...

andresag01 · 2018-05-15T19:11:06Z

benchmark/main.cpp

+    int i;
+    todo_list todo;
+#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C)
+    unsigned char malloc_buf[HEAP_SIZE] = { 0 };


I do not think this is going to work. This line will allocate a 64KB array on the stack that will then be managed by Mbed TLS. However, the default stack size in Mbed OS is 4KB and it almost certainly cause problems. I suggest removing the use of MBEDTLS_MEMORY_BUFFER_ALLOC_C from this example. The embedded platforms that this example is targeting cannot cope with these huge allocations. If you still want to make it work, you can probably look up the appropriate symbols in the linker files and pass it real pointers to the heap. Even then, this might cause trouble if other parts of the system use the real malloc/calloc...

andresag01 · 2018-05-15T19:15:28Z

benchmark/main.cpp

+        for( i = 1; i < argc; i++ )
+        {
+            if( strcmp( argv[i], "md4" ) == 0 )
+                todo.md4 = 1;


I think this todo struct is a trace of the origin of this example, that is, the benchmark application in mbedtls/programs/benchmark.c. That application was meant for command line usage, so it made sense to provide an interface to enable/disable parts of the benchmark. However, this app is meant for an embedded platform and (as far as I remember) these values have never been changed (proof of that is that we used ENABLE_ECDSA instead of flicking the appropriate todo bit). I think this feature is entirely redundant and causes clutter. I suggest removing the todo struct and all its associated code.

I agree it's redundant and not needed. Opened #170 to track it,
Side note: ENABLE_ECDSA was used to reduce stack size. Flipping the todo bit would keep the code compiled and keep the stack usage.

andresag01 · 2018-05-15T19:15:52Z

benchmark/main.cpp

+#endif
+    memset( buf, 0xAA, sizeof( buf ) );
+
+    if( test_md( &todo, ctx ) != 0)


style: Please add before ).

andresag01 · 2018-05-15T19:17:58Z

benchmark/main.cpp

+    return ( 0 );
+}
+
+static int test_rng( const todo_list * todo, mbedtls_platform_context* ctx )


I like the idea of this function returning an error code. However, as far as I can tell, this is currently not properly set in all cases (same situation with all the other test_*()). It would be great to properly set the value, specially if there are more platforms with hardware acceleration support.

I don't understand what you mean. Do you mean to set a ret variable and set its value according to the crypto functions? I agree it should be done, but I think it should be part of a different PR, as this PR is only refactoring the function calls to separate functions, to reduce stack size

Yep, thats what I mean.
Agreed, preexisting and different issue

andresag01 · 2018-05-15T19:20:48Z

benchmark/main.cpp

    memset( tmp, 0xBB, sizeof( tmp ) );

 #if defined(MBEDTLS_MD4_C)
-    if( todo.md4 )
+    if( todo->md4 )
        TIME_AND_TSC( "MD4", mbedtls_md4( buf, BUFSIZE, tmp ) );


I dont think the return code in any of these is being checked.

Correct, but this is pre-existing behavior. Opened #172 to track this

andresag01 · 2018-05-15T19:21:25Z

benchmark/main.cpp


        if( mbedtls_ctr_drbg_seed( &ctr_drbg, myrand, NULL, NULL, 0 ) != 0 )
            return(1);
        mbedtls_ctr_drbg_set_prediction_resistance( &ctr_drbg, MBEDTLS_CTR_DRBG_PR_ON );
        TIME_AND_TSC( "CTR_DRBG (PR)",
                if( mbedtls_ctr_drbg_random( &ctr_drbg, buf, BUFSIZE ) != 0 )
-                return(1) );
+                    return(1) );


I think the return value in all these is lost...

why? this return is part of the CODE being set in the TIME_AND_TSC macro

It is returning 1 in all cases, so if it fails, the user would not know the cause. That is what I mean by "lost"

RonEld · 2018-05-23T12:11:59Z

@andresag01 Thank you for your comments.
Note this PR is only refactoring(and enabling ECDSA). No other preexisting issue was addressed and I opened issues to track those

andresag01 · 2018-05-23T14:03:25Z

@RonEld: I agree. As I mentioned in my review comments, I do not think these are blockers because they are preexisting issues.

k-stachowiak · 2018-05-24T07:23:10Z

could you please specify where there are resources not being freed? If you found some memory leaks, I think it would be a good idea to open a separate issue, as I didn't really modify code, except the ecdsa code that didn't work on my HW accelerator.

My comment only stated, what I was checking for, including unreleased resources. Let me confirm, that I have found no issues of that kind.

RonEld · 2018-05-24T07:48:56Z

@k-stachowiak Thanks for confirming.
@andresag01 I believe I addressed all you issues

andresag01

@RonEld: As discussed, most of my comments are related to preexisting issues and are best addressed in another PR. There is just one or two style issues, but that is not a blocker, so I am approving this PR.

RonEld requested review from andresag01 and hanno-becker September 19, 2017 10:50

RonEld force-pushed the benchmark_refactor branch from a4d7dbf to 76b3bc7 Compare September 19, 2017 11:45

andresag01 added enhancement mbed TLS team labels Sep 20, 2017

andresag01 requested review from AndrzejKurek and removed request for hanno-becker October 6, 2017 15:46

andresag01 added the needs: review label Oct 6, 2017

k-stachowiak reviewed Oct 10, 2017

View reviewed changes

AndrzejKurek approved these changes Oct 19, 2017

View reviewed changes

RonEld mentioned this pull request Apr 17, 2018

Add platform setup and teardown support #165

Merged

Patater added needs: rework and removed needs: review labels May 9, 2018

Rewrite changes after rebase

a198d9b

Rewrite after a rebase with many conflicts

RonEld force-pushed the benchmark_refactor branch from 76b3bc7 to a198d9b Compare May 13, 2018 14:55

Ron Eldor added 2 commits May 13, 2018 18:12

Some minor modifications after refactoring

4582a93

1. update copyright year. 2. Enable ecdsa. 3. Change `hlen` parameter in the ecdsa test to byte size, as this is what the API expects. 4. Remove unused variables.

Add platform context to the test functions

d86a33b

Add the platform context to the test functions, as these are the functions handling the cryptographic modules.

RonEld added needs: review and removed needs: rework labels May 13, 2018

make test function return error code

33786fe

Have the test functions return error code, to avoid resource leak of the platform context, and for cleaner exit.

AndrzejKurek reviewed May 14, 2018

View reviewed changes

AndrzejKurek approved these changes May 14, 2018

View reviewed changes

andresag01 reviewed May 15, 2018

View reviewed changes

andresag01 approved these changes May 30, 2018

View reviewed changes

RonEld changed the title ~~Refactor benchmark example to reduce stack size~~ Refactor benchmark example to reduce stack usage May 31, 2018

RonEld mentioned this pull request Jun 10, 2018

mbedTLS causes stack overflow on some mbedOS targets #14

Closed

RonEld removed the needs: review label Jun 11, 2018

k-stachowiak added runCI and removed runCI labels Jul 19, 2018

k-stachowiak merged commit d576d43 into ARMmbed:master Jul 19, 2018

simonbutcher mentioned this pull request Jul 25, 2018

Benchmark example fails on K64F with ARM C Compiler #184

Closed

This was referenced Jul 26, 2018

MBEDTLS_MEMORY_BUFFER_ALLOC_C feature in benchmark is problematic #171

Closed

Benchmark example built with make_iar thows runtime error #92

Closed

RonEld mentioned this pull request Aug 14, 2018

Refactor benchmark to mitigate memory problems #194

Merged

Refactor benchmark example to reduce stack usage #116

Refactor benchmark example to reduce stack usage #116

Uh oh!

Conversation

RonEld commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek commented Oct 19, 2017

Uh oh!

Patater commented May 9, 2018

Uh oh!

RonEld commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonEld commented May 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek left a comment

Choose a reason for hiding this comment

Uh oh!

RonEld commented May 14, 2018

Uh oh!

andresag01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RonEld commented May 23, 2018

Uh oh!

andresag01 commented May 23, 2018

Uh oh!

k-stachowiak commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RonEld commented May 24, 2018

Uh oh!

RonEld commented Sep 19, 2017 •

edited

Loading

RonEld commented May 13, 2018 •

edited

Loading

RonEld Jul 26, 2018 •

edited

Loading

RonEld May 23, 2018 •

edited

Loading

k-stachowiak commented May 24, 2018 •

edited

Loading