Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] Unsound transmute in `FileOps::fops` leads to Stack Use-After-Free due to silent constant promotion failure for non-'static types. - [High] Unsound reliance on RAII (Drop) to manage C API unregistration for non-'static types allows a Use-After-Free via `mem::forget`. - [Medium] The patch missed dropping the `T: 'static` bound from `ScopedDir`'s callback-based file creation methods. - [Low] Missing `#[inline]` annotation on the small abstraction function `fops`. --
commit 7b2fa56e56ab7cdc906ac4f77ac3f200f9ffe65e Author: Danilo Krummrich <[email protected]> rust: debugfs: drop 'static bound from ScopedDir file creation methods This commit drops the T: 'static bound from various ScopedDir file creation methods to support registering debugfs files backed by types that contain non-'static references. It formalizes a type invariant that FileOps instances reside in static storage and replaces the Deref implementation with an explicit fops() method. > diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs > index d7b8014a64746..2beb55d444ca1 100644 > --- a/rust/kernel/debugfs.rs > +++ b/rust/kernel/debugfs.rs [ ... ] > @@ -550,7 +550,7 @@ fn create_file<T: Sync>(&self, name: &CStr, data: &'data > T, vtable: &'static Fil > /// This function does not produce an owning handle to the file. The > created > /// file is removed when the [`Scope`] that this directory belongs > /// to is dropped. > - pub fn read_only_file<T: Writer + Send + Sync + 'static>(&self, name: > &CStr, data: &'data T) { > + pub fn read_only_file<T: Writer + Send + Sync>(&self, name: &CStr, data: > &'data T) { > self.create_file(name, data, &T::FILE_OPS) > } [Severity: High] Does removing the 'static bound introduce a potential use-after-free if the directory scope is leaked? If a developer safely leaks a Pin<KBox<Scope<T>>> (e.g., via core::mem::forget) where T borrows a local stack variable, the Drop-based unregistration will be skipped. The local variables borrowed by T will still expire, but the C VFS will retain a pointer to them, leading to a dangling pointer when the debugfs file is later accessed. [ ... ] > @@ -596,11 +592,7 @@ pub fn read_callback_file<T, F>(&self, name: &CStr, > data: &'data T, _f: &'static > /// This function does not produce an owning handle to the file. The > created > /// file is removed when the [`Scope`] that this directory belongs > /// to is dropped. > - pub fn read_write_file<T: Writer + Reader + Send + Sync + 'static>( > - &self, > - name: &CStr, > - data: &'data T, > - ) { > + pub fn read_write_file<T: Writer + Reader + Send + Sync>(&self, name: > &CStr, data: &'data T) { > let vtable = &<T as ReadWriteFile<_>>::FILE_OPS; > self.create_file(name, data, vtable) > } [Severity: Medium] Was it intended to leave the T: 'static bound in place for the callback-based methods like read_callback_file() and read_write_callback_file()? The commit states the goal is to drop the 'static bound from ScopedDir's file creation methods, but looking at the full file, it appears it was retained on the callback variants. [ ... ] > diff --git a/rust/kernel/debugfs/file_ops.rs b/rust/kernel/debugfs/file_ops.rs > index f15908f71c4a2..7e1dd8c75ad92 100644 > --- a/rust/kernel/debugfs/file_ops.rs > +++ b/rust/kernel/debugfs/file_ops.rs [ ... ] > @@ -65,11 +67,11 @@ pub(super) const fn adapt(&self) -> &FileOps<T::Inner> { > } > > #[cfg(CONFIG_DEBUG_FS)] > -impl<T> Deref for FileOps<T> { > - type Target = bindings::file_operations; > - > - fn deref(&self) -> &Self::Target { > - &self.operations > +impl<T> FileOps<T> { > + /// Returns a `'static` reference to the inner `file_operations`. > + pub(crate) fn fops(&self) -> &'static bindings::file_operations { [Severity: Low] Does this small abstraction function need the #[inline] attribute? The subsystem coding guidelines specify that small functions in the kernel crate that forward to a C binding call or wrap basic data access should be annotated with #[inline]. > + // SAFETY: By the type invariant, `self` resides in static storage. > + unsafe { core::mem::transmute(&self.operations) } > } > } [Severity: Critical] Can this transmute cause a stack use-after-free by extending a short-lived reference to 'static? Since the T: 'static bound was dropped from methods like read_only_file(), using a non-'static T will cause constant promotion to fail when evaluating &T::FILE_OPS. This creates a temporary FileOps<T> on the local stack instead of in static storage. When fops() is called on this stack temporary, the transmute bypasses the actual lifetime and returns a 'static reference to stack memory. When the stack frame pops, the C VFS is left holding a dangling pointer. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
