mirage / mirage-crypto

Cryptographic primitives for OCaml, in OCaml (also used in MirageOS)
ISC License
77 stars 43 forks source link

Use an atomic instead of a reference to be domain-safe #221

Closed dinosaure closed 8 months ago

hannesm commented 8 months ago

CI is failing, is this expected?

dinosaure commented 8 months ago

CI is failing, is this expected?

Yes, the CI fails only for OCaml < 4.13 (backoff requires OCaml 4.13 at least) but it works fine for OCaml >= 4.14.

hannesm commented 8 months ago

CI is failing, is this expected?

Yes, the CI fails only for OCaml < 4.13 (backoff requires OCaml 4.13 at least) but it works fine for OCaml >= 4.14.

Hmm, ok. I'd prefer to keep OCaml 4.09 support. Any chance to get that?

reynir commented 8 months ago

Is it maybe worth it to not use backoff? I don't imagine there will be much contention so I think it should be perfectly fine without.

dinosaure commented 8 months ago

Hmm, ok. I'd prefer to keep OCaml 4.09 support. Any chance to get that?

Is it maybe worth it to not use backoff?

Atomic are introduced only since 4.12. backoff requires at least 4.13. So, if we would like to solve our initial issue, we have different possibilities:

hannesm commented 8 months ago

ok, so in the end I personally don't care much about OCaml < 4.14. Should we just put a lower bound? Or is there anyone hesitant to get their applications to work with older OCaml versions (/cc @mirage/core)

hannesm commented 8 months ago

I've not much clue -- but from what I can tell backoff requires on OCaml 4 the threads -- and I wonder whether this is available on MirageOS. Any hints welcome. I also don't know anything about the Atomic stuff, does this require threads or something on OCaml 4?

palainp commented 8 months ago

I recall pthread* being introduced in the pending ocaml-solo5 PR, so I guess threads are currently not available with solo5 targets :(

hannesm commented 8 months ago

so I took the initiative to not use backoff (unclear about the threads and MirageOS) -- instead use Atomic and compare_and_set in a tight loop.

Feels a bit strange since this may now not terminate, but given the amount of entropy sources that register themselves (at Rng.initialization time), I think it is safe (and/or once we observe infinite loop, we can reconsider).

/cc @dinosaure @reynir for review

reynir commented 8 months ago

It will always terminate since (at least) one domain will succeed in updating with compare_and_set* (think: if all domains fail every compare_and_set call then the value must have changed since they last read it => we have liveness). It can be beneficial to use backoff if there is a lot of contention to ease the CPU cache lines. Since we're probably going to add about a handful entropy sources it's IMO not worth it to add backoff.