Improve CI performance on valgrind components #6665

daverodgman · 2022-11-25T16:26:16Z

Description

Improve CI performance for valgrind components. Ideally it would only run over the constant-flow tests, but there isn't a good existing mechanism for implementing this. For now, skip the particularly expensive tests.

Fixes #6661

Gatekeeper checklist

changelog not required (test only)
backport Faster valgrind - backport 2.28 #6677 (includes Tell cmake to get SKIP_TEST_SUITES from ENV #6676)
tests N/A (test only)

Add base64 to the name, in preparation for moving the test functions from the base64 test suite to the constant_time test suite. Rename the dec test function to be closer to the library function. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Create a test suite for constant-time functions defined in constant_time.c. This follows the general organization of the project where each module has its own test suite(s). This may also make it easier to arrange tests or analyses of constant-timeness. This commit creates the new test suite and moves the existing base64 tests there. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Test functions of constant_time.c in its own test suite, as we usually do. ssl_cf_hmac gets its own test suite because it runs a large number of iterations and takes a long time. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Conditional inclusion of "mbedtls/md.h" via "constant_time_internal.h" isn't enough, because the test framework includes code to resolve constant values regardless of whether the test case is compiled in. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

gilles-peskine-arm · 2022-11-25T16:30:46Z

tests/scripts/all.sh

+    # The following components are especially slow and don't have any constant-flow annotations,
+    # so disable them for better CI performance.
+    # It would be better to pass a list of tests that we want to run, rather than tests to skip,
+    # but this functionality is currently missing from run-test-suites.pl.


run-test-suites.pl is not involved here: this shell function is only called during builds that use cmake.

gilles-peskine-arm · 2022-11-25T16:31:27Z

tests/scripts/all.sh

    cmake -D CMAKE_BUILD_TYPE:String=Release .
    make

    # this only shows a summary of the results (how many of each type)
    # details are left in Testing/<date>/DynamicAnalysis.xml
    msg "test: main suites (full minus MBEDTLS_USE_PSA_CRYPTO, valgrind + constant flow)"
    make memcheck
+    skip_non_constant_flow_components 0


This line isn't useful: components run in a subshell, so environment variables aren't retained.

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

gilles-peskine-arm · 2022-11-25T19:52:56Z

CI's still not happy.

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

mpg · 2022-11-29T09:57:27Z

As mentioned on Slack, I'm not happy about moving things to test_suite_constant_time: the functions being tested do NOT belong to constant_time.c in the first place, they never did it was supposed to be temporary, and they should be moved back to the module where they belong.

I agree this PR is making resolves an inconsistency, by having functions tested in a test suite that matches the place they are defined. But I think it's resolving this inconsistency in the wrong way: by moving the tests, while it's the functions that should be moved.

I understand that the purpose of the PR is to isolate test that should be run in the constant_flow components, but I think that can be done by putting them in new files such as test_suite_ssl.constant_time.data, and then running only those test suites in the constant_flow components. I understand this is harder to do as we only have SKIP_TEST_SUITE and not FILTER_TEST_SUITE (which would allow running only test suites that have constant_time in their name), but IMO that would be the right way to solve the problem.

I won't formally object to the present approach, as solving problems with the CI is urgent so we need to get improvements in as fast as we can, but long-term I don't think things are being moved to the right place.

mpg · 2022-11-29T10:02:41Z

PS: I just had a look at 78e4c6d and I think it actually supports moving things to the right place: create test_suite_ssl_contant_time.function and test_suite_ssl_constant_time.data, and move the tests for constant-time legacy CBC there, and similarly for other modules.

Again, I think it's really really wrong for SSL-specific functions, RSA-specific functions, etc, to live in constant-time.c and the fact that this file currently needs to include ssl_misc.h and rsa.h are bad smells, so I'd really like to avoid moving further in that direction, if we can improve the CI issues without doing that.

davidhorstmann-arm

LGTM, thanks!

gilles-peskine-arm · 2022-11-29T10:06:16Z

the functions being tested do NOT belong to constant_time.c in the first place

I agree. That's why I abandoned #6273. I meant to replace it by moving the other way round, but I haven't gotten around to it yet.

daverodgman · 2022-11-29T11:33:46Z

I can extract ssl_cf_memcpy_offset to test_suite_ssl_constant_time.function; do we also want to move ssl_cf_hmac here and eliminate test_suite_constant_time_hmac.function?

gilles-peskine-arm · 2022-11-29T11:37:50Z

I'd like ssl_cf_hmac in its own test suite because it's extremely slow and I want to be able to skip it easily.

…function Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

davidhorstmann-arm

LGTM

gilles-peskine-arm · 2022-11-29T14:05:50Z

I can extract ssl_cf_memcpy_offset to test_suite_ssl_constant_time.function

Although mbedtls_ct_memcpy_offset is only used by the TLS code, its specification and implementation are completely unrelated to TLS. So I think it belongs in constant_time.c and test_suite_constant_time.function, not in ssl*. Contrast with mbedtls_ct_hmac which is designed very specifically to solve a problem that appears when implementing TLS.

daverodgman · 2022-11-29T14:43:51Z

I can extract ssl_cf_memcpy_offset to test_suite_ssl_constant_time.function

Although mbedtls_ct_memcpy_offset is only used by the TLS code, its specification and implementation are completely unrelated to TLS. So I think it belongs in constant_time.c and test_suite_constant_time.function, not in ssl*. Contrast with mbedtls_ct_hmac which is designed very specifically to solve a problem that appears when implementing TLS.

I agree, it does look like mbedtls_ct_memcpy_offset can be considered a generic constant time function.

I don't have a strong view about where the test should live, but I also don't think it is very critical. I would prefer to get this merged & help the CI reliability rather than spend time going back and forth on which test suite is best.

gilles-peskine-arm

I'd like a little more documentation, and to not move the base64 tests.

I think this would benefit from a clean history. I'll make an alternative PR.

gilles-peskine-arm · 2022-11-29T14:51:50Z

tests/scripts/all.sh

@@ -1608,6 +1608,15 @@ component_test_memsan_constant_flow () {
    make test
 }

+skip_non_constant_flow_components () {


This isn't about skipping components, it's about skipping suites.

gilles-peskine-arm · 2022-11-29T14:52:41Z

tests/suites/test_suite_base64.data

@@ -1,33 +1,3 @@
-mask_of_range empty (1..0)


Let's not move the base64 tests since we know we actually want them right where they are.

gilles-peskine-arm · 2022-11-29T14:53:35Z

tests/scripts/all.sh

@@ -1608,6 +1608,15 @@ component_test_memsan_constant_flow () {
    make test
 }

+skip_non_constant_flow_components () {
+    # Skip the test suites that don't have any constant-flow annotations.


Please add an explicit note that this won't catch TEST_CF_SECRET from tests/include or tests/src, if we were to add some. And, since that's not really discoverable, we should have a note in the documentation of TEST_CF_SECRET as well.

gilles-peskine-arm · 2022-11-29T15:05:54Z

tests/scripts/all.sh

@@ -1662,6 +1672,7 @@ component_test_valgrind_constant_flow_psa () {
    msg "build: cmake release GCC, full config with constant flow testing"
    scripts/config.py full
    scripts/config.py set MBEDTLS_TEST_CONSTANT_FLOW_VALGRIND
+    skip_non_constant_flow_components


Uh-oh, we don't have a test_valgrind_psa. So there's loss of coverage here. We should either keep this component as is or add a test_valgrind_psa.

Fixed in #6685

daverodgman · 2022-11-29T15:11:17Z

I'd like a little more documentation, and to not move the base64 tests.

I think this would benefit from a clean history. I'll make an alternative PR.

I think this is not a good use of time. Feel free to push additional commits to this one if you feel they are really needed, but I'm concerned we are spending too much time going back and forth on polishing minor details and late feedback here rather than taking the improvement.

gilles-peskine-arm · 2022-11-29T15:35:24Z

I'm making a new PR (almost ready) because it's easier than building on the history here.

gilles-peskine-arm and others added 6 commits November 25, 2022 16:23

Include declarations of dependency symbols used in test data

9440fa1

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Disable exensive non-constant-flow tests under valgrind

d8cfbf3

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman requested a review from gilles-peskine-arm November 25, 2022 16:28

daverodgman changed the title ~~Faster valgrind~~ Improve CI performance Nov 25, 2022

daverodgman changed the title ~~Improve CI performance~~ Improve CI performance on valgrind components Nov 25, 2022

gilles-peskine-arm reviewed Nov 25, 2022

View reviewed changes

daverodgman added 4 commits November 25, 2022 16:34

Remove unncessary cleanup

d1ee682

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

Fix include error

b92f8f5

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

Add some more components to the list of not-run

6ce49d4

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

Fix missing include

d126de3

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Nov 25, 2022

Fix syntax error

ac4b5ec

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

gilles-peskine-arm removed the needs-work label Nov 26, 2022

Update comment

7754e03

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman requested review from davidhorstmann-arm and gilles-peskine-arm November 28, 2022 12:14

daverodgman mentioned this pull request Nov 28, 2022

Faster valgrind - backport 2.28 #6677

Closed

3 tasks

davidhorstmann-arm previously approved these changes Nov 29, 2022

View reviewed changes

Move ssl_cf_memcpy_offset to a separate test_suite_ssl_constant_time.…

80de190

…function Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

daverodgman dismissed davidhorstmann-arm’s stale review via 80de190 November 29, 2022 11:44

daverodgman added the needs-ci Needs to pass CI tests label Nov 29, 2022

Remove erroneous file

cba0a78

Signed-off-by: Dave Rodgman <dave.rodgman@arm.com>

gilles-peskine-arm removed the needs-preceding-pr Requires another PR to be merged first label Nov 29, 2022

davidhorstmann-arm approved these changes Nov 29, 2022

View reviewed changes

gilles-peskine-arm requested changes Nov 29, 2022

View reviewed changes

gilles-peskine-arm reviewed Nov 29, 2022

View reviewed changes

gilles-peskine-arm mentioned this pull request Nov 29, 2022

Rationalize Valgrind tests #6685

Merged

3 tasks

daverodgman closed this Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CI performance on valgrind components #6665

Improve CI performance on valgrind components #6665

daverodgman commented Nov 25, 2022 •

edited

Loading

gilles-peskine-arm Nov 25, 2022

gilles-peskine-arm Nov 25, 2022

gilles-peskine-arm commented Nov 25, 2022

mpg commented Nov 29, 2022

mpg commented Nov 29, 2022

davidhorstmann-arm left a comment

gilles-peskine-arm commented Nov 29, 2022

daverodgman commented Nov 29, 2022

gilles-peskine-arm commented Nov 29, 2022

davidhorstmann-arm left a comment

gilles-peskine-arm commented Nov 29, 2022

daverodgman commented Nov 29, 2022

gilles-peskine-arm left a comment

gilles-peskine-arm Nov 29, 2022

gilles-peskine-arm Nov 29, 2022

gilles-peskine-arm Nov 29, 2022

gilles-peskine-arm Nov 29, 2022

gilles-peskine-arm Nov 29, 2022

daverodgman commented Nov 29, 2022

gilles-peskine-arm commented Nov 29, 2022

Improve CI performance on valgrind components #6665

Improve CI performance on valgrind components #6665

Conversation

daverodgman commented Nov 25, 2022 • edited Loading

Description

Gatekeeper checklist

gilles-peskine-arm Nov 25, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 25, 2022

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 25, 2022

mpg commented Nov 29, 2022

mpg commented Nov 29, 2022

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 29, 2022

daverodgman commented Nov 29, 2022

gilles-peskine-arm commented Nov 29, 2022

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Nov 29, 2022

daverodgman commented Nov 29, 2022

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Nov 29, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 29, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 29, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 29, 2022

Choose a reason for hiding this comment

gilles-peskine-arm Nov 29, 2022

Choose a reason for hiding this comment

daverodgman commented Nov 29, 2022

gilles-peskine-arm commented Nov 29, 2022

daverodgman commented Nov 25, 2022 •

edited

Loading