linux-kernel - Re: [PATCH] rust: regulator: add a bare minimum regulator abstraction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7ZjzJ2Rfrt8j0s1@finisterre.sirena.org.uk>
Date: Wed, 19 Feb 2025 23:05:48 +0000
From: Mark Brown <broonie@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: lgirdwood@...il.com, sebastian.reichel@...labora.com,
	sjoerd.simons@...labora.co.uk, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, a.hindborg@...nel.org,
	benno.lossin@...ton.me, aliceryhl@...gle.com, tmgross@...ch.edu,
	dakr@...nel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: regulator: add a bare minimum regulator abstraction

On Wed, Feb 19, 2025 at 01:25:16PM -0300, Daniel Almeida wrote:

> Add a bare minimum regulator abstraction to be used by Rust drivers.
> This abstraction adds a small subset of the regulator API, which is
> thought to be sufficient for the drivers we have now.

...

> Note that each instance of [`Regulator`] obtained from
> `Regulator::get()` can only be enabled once. This ensures that the calls
> to enable and disable are perfectly balanced before `regulator_put()` is
> called, as mandated by the C API.

This sounds very wrong, a major use case for consumers is to enable and
disable regulators at runtime.  I would expect that the Rust code might
want to have it's own reference counting (and probably should), but not
being able to disable a regulator after enabling it seems to be a great
big red flag.

> +    /// Enable the regulator.
> +    pub fn enable(&mut self) -> Result {
> +        if self.enabled {
> +            return Ok(());
> +        }

This isn't what the changelog claimed, it's silently ignoring duplicate
attempts to enable and...

> +        // SAFETY: Safe as per the type invariants of `Regulator`.
> +        let res = to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) });
> +        if res.is_ok() {
> +            self.enabled = false;
> +        }

...we actually support disabling and reenabling multiple times, it's
just that there's a maximum of one enable at once from the consumer and
we silently ignore duplicated enables and disables.  That's better but
it's asking for trouble.

If you're going to support both enable and disable (and all things
considered you probably should TBH) it would be better to just reference
count the enables like the C code does and then complain about it if
someone tries to free the regulator object without underwinding their
enables, or just possibly just clean them up.  Silently ignoring
duplicate enables or overflowing disables doesn't seem like it plays
well with the safety goals that Rust has, it's asking for bugs - a big
part of the reason the C API does things the way it does is that it will
notice if the enables and disables aren't joined up.  You can easily
wind up with one part of the code disabling the regulator underneath
another part that's still trying to use the hardware and things tend not
to go well when that happens.

Perhaps an enable should be an object that's allocated and carried about
by whatever's holding the reference, I don't know if that plays nicely
with how Rust likes to ensure safety with this stuff?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)