MPI: Add stack-local limb storage to MPI objects. #306

Closed

areading wants to merge 1 commit from ar/mpi_limbstorage into development

areading commented

2019-10-29 05:53:40 -05:00

(Migrated from github.com)

This is particularly useful for elliptic curve operations: a 10%
reduction to execution time was observed for operations against the
secp256{r,k}1 curves with a minor (temporary) increase in the
required amount of stack space. To maintain the current behavior of
always using heap allocation regardless of MPI size, the library can
be compiled with MBEDTLS_MPI_LOCAL_LIMB_SIZE defined to 0.

I have tested this change with the existing test suite. I believe existing
tests adequately cover this change and that no new tests would be
required.

I've made this change for a performance-sensitive project that I'm
currently working on, and am submitting this pull request in hopes the
change would be useful for mbedtls/mbed-crypto upstream. My mbed
username is also 'areading', and I accept the Contributor License Agreement.

If this commit is in line with mbedTLS project goals and is a worthwhile
candidate for inclusion into the upstream development, I would be happy to
backport this into versions 2.16 or 2.7 as well, if deemed appropriate.

This allows for a small chunk of memory to be added to each mbedtls_mpi structure instantiation, the amount of which is adjustable via a new compile-time parameter MBEDTLS_MPI_LOCAL_LIMB_SIZE. If a given MPI can fit wholly within this chunk of memory (which will typically reside on the stack), no heap allocation calls for that MPI will be made. Should, however, an MPI require more space or grow beyond the extents provided by that chunk, then an allocation call will be made as usual and limb storage will be diverted appropriately. The default size of stack-local storage to be reserved is 32 bytes (i.e., 256 bits). This is particularly useful for elliptic curve operations: a 10% reduction to execution time was observed for operations against the secp256{r,k}1 curves with a minor (temporary) increase in the required amount of stack space. To maintain the current behavior of always using heap allocation regardless of MPI size, the library can be compiled with MBEDTLS_MPI_LOCAL_LIMB_SIZE defined to 0. I have tested this change with the existing test suite. I believe existing tests adequately cover this change and that no new tests would be required. I've made this change for a performance-sensitive project that I'm currently working on, and am submitting this pull request in hopes the change would be useful for mbedtls/mbed-crypto upstream. My mbed username is also 'areading', and I accept the Contributor License Agreement. If this commit is in line with mbedTLS project goals and is a worthwhile candidate for inclusion into the upstream development, I would be happy to backport this into versions 2.16 or 2.7 as well, if deemed appropriate.

❤️ 1

areading commented

2019-10-29 06:45:51 -05:00

(Migrated from github.com)

Updated query_config.c per CI failure in original commit.

gilles-peskine-arm commented

2019-10-29 11:34:03 -05:00

(Migrated from github.com)

Thank you for your contribution! We've indeed identified the heavy malloc use in bignum as a source of inefficiency in asymmetric cryptography, so improvements there are welcome.

Unfortunately, we cannot accept this code as is, and I'm not sure about the general approach.

Blockers:

We have stringent rules about backwards compatibility. For the time being, the API of bignum.h must not change under the default configuration. The representation of mbedtls_mpi is in a public header and is not explicitly marked as something that may change in a future version. For example an application is entitled to write mbedtls_mpi X = {1, 0, 0} or even mbedtls_mpi X = {1, 3, malloc(3*sizeof(mbedtls_mpi_uint))}. Therefore MBEDTLS_MPI_LOCAL_LIMB_SIZE must default to 0, and all new fields in mbedtls_mpi must be disabled when MBEDTLS_MPI_LOCAL_LIMB_SIZE is 0.
We need to run tests both with MBEDTLS_MPI_LOCAL_LIMB_SIZE equal to 0 and with MBEDTLS_MPI_LOCAL_LIMB_SIZE nonzero. This means at a minimum adding a component to all.sh with a nonzero value for MBEDTLS_MPI_LOCAL_LIMB_SIZE.

Regarding the design, turning on MBEDTLS_MPI_LOCAL_LIMB_SIZE means an increase in stack size. This increase is minor by the standards of a typical hosted platform, but we also target microcontrollers where a kilobyte of stack is huge. For example, mbedtls_mpi_exp_mod has 7 to 131 mbedtls_mpi structures on its stack (depending on the value of the memory/performance tuning parameter MBEDTLS_MPI_WINDOW_SIZE, so adding 32 bytes there means an increase of 224B to ~4kB in stack usage. So even if we were willing to break backward compatibility, we'd still want to turn the feature off on constrained platforms.

In the long term, we'd like to completely rewrite the bignum module to never use malloc. This means rewriting the cryptography code as well, so it's a huge task, and I don't know when we'll be able to do that if ever.

In the meantime, we are definitely interested in incremental improvements like yours. But we have to balance those with the backward compatibility requirements and the testing burden, so I can't promise that we'll add this feature even if it defaults off. Please give us some time to evaluate the design and discuss it internally.

Thank you for your contribution! We've indeed identified the heavy malloc use in bignum as a source of inefficiency in asymmetric cryptography, so improvements there are welcome. Unfortunately, we cannot accept this code as is, and I'm not sure about the general approach. Blockers: * We have stringent rules about backwards compatibility. For the time being, the API of `bignum.h` must not change under the default configuration. The representation of `mbedtls_mpi` is in a public header and is not explicitly marked as something that may change in a future version. For example an application is entitled to write `mbedtls_mpi X = {1, 0, 0}` or even `mbedtls_mpi X = {1, 3, malloc(3*sizeof(mbedtls_mpi_uint))}`. Therefore `MBEDTLS_MPI_LOCAL_LIMB_SIZE` must default to 0, and all new fields in `mbedtls_mpi` must be disabled when `MBEDTLS_MPI_LOCAL_LIMB_SIZE` is 0. * We need to run tests both with `MBEDTLS_MPI_LOCAL_LIMB_SIZE` equal to 0 and with `MBEDTLS_MPI_LOCAL_LIMB_SIZE` nonzero. This means at a minimum adding a component to `all.sh` with a nonzero value for `MBEDTLS_MPI_LOCAL_LIMB_SIZE`. Regarding the design, turning on `MBEDTLS_MPI_LOCAL_LIMB_SIZE` means an increase in stack size. This increase is minor by the standards of a typical hosted platform, but we also target microcontrollers where a kilobyte of stack is huge. For example, `mbedtls_mpi_exp_mod` has 7 to 131 `mbedtls_mpi` structures on its stack (depending on the value of the memory/performance tuning parameter `MBEDTLS_MPI_WINDOW_SIZE`, so adding 32 bytes there means an increase of 224B to ~4kB in stack usage. So even if we were willing to break backward compatibility, we'd still want to turn the feature off on constrained platforms. In the long term, we'd like to completely rewrite the bignum module to never use malloc. This means rewriting the cryptography code as well, so it's a huge task, and I don't know when we'll be able to do that if ever. In the meantime, we are definitely interested in incremental improvements like yours. But we have to balance those with the backward compatibility requirements and the testing burden, so I can't promise that we'll add this feature even if it defaults off. Please give us some time to evaluate the design and discuss it internally.

mpg commented

2020-04-01 05:08:38 -05:00

(Migrated from github.com)

Hi @areading! We've recently merged back mbed-crypto to the mbedtls repo, as announced here. As a result, we are no longer accepting PRs to the mbed-crypto repo (except a short grace period for recently active PR that can be finalized quickly).

Since this PR hasn't been active very recently and isn't ready to be merged as it is, I'm closing it and encouraging you to re-submit your contribution as a new PR on the mbedtls repository: https://github.com/ARMmbed/mbedtls/pulls

If you do so, you might want to take this occasion to review our new contribution licensing rules (no more CLA, but we now require a Signed-of-by tag in each commit) and update your commit messages accordingly.

Thanks again for your contribution, and I hope to see you soon with a new PR in the mbedtls repo!

Hi @areading! We've recently merged back mbed-crypto to the mbedtls repo, as [announced here](https://github.com/ARMmbed/mbedtls/issues/3064). As a result, we are no longer accepting PRs to the mbed-crypto repo (except a short grace period for recently active PR that can be finalized quickly). Since this PR hasn't been active very recently and isn't ready to be merged as it is, I'm closing it and encouraging you to re-submit your contribution as a new PR on the mbedtls repository: https://github.com/ARMmbed/mbedtls/pulls If you do so, you might want to take this occasion to review our new contribution licensing rules ([no more CLA, but we now require a Signed-of-by tag in each commit](https://github.com/ARMmbed/mbedtls/issues/3054)) and update your commit messages accordingly. Thanks again for your contribution, and I hope to see you soon with a new PR in the mbedtls repo!