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

embedded source compatibility #4

Closed
dhardy opened this issue Jan 19, 2019 · 18 comments
Closed

embedded source compatibility #4

dhardy opened this issue Jan 19, 2019 · 18 comments
Milestone

Comments

@dhardy
Copy link
Member

dhardy commented Jan 19, 2019

This crate requires std::io code on many platforms to read from special devices, therefore will normally require std. Ideally we should provide a smooth route to no_std compatibility however, possibly by allowing users to define extern functions. (We've discussed using "lang items" but these are restricted to Rust's core libraries; we may or may not integrate this code into std in the future and use that option, but ideally should have a no_std option now.)

@dhardy
Copy link
Member Author

dhardy commented Feb 28, 2019

One solution here would be a special feature enabling an alternative mode:

#[cfg(feature = "getrandom_custom")]
extern "C" {
    fn getrandom_custom(dest: *mut u8, len: usize) -> u32;
}

pub fn getrandom(dest: &mut [u8]) -> Result<(), Error> {
    #[cfg(feature = "getrandom_custom")] {
        use core::num::NonZeroU32;
        let ret = unsafe { getrandom_custom(dest.as_mut_ptr(), dest.len()) };
        return if let Some(err) = NonZeroU32::new(ret) {
            Err(Error::from(err))
        } else {
            Ok(())
        };
    }
    getrandom_inner(dest)
}

This would allow any single crate in the build to replace the entropy source, which has both advantages (easier testing with dummy entropy, easy of usage of other randomness source libs) and disadvantages (relatively easy to accidentally or possibly even maliciously mess this up; no single place to check whether the alternative is enabled).


Another solution (which doesn't need any support here) would be to make a replacement crate with the same API (copy error types, provide fn getrandom), then use a patch section in the top Cargo.toml to replace this.

[patch.crates-io]
getrandom = { path = "my/replacement/getrandom" }

@dhardy
Copy link
Member Author

dhardy commented Feb 28, 2019

There is another issue with no_std support: the error type should implement std::error::Error iff using std. Currently the lib does so on all platforms except SGX, i.e. this is automatic and requires std on all platforms which should support std. IIRC we did once have a reason to disable support on Mac OS.

In contrast, rand_core makes std-support a crate feature, and causes confusing errors if you get this wrong in the build.

@dhardy
Copy link
Member Author

dhardy commented Jun 3, 2019

Note: there is a long discussion on this topic in rand#579 with several sub-topics:

At any rate, development appears to be stalled due to lack of demand.

@tarcieri
Copy link

tarcieri commented Jun 4, 2019

@dhardy I opened an issue on the embedded WG to call attention to the issue of no_std support (and pinged some current embedded rand_core users) rust-embedded/wg#353

@josephlr
Copy link
Member

@dhardy now that #58 is merged, do we still want this to stay open? Should it be renamed to focus on just providing user-defined custom implementations?

@dhardy
Copy link
Member Author

dhardy commented Aug 13, 2019

Some of the ideas aired above may still be relevant in the future, e.g. use of an entropy pool (via another crate). This issue is too broad I guess, ranging from std dependencies to how to get entropy on embedded platforms. Perhaps I should rename it to focus on that?

@mati865

This comment has been minimized.

@newpavlov
Copy link
Member

newpavlov commented Aug 17, 2019

@mati865
This module is feature gated behind the std feature. You forgot to use default-features = false for rand (it enables std feature by default), after that you can enable getrandom feature manually. But note that you will not have ThreadRng with such config, so you will have to cascade std feature to rand and switch between ThreadRng and OsRng depending on std.

@mati865
Copy link

mati865 commented Aug 18, 2019

@newpavlov I totally missed that gate and couldn't find default features field for this crate. I haven't thought about other rand crates enabling it.
Thank you for the explanation and sorry for the noise.

@josephlr josephlr added this to the 0.2 milestone Jan 9, 2020
@josephlr
Copy link
Member

Do we think that Custom RNGs (#109) and the cpu feature (#133) are sufficient to close this issue?

@tarcieri
Copy link

@josephlr I think both of those PRs are irrelevant to the embedded use case, as none of them cover embedded targets.

Per the discussion on rust-embedded/wg#353 I’m wondering of the getrandom API is fundamentally incompatible with embedded-hal style TRNG peripherals, since getrandom generates randomness as a side effect, and accessing a TRNG will typically involve passing in an owned peripheral device.

I think the existing RngCore trait probably provides the most embedded-friendly API.

@josephlr
Copy link
Member

#109 (in addition to dealing with WASM targets) also adds a generic way for an external crate to register a custom getrandom implementation. This concept of an externally defined hook was specifically intended to work with embedded targets.

For example, if there was some external device that you want to get randomness from, you would write a custom crate which would depend on getrandom like:

name = "mydevice-getrandom"
[dependencies]
getrandom = { version = "0.2", features = ["custom"] }

And your code would then specify a function to be called. A sample lib.rs would look like:

use spin::Mutex;
use getrandom::{register_custom_getrandom, Error};

struct Device{ /* device specific state */ }

impl Device {
  fn new() -> Result<Self, Error> {
    /* Initialize and Create the device */
  }
  fn read(&mut self, buf: &mut [u8]) -> Result<(), Error> {
    /* Read randomness from the device */
  }
}

// Global instance of device
static DEVICE: Mutex<Option<Device>> = Mutex::new(None);

fn my_getrandom(buf: &mut[u8]) -> Result<(), Error> {
  let mut guard = DEVICE.lock();
  let dev: &mut Device = guard.get_or_insert(Device::new()?);
  dev.read(buf)
}

register_custom_getrandom!(my_getrandom);

The above code uses the no_std-compatible spin::Mutex, but you could also use lock_api::Mutex if you wanted to avoid spin locks and use platform specific synchronization primitives.

@tarcieri, Is there a reason that wouldn't work for your use case?

@josephlr
Copy link
Member

josephlr commented Feb 21, 2020

While the embedded-hal crate specifically seems to encourage Global Singletons (per their example code), I realize that this does not map onto all workflows.

Alternatively, if you had some device-specific mydevice crate which exported an api like:

struct RNGDevice{/* private */}

impl RNGDevice {
  fn get_mut() -> Result<&mut Self, Error>;
  fn read(&mut self, buf: &mut [u8]) -> Result<(), Error>;
}

You could implement a mydevice-getrandom crate like:

use mydevice::RNGDevice;
use getrandom::{register_custom_getrandom, Error};

fn my_getrandom(buf: &mut[u8]) -> Result<(), Error> {
  RNGDevice::get_mut()?.read(buf)
}

register_custom_getrandom!(my_getrandom);

@tarcieri
Copy link

tarcieri commented Feb 21, 2020

Well first, the spin crate is unmaintained... that aside...

I suppose this accomplishes the goal of having something that potentially works for embedded, however I think it’s complex solution as opposed to passing a &mut MyTrng where MyTrng: RngCore.

I’d probably still choose to use RngCore over getrandom for anything intended for embedded use.

One nit here:

 &mut Device = guard.get_or_insert(Device::new()?);

This is assuming device initialization as a side effect, which isn’t how the embedded-hal impls I’ve used work. Instead you receive all of the device’s “pins” (one of which you’d use to talk to a TRNG) passed in as an owned value.

To make that work with what you’ve proposed, you’d need a more once_cell-like API for registering an existing owned TRNG device.

@josephlr
Copy link
Member

I’d probably still choose to use RngCore over getrandom for anything intended for embedded use.

I'd agree that's the best bet if you're using something directly. The main reason for the custom impl is to handle cases where getrandom is a transitive dependency, because in that case you might not be able to pass around state.

@dhardy
Copy link
Member Author

dhardy commented Mar 10, 2020

Appologies for the late reply. @josephlr's example above can certainly be adapted to keep the global singleton but perform initialisation via a user-called function — it just means that getrandom must block or fail if called before this happens.

However, granted, this is a lot more complexity than strictly required. getrandom was intended for initialising ThreadRng and similar constructs which just work — but attempting to make this compatible with non-std targets may simply be misguided (since the latter also requires thread-local memory or global memory with a lock, and the whole construct carries a lot more complexity than is desirable on embedded).

Should we then close this issue as not-worth-delivering?

@tarcieri
Copy link

#109 seems good enough to address the immediate issue

@dhardy
Copy link
Member Author

dhardy commented Apr 16, 2020

Guess I'll close this then.

@dhardy dhardy closed this as completed Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants