[PATCH v2] rust: opp: simplify callers of `to_c_str_array`

[PATCH v2] rust: opp: simplify callers of `to_c_str_array`

[Tamir Duberstein]

Use `Option` combinators to make this a bit less noisy.

Wrap the `dev_pm_opp_set_config` operation in a closure and use type
ascription to leverage the compiler to check for use after free.

Signed-off-by: Tamir Duberstein <tamird@kernel.org>
---
Note: this diff is much smaller with whitespace suppressed (`-w`).

An alternative approach to compiler checking for UAF that doesn't change
indentation is to add `drop((self, clk_names, regulator_names))` after
`let ret = ...;` but that felt more prone to becoming out of date if
more owned data needed to be added to the function scope.
---
Changes in v2:
- Avoid use after free; add compiler checking. (Thanks Viresh!)
- Link to v1: https://patch.msgid.link/20251020-opp-simpler-code-v1-1-04f7f447712f@kernel.org
---
 rust/kernel/opp.rs | 112 +++++++++++++++++++++++++++--------------------------
 1 file changed, 58 insertions(+), 54 deletions(-)

diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index 04472a8de3ff..f9641c639fff 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -443,66 +443,70 @@ pub fn set_supported_hw(mut self, hw: KVec<u32>) -> Result<Self> {
     ///
     /// The returned [`ConfigToken`] will remove the configuration when dropped.
     pub fn set(self, dev: &Device) -> Result<ConfigToken> {
-        let (_clk_list, clk_names) = match &self.clk_names {
-            Some(x) => {
-                let list = to_c_str_array(x)?;
-                let ptr = list.as_ptr();
-                (Some(list), ptr)
-            }
-            None => (None, ptr::null()),
-        };
+        let clk_names = self.clk_names.as_deref().map(to_c_str_array).transpose()?;
+        let regulator_names = self
+            .regulator_names
+            .as_deref()
+            .map(to_c_str_array)
+            .transpose()?;
+
+        let set_config = || {
+            let clk_names = clk_names.as_ref().map_or(ptr::null(), |c| c.as_ptr());
+            let regulator_names = regulator_names.as_ref().map_or(ptr::null(), |c| c.as_ptr());
+
+            let prop_name = self
+                .prop_name
+                .as_ref()
+                .map_or(ptr::null(), |p| p.as_char_ptr());
+
+            let (supported_hw, supported_hw_count) = self
+                .supported_hw
+                .as_ref()
+                .map_or((ptr::null(), 0), |hw| (hw.as_ptr(), hw.len() as u32));
+
+            let (required_dev, required_dev_index) = self
+                .required_dev
+                .as_ref()
+                .map_or((ptr::null_mut(), 0), |(dev, idx)| (dev.as_raw(), *idx));
+
+            let mut config = bindings::dev_pm_opp_config {
+                clk_names,
+                config_clks: if T::HAS_CONFIG_CLKS {
+                    Some(Self::config_clks)
+                } else {
+                    None
+                },
+                prop_name,
+                regulator_names,
+                config_regulators: if T::HAS_CONFIG_REGULATORS {
+                    Some(Self::config_regulators)
+                } else {
+                    None
+                },
+                supported_hw,
+                supported_hw_count,
 
-        let (_regulator_list, regulator_names) = match &self.regulator_names {
-            Some(x) => {
-                let list = to_c_str_array(x)?;
-                let ptr = list.as_ptr();
-                (Some(list), ptr)
-            }
-            None => (None, ptr::null()),
-        };
+                required_dev,
+                required_dev_index,
+            };
 
-        let prop_name = self
-            .prop_name
-            .as_ref()
-            .map_or(ptr::null(), |p| p.as_char_ptr());
-
-        let (supported_hw, supported_hw_count) = self
-            .supported_hw
-            .as_ref()
-            .map_or((ptr::null(), 0), |hw| (hw.as_ptr(), hw.len() as u32));
-
-        let (required_dev, required_dev_index) = self
-            .required_dev
-            .as_ref()
-            .map_or((ptr::null_mut(), 0), |(dev, idx)| (dev.as_raw(), *idx));
-
-        let mut config = bindings::dev_pm_opp_config {
-            clk_names,
-            config_clks: if T::HAS_CONFIG_CLKS {
-                Some(Self::config_clks)
-            } else {
-                None
-            },
-            prop_name,
-            regulator_names,
-            config_regulators: if T::HAS_CONFIG_REGULATORS {
-                Some(Self::config_regulators)
-            } else {
-                None
-            },
-            supported_hw,
-            supported_hw_count,
+            // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
+            // requirements. The OPP core guarantees not to access fields of [`Config`] after this
+            // call and so we don't need to save a copy of them for future use.
+            let ret = unsafe { bindings::dev_pm_opp_set_config(dev.as_raw(), &mut config) };
 
-            required_dev,
-            required_dev_index,
+            to_result(ret).map(|()| ConfigToken(ret))
         };
 
-        // SAFETY: The requirements are satisfied by the existence of [`Device`] and its safety
-        // requirements. The OPP core guarantees not to access fields of [`Config`] after this call
-        // and so we don't need to save a copy of them for future use.
-        let ret = unsafe { bindings::dev_pm_opp_set_config(dev.as_raw(), &mut config) };
+        // Ensure the closure does not accidentally drop owned data; if violated, the compiler
+        // produces E0525 with e.g.:
+        //
+        // ```
+        // closure is `FnOnce` because it moves the variable `clk_names` out of its environment
+        // ```
+        let _: &dyn Fn() -> _ = &set_config;
 
-        to_result(ret).map(|()| ConfigToken(ret))
+        set_config()
     }
 
     /// Config's clk callback.

---
base-commit: 173e02d674946ff3ef8da7f44a9d5b820b9af21c
change-id: 20251018-opp-simpler-code-58662c627bf4

Best regards,
--  
Tamir Duberstein <tamird@kernel.org>

Re: [PATCH v2] rust: opp: simplify callers of `to_c_str_array`

[Viresh Kumar]

On 23-10-25, 09:59, Tamir Duberstein wrote:
> Use `Option` combinators to make this a bit less noisy.
> 
> Wrap the `dev_pm_opp_set_config` operation in a closure and use type
> ascription to leverage the compiler to check for use after free.
> 
> Signed-off-by: Tamir Duberstein <tamird@kernel.org>
> ---
> Note: this diff is much smaller with whitespace suppressed (`-w`).
> 
> An alternative approach to compiler checking for UAF that doesn't change
> indentation is to add `drop((self, clk_names, regulator_names))` after
> `let ret = ...;` but that felt more prone to becoming out of date if
> more owned data needed to be added to the function scope.
> ---
> Changes in v2:
> - Avoid use after free; add compiler checking. (Thanks Viresh!)
> - Link to v1: https://patch.msgid.link/20251020-opp-simpler-code-v1-1-04f7f447712f@kernel.org
> ---
>  rust/kernel/opp.rs | 112 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 58 insertions(+), 54 deletions(-)

Thanks, tested this and it works.

Lets see if someone has any more inputs, otherwise I can apply it.

-- 
viresh

Re: [PATCH v2] rust: opp: simplify callers of `to_c_str_array`

[Tamir Duberstein]

On Thu, Oct 23, 2025 at 11:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-10-25, 09:59, Tamir Duberstein wrote:
> > Use `Option` combinators to make this a bit less noisy.
> >
> > Wrap the `dev_pm_opp_set_config` operation in a closure and use type
> > ascription to leverage the compiler to check for use after free.
> >
> > Signed-off-by: Tamir Duberstein <tamird@kernel.org>
> > ---
> > Note: this diff is much smaller with whitespace suppressed (`-w`).
> >
> > An alternative approach to compiler checking for UAF that doesn't change
> > indentation is to add `drop((self, clk_names, regulator_names))` after
> > `let ret = ...;` but that felt more prone to becoming out of date if
> > more owned data needed to be added to the function scope.
> > ---
> > Changes in v2:
> > - Avoid use after free; add compiler checking. (Thanks Viresh!)
> > - Link to v1: https://patch.msgid.link/20251020-opp-simpler-code-v1-1-04f7f447712f@kernel.org
> > ---
> >  rust/kernel/opp.rs | 112 +++++++++++++++++++++++++++--------------------------
> >  1 file changed, 58 insertions(+), 54 deletions(-)
>
> Thanks, tested this and it works.
>
> Lets see if someone has any more inputs, otherwise I can apply it.

Thanks for testing! Doesn't look like anyone has opinions. Good to go?

Re: [PATCH v2] rust: opp: simplify callers of `to_c_str_array`

[Tamir Duberstein]

On Wed, Oct 29, 2025 at 10:24 AM Tamir Duberstein <tamird@kernel.org> wrote:
>
> On Thu, Oct 23, 2025 at 11:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 23-10-25, 09:59, Tamir Duberstein wrote:
> > > Use `Option` combinators to make this a bit less noisy.
> > >
> > > Wrap the `dev_pm_opp_set_config` operation in a closure and use type
> > > ascription to leverage the compiler to check for use after free.
> > >
> > > Signed-off-by: Tamir Duberstein <tamird@kernel.org>
> > > ---
> > > Note: this diff is much smaller with whitespace suppressed (`-w`).
> > >
> > > An alternative approach to compiler checking for UAF that doesn't change
> > > indentation is to add `drop((self, clk_names, regulator_names))` after
> > > `let ret = ...;` but that felt more prone to becoming out of date if
> > > more owned data needed to be added to the function scope.
> > > ---
> > > Changes in v2:
> > > - Avoid use after free; add compiler checking. (Thanks Viresh!)
> > > - Link to v1: https://patch.msgid.link/20251020-opp-simpler-code-v1-1-04f7f447712f@kernel.org
> > > ---
> > >  rust/kernel/opp.rs | 112 +++++++++++++++++++++++++++--------------------------
> > >  1 file changed, 58 insertions(+), 54 deletions(-)
> >
> > Thanks, tested this and it works.
> >
> > Lets see if someone has any more inputs, otherwise I can apply it.
>
> Thanks for testing! Doesn't look like anyone has opinions. Good to go?

Ah I see you've already applied it. Thanks!