Re: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `blink_set_callback` creates aliased mutable references if `delay_on` and `delay_off` point to the same memory location.
- [High] The lifetime bound `'bound: 'init` on `DeviceBuilder::build` is inverted, allowing the Device to outlive its borrowed C strings.
- [Medium] The commit message description is not capitalized after the subsystem prefix.
- [Low] Small functions in abstractions must be annotated with `#[inline]`.
--

--- Patch [1]: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions ---
commit 0bee73d85d7c6052f28b0588d95b4632e4bdc662
Author: Markus Probst <[email protected]>

rust: leds: add basic led classdev abstractions

Implement the core abstractions needed for led class devices, including
the Device builder and LedOps trait.

> rust: leds: add basic led classdev abstractions

[Severity: Medium]
This isn't a bug, but the subsystem guidelines request capitalizing the 
description after the subsystem prefix. Should this be "rust: leds: Add 
basic led classdev abstractions" instead?

> diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> index 596975e103b8b.....
> --- /dev/null
> +++ b/rust/kernel/led.rs

[ ... ]

> +impl TryFrom<u32> for Color {
> +    type Error = Error;
> +
> +    fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {

[Severity: Low]
This isn't a bug, but the Rust subsystem guidelines specify that small 
functions in abstractions should be annotated with #[inline]. Could we add 
that attribute to this try_from() implementation?

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 816db7c576894.....
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs

[ ... ]

> +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
> +    /// Registers a new [`Device`].
> +    pub fn build<'bound: 'init, T: LedOps + 'bound>(
> +        self,
> +        parent: &'bound T::Bus,
> +        ops: impl PinInit<T, Error> + 'init,
> +    ) -> impl PinInit<Device<'bound, T>, Error> + 'init {

[Severity: High]
Is the lifetime bound 'bound: 'init inverted here?

DeviceBuilder captures string references with the 'init lifetime. If the 
returned Device has a 'bound lifetime that outlives 'init, a caller could 
create a Device that escapes the scope of the borrowed strings.

Since the underlying C led_classdev stores raw pointers to these strings, 
could this lead to a use-after-free when accessed via sysfs if the strings
are dropped?

[ ... ]

> +    unsafe extern "C" fn blink_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        delay_on: *mut usize,
> +        delay_off: *mut usize,
> +    ) -> i32 {

[ ... ]

> +            classdev.ops.blink_set(
> +                parent,
> +                classdev,
> +                // SAFETY: The function's contract guarantees that `delay_on` points to a `usize`
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_on },
> +                // SAFETY: The function's contract guarantees that `delay_off` points to a `usize`
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_off },
> +            )?;

[Severity: High]
Does this create aliased mutable references if the C subsystem passes the 
same pointer for delay_on and delay_off?

The C LED subsystem explicitly permits and frequently passes the exact same 
pointer for both arguments. For example, led_blink_set_oneshot() in 
ledtrig-netdev.c passes identical pointers.

If both pointers are identical, converting them concurrently into two 
&mut usize references triggers undefined behavior in Rust. Could we handle 
this aliasing safely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1