Re: [PATCH v17 5/6] rust: sync: Add SetOnce::reset()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Potential double-drop vulnerability (Use-After-Free) if the inner value's Drop implementation panics.
- [Low] Incorrect use of inline annotations for abstraction functions.
- [Low] The top-level docstring for `SetOnce` is contradicted by the newly added `reset` function.
--

commit 7a51e89f6588dab952d801924de16b34983e224f
Author: Lyude Paul <lyude@redhat.com>

rust: sync: Add SetOnce::reset()

This function simply drops the contents of the SetOnce, given a mutable
reference - since that proves we have exclusive access to the SetOnce.
Additionally, update the invariants for SetOnce to make it clear as to why
this is safe.

> diff --git a/rust/kernel/sync/set_once.rs b/rust/kernel/sync/set_once.rs
> index 139cef05e935f..d6e4fc2695673 100644
> --- a/rust/kernel/sync/set_once.rs
> +++ b/rust/kernel/sync/set_once.rs
> @@ -15,7 +15,7 @@
>  ///
>  /// # Invariants
>  ///
> -/// - `init` may only increase in value.
> +/// - `init` may only increase in value, unless modified through a mutable reference.
>  /// - `init` may only assume values in the range `0..=2`.

[Severity: Low]
Since this patch adds the ability to reset and repopulate the container,
does the top-level struct documentation for SetOnce also need updating?

The struct docstring guarantees that the container can be populated at most
once and remains populated by the same object for its lifetime, which
appears to be contradicted by the new reset() method.

[ ... ]

> @@ -110,17 +110,61 @@ pub fn copy(&self) -> Option<T>
>      {
>          self.as_ref().copied()
>      }
> +
> +    /// # Safety
> +    ///
> +    /// If this function returns `true`, `self` must be freed or `init` must be reset to `0`.
> +    unsafe fn drop_val(&mut self) -> bool {

[Severity: Low]
This isn't a bug, but according to the Rust subsystem coding guidelines,
should small abstraction functions like drop_val() be annotated with
the #[inline] attribute?

> +        if *self.init.get_mut() != 2 {
> +            return false;
> +        }
> +
> +        let value = self.value.get_mut();
> +        // SAFETY: Via our type invariants, `init` == 2 means `value` is initialized.
> +        unsafe { value.assume_init_drop() };
> +
> +        true
> +    }
> +
> +    /// Unset the [`SetOnce`].
> +    ///
> +    /// After this function is called, the [`SetOnce`] is empty and uninitialized. This function is
[ ... ]
> +    pub fn reset(&mut self) -> bool {

[Severity: Low]
This isn't a bug, but should reset() also have an #[inline] annotation
following the subsystem guidelines for small abstraction functions?

> +        // SAFETY: We write `0` to init below if this returns true.
> +        let dropped = unsafe { self.drop_val() };
> +        if dropped {
> +            // INVARIANT:
> +            // - We set `init` back to 0 through a mutable reference.
> +            // - We dropped `value` above.
> +            *self.init.get_mut() = 0;
> +        }
> +
> +        dropped
> +    }

[Severity: High]
Could this code sequence lead to a double-drop use-after-free if the
inner value's drop implementation panics?

If drop_val() is called and value.assume_init_drop() panics, the process
will unwind. Because *self.init.get_mut() = 0 is only executed after
drop_val() returns successfully, init will remain 2.

During the unwind, the SetOnce drop implementation would execute:

SetOnce::drop() -> drop_val()

Since init is still 2, drop_val() would call value.assume_init_drop()
again on the already-dropped memory.

>  impl<T> Drop for SetOnce<T> {
> +    #[inline(always)]
>      fn drop(&mut self) {

[Severity: Low]
This isn't a bug, but the Rust subsystem guidelines note that
#[inline(always)] should only be used for functions utilizing
build_assert!() that depend on function parameters.

Should this just be a standard #[inline] attribute instead?

> -        if *self.init.get_mut() == 2 {
> -            let value = self.value.get_mut();
> -            // SAFETY: By the type invariants of `Self`, `self.init == 2` means that `self.value`
> -            // contains a valid value. We have exclusive access, as we hold a `mut` reference to
> -            // `self`.
> -            unsafe { value.assume_init_drop() };
> -        }
> +        // SAFETY: We are dropping this value.
> +        unsafe { self.drop_val() };
>      }
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603195210.693856-1-lyude@redhat.com?part=5