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

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

[Alice Ryhl]

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

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

[Daniel Almeida]

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

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

[Mark Brown]

On Tue, 24 Mar 2026 10:49:59 +0000, Alice Ryhl wrote:
> rust: regulator: do not assume that regulator_get() returns non-null

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-7.0

Thanks!

[1/1] rust: regulator: do not assume that regulator_get() returns non-null
      https://git.kernel.org/broonie/regulator/c/8121353a4bf8

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Miguel Ojeda]

On Tue, Mar 24, 2026 at 1:42 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Sorry, this got buried in all the email I get from the list.

No worries, I didn't know if you had seen the other thread -- thanks
for quickly reviewing it!

By the way, by chance, do you have any comments on the couple other
suggestions I mentioned in the linked thread?

  https://lore.kernel.org/rust-for-linux/20260322193830.89324-1-ojeda@kernel.org/

i.e. I was trying to understand what led to the issue and how we could
avoid it again.

Thanks!

Cheers,
Miguel

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

[Miguel Ojeda]

On Tue, Mar 24, 2026 at 11:50 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> 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.

Yeah, matching the transparency of the C API is what we should
generally do, so unless there is a reason not to do it here (and
Daniel already replied, so I guess not), this looks good.

Liam, Mark: I guess you will pick this up, but if not, please let me
know -- thanks!

Cheers,
Miguel