linux-kernel - Re: [RFC PATCH 1/2] rust: LED 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: <CAH5fLgivWoo=FpKAhTsHPOot7ptWvezrgsB8YoHKsRobXok4MA@mail.gmail.com>
Date: Wed, 27 Nov 2024 12:39:49 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Fiona Behrens <me@...enk.dev>
Cc: Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, linux-leds@...r.kernel.org, 
	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>, 
	Trevor Gross <tmgross@...ch.edu>, FUJITA Tomonori <fujita.tomonori@...il.com>, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] rust: LED abstraction

On Thu, Nov 21, 2024 at 10:47 AM Fiona Behrens <me@...enk.dev> wrote:
>
> On 18 Nov 2024, at 11:22, Alice Ryhl wrote:
>
> > On Wed, Oct 9, 2024 at 12:58 PM Fiona Behrens <me@...enk.dev> wrote:
> >> +impl<'a, T> Led<T>
> >> +where
> >> +    T: Operations + 'a,
> >> +{
> >> +    /// Register a new LED with a predefine name.
> >> +    pub fn register_with_name(
> >> +        name: &'a CStr,
> >> +        device: Option<&'a Device>,
> >> +        config: &'a LedConfig,
> >> +        data: T,
> >> +    ) -> impl PinInit<Self, Error> + 'a {
> >> +        try_pin_init!( Self {
> >> +            led <- Opaque::try_ffi_init(move |place: *mut bindings::led_classdev| {
> >> +            // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
> >> +            unsafe { place.write_bytes(0, 1) };
> >> +
> >> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> >> +            unsafe { Self::build_with_name(place, name) };
> >> +
> >> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> >> +            unsafe { Self::build_config(place, config) };
> >> +
> >> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> >> +            unsafe { Self::build_vtable(place) };
> >> +
> >> +        let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
> >> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> >> +        crate::error::to_result(unsafe {
> >> +            bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
> >> +        })
> >> +            }),
> >> +            data: data,
> >> +        })
> >> +    }
> >> +
> >> +    /// Add nameto the led_classdev.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// `ptr` has to be valid.
> >> +    unsafe fn build_with_name(ptr: *mut bindings::led_classdev, name: &'a CStr) {
> >> +        // SAFETY: `ptr` is pointing to a live allocation, so the deref is safe.
> >> +        let name_ptr = unsafe { ptr::addr_of_mut!((*ptr).name) };
> >> +        // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
> >> +        unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
> >> +    }
> >> +
> >> +    /// Add config to led_classdev.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// `ptr` has to be valid.
> >> +    unsafe fn build_config(ptr: *mut bindings::led_classdev, config: &'a LedConfig) {
> >> +        // SAFETY: `ptr` is pointing to a live allocation, so the deref is safe.
> >> +        let color_ptr = unsafe { ptr::addr_of_mut!((*ptr).color) };
> >> +        // SAFETY: `color_ptr` points to a valid allocation and we have exclusive access.
> >> +        unsafe { ptr::write(color_ptr, config.color.into()) };
> >> +    }
> >> +}
> >
> > This usage of lifetimes looks incorrect to me. It looks like you are
> > trying to say that the references must be valid for longer than the
> > Led<T>, but what you are writing here does not enforce that. The Led
> > struct must be annotated with the 'a lifetime if you want that, but
> > I'm inclined to say you should not go for the lifetime solution in the
> > first place.
>
> The `led_classdev_register_ext` function copies the name, therefore the idea was that the name only has to exists until the pin init function is called, which should be the case with how I used the lifetimes here

In that case you should be able to get rid of the lifetime like this:

impl<T> Led<T>
where
    T: Operations,
{
    /// Register a new LED with a predefine name.
    pub fn register_with_name(
        name: &CStr,
        device: Option<&Device>,
        config: &LedConfig,
        data: T,
    ) -> impl PinInit<Self, Error> {
        ...
    }