Re: [PATCH] rust: regulator: do not assume that regulator_get() returns non-null

[Daniel Almeida <daniel.almeida@collabora.com>]

> On 24 Mar 2026, at 07:49, Alice Ryhl <aliceryhl@google.com> wrote:
> 
> The Rust `Regulator` abstraction uses `NonNull` to wrap the underlying
> `struct regulator` pointer. When `CONFIG_REGULATOR` is disabled, the C
> stub for `regulator_get` returns `NULL`. `from_err_ptr` does not treat
> `NULL` as an error, so it was passed to `NonNull::new_unchecked`,
> causing undefined behavior.
> 
> Fix this by using a raw pointer `*mut bindings::regulator` instead of
> `NonNull`. This allows `inner` to be `NULL` when `CONFIG_REGULATOR` is
> disabled, and leverages the C stubs which are designed to handle `NULL`
> or are no-ops.
> 
> Fixes: 9b614ceada7c ("rust: regulator: add a bare minimum regulator abstraction")
> Reported-by: Miguel Ojeda <ojeda@kernel.org>
> Closes: https://lore.kernel.org/r/20260322193830.89324-1-ojeda@kernel.org

Sorry, this got buried in all the email I get from the list.

Thanks for picking it up, Alice.

> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/regulator.rs | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
> index 4f7837c7e53a..41e730cedc81 100644
> --- a/rust/kernel/regulator.rs
> +++ b/rust/kernel/regulator.rs
> @@ -23,7 +23,10 @@
>     prelude::*,
> };
> 
> -use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull};
> +use core::{
> +    marker::PhantomData,
> +    mem::ManuallyDrop, //
> +};
> 
> mod private {
>     pub trait Sealed {}
> @@ -229,15 +232,17 @@ pub fn devm_enable_optional(dev: &Device<Bound>, name: &CStr) -> Result {
> ///
> /// # Invariants
> ///
> -/// - `inner` is a non-null wrapper over a pointer to a `struct
> -///   regulator` obtained from [`regulator_get()`].
> +/// - `inner` is a pointer obtained from a successful call to
> +///   [`regulator_get()`]. It is treated as an opaque token that may only be
> +///   accessed using C API methods (e.g., it may be `NULL` if the C API returns
> +///   `NULL`).
> ///
> /// [`regulator_get()`]: https://docs.kernel.org/driver-api/regulator.html#c.regulator_get
> pub struct Regulator<State>
> where
>     State: RegulatorState,
> {
> -    inner: NonNull<bindings::regulator>,
> +    inner: *mut bindings::regulator,
>     _phantom: PhantomData<State>,
> }
> 
> @@ -249,7 +254,7 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
>         // SAFETY: Safe as per the type invariants of `Regulator`.
>         to_result(unsafe {
>             bindings::regulator_set_voltage(
> -                self.inner.as_ptr(),
> +                self.inner,
>                 min_voltage.as_microvolts(),
>                 max_voltage.as_microvolts(),
>             )
> @@ -259,7 +264,7 @@ pub fn set_voltage(&self, min_voltage: Voltage, max_voltage: Voltage) -> Result
>     /// Gets the current voltage of the regulator.
>     pub fn get_voltage(&self) -> Result<Voltage> {
>         // SAFETY: Safe as per the type invariants of `Regulator`.
> -        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
> +        let voltage = unsafe { bindings::regulator_get_voltage(self.inner) };
> 
>         to_result(voltage).map(|()| Voltage::from_microvolts(voltage))
>     }
> @@ -270,10 +275,8 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
>             // received from the C code.
>             from_err_ptr(unsafe { bindings::regulator_get(dev.as_raw(), name.as_char_ptr()) })?;
> 
> -        // SAFETY: We can safely trust `inner` to be a pointer to a valid
> -        // regulator if `ERR_PTR` was not returned.
> -        let inner = unsafe { NonNull::new_unchecked(inner) };
> -
> +        // INVARIANT: `inner` is a pointer obtained from `regulator_get()`, and
> +        // the call was successful.
>         Ok(Self {
>             inner,
>             _phantom: PhantomData,
> @@ -282,12 +285,12 @@ fn get_internal(dev: &Device, name: &CStr) -> Result<Regulator<T>> {
> 
>     fn enable_internal(&self) -> Result {
>         // SAFETY: Safe as per the type invariants of `Regulator`.
> -        to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) })
> +        to_result(unsafe { bindings::regulator_enable(self.inner) })
>     }
> 
>     fn disable_internal(&self) -> Result {
>         // SAFETY: Safe as per the type invariants of `Regulator`.
> -        to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) })
> +        to_result(unsafe { bindings::regulator_disable(self.inner) })
>     }
> }
> 
> @@ -349,7 +352,7 @@ impl<T: IsEnabled> Regulator<T> {
>     /// Checks if the regulator is enabled.
>     pub fn is_enabled(&self) -> bool {
>         // SAFETY: Safe as per the type invariants of `Regulator`.
> -        unsafe { bindings::regulator_is_enabled(self.inner.as_ptr()) != 0 }
> +        unsafe { bindings::regulator_is_enabled(self.inner) != 0 }
>     }
> }
> 
> @@ -359,11 +362,11 @@ fn drop(&mut self) {
>             // SAFETY: By the type invariants, we know that `self` owns a
>             // reference on the enabled refcount, so it is safe to relinquish it
>             // now.
> -            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
> +            unsafe { bindings::regulator_disable(self.inner) };
>         }
>         // SAFETY: By the type invariants, we know that `self` owns a reference,
>         // so it is safe to relinquish it now.
> -        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
> +        unsafe { bindings::regulator_put(self.inner) };
>     }
> }
> 
> 
> ---
> base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
> change-id: 20260324-regulator-fix-167227abcc92
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>