linux-kernel - Re: [PATCH v5 1/2] 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] [day] [month] [year] [list]

Message-Id: <DAV0H83BZO0K.376YKJOOGL48H@kernel.org>
Date: Tue, 24 Jun 2025 21:30:42 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Boris Brezillon"
 <boris.brezillon@...labora.com>, "Sebastian Reichel"
 <sebastian.reichel@...labora.com>, "Liam Girdwood" <lgirdwood@...il.com>,
 "Mark Brown" <broonie@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v5 1/2] rust: regulator: add a bare minimum regulator
 abstraction

On Tue Jun 24, 2025 at 7:31 PM CEST, Daniel Almeida wrote:
> Hi,
>
>> +    /// Attempts to convert the regulator to an enabled state.
>> +    pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<Disabled>> {
>> +        self.enable_internal()
>> +            .map(|()| Regulator {
>> +                inner: self.inner,
>> +                _phantom: PhantomData,
>> +            })
>> +            .map_err(|error| Error {
>> +                error,
>> +                regulator: self,
>> +            })
>> +    }
>> +}
>> +
>> +impl Regulator<Enabled> {
>> +    /// Obtains a [`Regulator`] instance from the system and enables it.
>> +    ///
>> +    /// This is equivalent to calling `regulator_get_enable()` in the C API.
>> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
>> +        Regulator::<Disabled>::get_internal(dev, name)?
>> +            .try_into_enabled()
>> +            .map_err(|error| error.error)
>> +    }
>
> I just realized that this is a bug.
>
> The pre-typestate code was using ManuallyDrop<T> here, and it was forgotten on
> the newer versions. This means that the destructor for self runs here, which
> decreases the refcount by calling regulator_put().

Yeah that is correct.

> My proposed solution is:
>
>     /// Attempts to convert the regulator to an enabled state.
>     pub fn try_into_enabled(mut self) -> Result<Regulator<Enabled>, Error<Disabled>> {
>         // We will be transferring the ownership of our regulator_get() count to Regulator<Enabled>
>         let mut regulator = ManuallyDrop::new(self);
>
>         regulator.enable_internal()
>             .map(|()| Regulator {
>                 inner: regulator.inner,

This will only work if the type of `inner` implements `Copy`. I forgot
if it does.

>                 _phantom: PhantomData,
>             })
>             .map_err(|error| Error {
>                 error,
>                 regulator: ManuallyDrop::into_inner(regulator),
>             })
>     }
> }
>
>
>
> Alex, Benno, thoughts?

Looks correct.

---
Cheers,
Benno