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: <5E354BB7-9CD5-4615-8EAF-98B9F498713A@collabora.com>
Date: Wed, 19 Feb 2025 20:35:47 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Mark Brown <broonie@...nel.org>
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

Hi Mark,

> On 19 Feb 2025, at 20:05, Mark Brown <broonie@...nel.org> wrote:
> 
> 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.

Maybe there was a language issue on my end here.

When I say once, I mean exactly what the source code shows, i.e.: it can be
enabled once, and further calls will be ignored. If it is disabled, it can then be
enabled once, with all further calls ignored, etc.

What I wanted to say is that this abstraction does not support exactly what you
suggested, i.e.: a refcount of how many times it was enabled.

I agree that this is not what the word “once” means. Sorry for the confusion.

> 
> 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.

I thought about that, something like:

```
  fn drop(&mut self) {

   // someone probably forgot to call disable?
    while self.enabled_count > 0 {

            // Try to disable, this will decrement self.enabled_count
            if let Err(e) = self.disable() {

              // But don’t loop forever if it fails
              break;
            }
        }
  }
```

I asked for a few opinions privately and was told that “if the C API prefers not to do that
why should you?”

> 
> 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?

As I said, this doesn’t work very well, unless someone corrects my reasoning on a
previous email. Having a single type “Regulator” is probably much saner than “Regulator”
and “EnabledRegulator”.

Your “refcount” idea works just fine, though. I’ll send a v2.

— Daniel