Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move part of timing module out of the library and to test #4083

Closed
daverodgman opened this issue Jan 27, 2021 · 8 comments · Fixed by #4640
Closed

Move part of timing module out of the library and to test #4083

daverodgman opened this issue Jan 27, 2021 · 8 comments · Fixed by #4640
Assignees
Labels
enhancement size-s Estimated task size: small (~2d)

Comments

@daverodgman
Copy link
Contributor

Investigate options for removing the timing module.

Discussion on the list suggests that there are good arguments for moving at least parts of this out of the main library (although DTLS has uses for some of this, so we might want to keep some functionality in libmbedtls), but we still want the functionality in the example programs.

Mailing list discussion: https://lists.trustedfirmware.org/pipermail/mbed-tls/2020-April/000051.html

This is part of #4030

@daverodgman daverodgman added enhancement mbedtls-3 size-s Estimated task size: small (~2d) labels Jan 27, 2021
@mpg
Copy link
Contributor

mpg commented Feb 24, 2021

Moving this would probably involve splitting it, moving one bit to the SSL library (perhaps a new sub-module there), and for most of the rest, creating a new "Unix / Windows" platform layer, which would have to be designed and would probably require discussion before we even agree on what its goal should be.

I think this would require more design work that we can afford for 3.0, and since we agreed to limit 3.0 to simple changes, I'm inclined to post-pone that one to 4.0 and just keep the timing module as it is for 3.0.

@gilles-peskine-arm
Copy link
Contributor

As I wrote on the mailing list, I think that the fate of functions that the library needs (for DTLS) and functions that only test/sample programs need should be distinct considerations.

For the functions that library needs, it wouldn't be much work to move them to platform.c and rename them to mbedtls_platform_xxx. We could then later in 3.x add the same configurability mechanism that other mbedtls_platform_xxx have.

I still think that we should move the benchmark-only functions to tests/src (potentially later in 3.x to be moved to programs/src or some such, but that's a separate discussion).

@mpg
Copy link
Contributor

mpg commented Feb 24, 2021

I have no objections to doing what you suggest in 3.0 if it we can agree on a design quickly enough (in particular, which functions should go where, and associated options to replace MBEDTLS_TIMING_C).

@mpg
Copy link
Contributor

mpg commented Apr 22, 2021

In this comment on net_sockets you suggest that in the future (probably 4.0) we should re-design the platform module in order to include things that are currently in some other modules, including timing. I agree with that, and based on that "do nothing for 3.0, revisit in 4.0" has my vote here too.

@mpg mpg changed the title Investigate removing module: timing Move part of timing module out of the library and to test Apr 29, 2021
@mpg
Copy link
Contributor

mpg commented Apr 29, 2021

Team decision: keep what's needed for DTLS, move the rest (benchmarking etc) out of the library. Exact list TBD.

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Apr 29, 2021

  • mbedtls_timing_hardclock: was used for HAVEGE and mbedtls_hardclock_poll. HAVEGE has been removed. mbedtls_hardclock_poll is a weak source of entropy, which is not really useful since there is no guarantee that it has any entropy, so it should be removed. Remove.
  • mbedtls_timing_get_timer: I'm on the fence about keeping it in the API. For 3.0, keep it in the header but document as “may change without notice”?
    • used directly in udp_proxy, which is not meant to be portable beyond POSIX+Windows, so it can.
    • used as an internal function in timing.c.
  • mbedtls_set_alarm: only used in benchmark.c. Move there.
  • mbedtls_timing_set_delay, mbedtls_timing_get_delay: not used in the library, but necessary for DTLS. Keep.
  • mbedtls_timing_self_test: Pointless. Remove.

@TRodziewicz TRodziewicz self-assigned this Jun 9, 2021
@TRodziewicz
Copy link
Contributor

TRodziewicz commented Jun 9, 2021

@gilles-peskine-arm , seems that the mbedtls_timing_hardclock() is still used in the macro TIME_AND_TSC( TITLE, CODE ) in benchmark.c. I guess that the former should be moved to the latter? And if so I assume that it can be removed from the test_suite_timing.function? Similar question is with volatile global variable mbedtls_timing_alarmed (i.e. moving to the benchmark.c).

@gilles-peskine-arm
Copy link
Contributor

Yes, if something is only used in the benchmark program and not useful for DTLS, please move it to the benchmark program. If a function is no longer in the library then corresponding test code (in a test suite or in a self-test function) needs to be removed as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-s Estimated task size: small (~2d)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants