Rust bindings for gem shmem
Subsystems (6)
Lyude Paul
reviewerv17 cover Rust bindings for gem shmem
Most of this patch series has already been pushed upstream, this is just the second half of the patch series that has not been pushed yet + some additional changes which were required to implement changes requested by the mailing list. This patch series is originally from Asahi, previously posted by Daniel Almeida.
The previous version of the patch series can be found here:
https://patchwork.freedesktop.org/series/164580/
Branch with patches applied available here:
https://gitlab.freedesktop.org/lyudess/linux/-/commits/rust/gem-shmem
This patch series applies on top of drm-rust-next
Patch-series wide changes since V15:
* Fix some major rebasing errors I somehow didn't notice :(
* Drop the dependency on LazyInit, use the trick that Alice suggested instead.
* Fix dependency ordering so that Tyr can get the vmap stuff first without the other bits.
Patch-series wide changes since V16:
* Fix ordering one more time (SetOnce::reset() doesn't need to come before adding vmap functions)
* Rebase against the latest DeviceContext changes from me that got pushed.
Lyude Paul (6):
rust: drm: gem: shmem: Fix Default implementation for ObjectConfig
rust: drm: gem: shmem: Add DmaResvGuard helper
rust: drm: gem: shmem: Add vmap functions
rust: faux: Allow retrieving a bound Device
rust: sync: Add SetOnce::reset()
rust: drm: gem: Introduce shmem::Object::sg_table()
rust/kernel/drm/gem/shmem.rs | 518 ++++++++++++++++++++++++++++++++++- rust/kernel/faux.rs | 7 +- rust/kernel/sync/set_once.rs | 60 +++- 3 files changed, 563 insertions(+), 22 deletions(-)
base-commit: 723bd79ca9e492cc91850094a2892bde0345c51a
Signature · 1 line
2.54.0
Lyude Paul
reviewerPATCH v17 1/6 rust: drm: gem: shmem: Fix Default implementation for ObjectConfig
I completely forgot when coming up with this type that #[derive(Default)] only works if all generics mentioned in the type implement Default (and T usually doesn't). This being said: We don't use T for anything besides using it for a reference type, so whether or not it implements Default shouldn't actually need to matter.
So, fix this by just manually implementing Default instead of deriving it.
---
V17:
* Rebase
* Fix format of commit message subject
rust/kernel/drm/gem/shmem.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
rust/kernel/drm/gem/shmem.rs +10 -1
@@ -42,7 +42,6 @@/// /// This is used with [`Object::new()`] to control various properties that can only be set when /// initially creating a shmem-backed GEM object. -#[derive( Default )] pub struct ObjectConfig <'a,T :DriverObject ,C :DeviceContext =Registered > {/// Whether to set the write-combine map flag. pub map_wc:bool ,
@@ -53,6 +52,16 @@ pub struct ObjectConfig<'a, T: DriverObject, C: DeviceContext = Registered> { pub parent_resv_obj: Option<&'a Object<T, C>>, } +impl <'a,T :DriverObject ,C :DeviceContext >Default for ObjectConfig <'a,T ,C > { +#[inline(always)] +fn default () ->Self { +Self { + map_wc:false , + parent_resv_obj:None , + } + } +} +/// A shmem-backed GEM object. /// /// # Invariants
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v17 1/6] rust: drm: gem: shmem: Fix Default implementation for ObjectConfig
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Incorrect use of `#[inline(always)]` annotation for `ObjectConfig::default()`. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Incorrect use of #[inline(always)] annotation for ObjectConfig::default(). --
commit 974110b1b618554fdff7106e2b5a8152da0f6df4
Author: Lyude Paul <[email protected]>
rust: drm: gem: shmem: Fix Default implementation for ObjectConfig
This commit replaces the derived Default implementation for ObjectConfig with a manual one. This avoids the requirement for the generic type T to also implement Default, since T is only used as a reference type here.
Quoted 4 lines
rust/kernel/drm/gem/shmem.rs +0
-0
[ ... ]
rust/kernel/drm/gem/shmem.rs pub struct ObjectConfig<'a, T: DriverObject, C: DeviceContext = Registered> { · line 57
Quoted 7 lines
rust/kernel/drm/gem/shmem.rs +3
-0@@ -53,6 +52,16 @@ pub struct ObjectConfig<'a, T: DriverObject, C: DeviceContext = Registered> { pub parent_resv_obj: Option<&'a Object<T, C>>, } +impl <'a,T :DriverObject ,C :DeviceContext >Default for ObjectConfig <'a,T ,C > { +#[inline(always)] +fn default () ->Self {
[Severity: Low]
This isn't a bug, but should this be annotated with #[inline] instead of #[inline(always)]?
According to the Rust subsystem guidelines, #[inline(always)] is reserved for functions using build_assert!() that depend on function parameters, while #[inline] is meant for small abstraction functions like ObjectConfig::default().
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +7
-0+Self { + map_wc:false , + parent_resv_obj:None , + } + } +} +/// A shmem-backed GEM object. /// /// # Invariants
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Alice Ryhl
maintainerRe: [PATCH v17 1/6] rust: drm: gem: shmem: Fix Default implementation for ObjectConfig
In reply to Lyude PaulQuoted 9 lines · Lyude Paul
I completely forgot when coming up with this type that #[derive(Default)] only works if all generics mentioned in the type implement Default (and T usually doesn't). This being said: We don't use
Tfor anything besides using it for a reference type, so whether or not it implementsDefaultshouldn't actually need to matter.So, fix this by just manually implementing Default instead of deriving it.
Signed-off-by Lyude Paul <[email protected]>
This one is so simple I went ahead and applied it to drm-rust-next right away.
Alice
Lyude Paul
reviewerPATCH v17 2/6 rust: drm: gem: shmem: Add DmaResvGuard helper
In reply to Lyude PaulJust a temporary holdover to make locking/unlocking the dma_resv lock much easier.
---
V17:
* Fix format of commit message title
rust/kernel/drm/gem/shmem.rs | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)
rust/kernel/drm/gem/shmem.rs +30 -1
@@ -30,7 +30,10 @@ Deref, DerefMut, // }, - ptr::NonNull, // + ptr::{ + self, +NonNull ,// + }, };use gem::{BaseObjectPrivate ,
@@ -244,3 +247,29 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> { dumb_map_offset: None, }; } + +/// Private helper-type for holding the `dma_resv` object for a GEM shmem object. +/// +/// When this is dropped, the `dma_resv` lock is dropped as well. +/// +// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel. +struct DmaResvGuard <'a,T :DriverObject ,C :DeviceContext =Registered >(&'aObject <T ,C >); + +impl <'a,T :DriverObject ,C :DeviceContext >DmaResvGuard <'a,T ,C > { +#[inline(always)] +#[expect(unused)] +fn new (obj: &'aObject <T ,C >) ->Self { +// SAFETY: This lock is initialized throughout the lifetime of `object`. +unsafe { bindings::dma_resv_lock (obj.raw_dma_resv (), ptr::null_mut ()) }; + +Self (obj) + } +} + +impl <'a,T :DriverObject ,C :DeviceContext >Drop for DmaResvGuard <'a,T ,C > { +#[inline(always)] +fn drop (&mut self) { +// SAFETY: We are releasing the lock grabbed during the creation of this object. +unsafe { bindings::dma_resv_unlock (self.0 .raw_dma_resv ()) }; + } +}
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v17 2/6] rust: drm: gem: shmem: Add DmaResvGuard helper
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] `DmaResvGuard` implicitly implements `Send`, allowing the lock to be sent to and unlocked by a…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] DmaResvGuard implicitly implements Send, allowing the lock to be sent to and unlocked by a different thread, violating kernel locking rules.
- [Medium] DmaResvGuard lacks the #[must_use] attribute, which can lead to accidental immediate lock drops.
- [Low] Incorrect use of #[inline(always)] instead of #[inline] on abstraction functions.
--
commit 35ca0f9ce4df1005935e123463429a164cf640a5
Author: Lyude Paul <[email protected]>
rust: drm: gem: shmem: Add DmaResvGuard helper
Just a temporary holdover to make locking/unlocking the dma_resv lock much easier.
Quoted 4 lines
rust/kernel/drm/gem/shmem.rs +0
-0
[ ... ]
rust/kernel/drm/gem/shmem.rs impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> { · line 256
Quoted 11 lines
rust/kernel/drm/gem/shmem.rs +7
-0@@ -244,3 +247,29 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> { dumb_map_offset: None, }; } + +/// Private helper-type for holding the `dma_resv` object for a GEM shmem object. +/// +/// When this is dropped, the `dma_resv` lock is dropped as well. +/// +// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel. +struct DmaResvGuard <'a,T :DriverObject ,C :DeviceContext =Registered >(&'aObject <T ,C >);
[Severity: High]
Does DmaResvGuard implicitly implement Send, allowing the lock to be acquired in one thread, sent to another thread, and unlocked there?
Since Object<T, C> implements Sync, the shared reference &'a Object<T, C> is Send. However, bindings::dma_resv_unlock internally calls ww_mutex_unlock(), which requires the mutex to be unlocked by the exact same task that locked it.
Should this struct include NotThreadSafe (or PhantomData<*mut ()>) to ensure it cannot be sent across threads?
[Severity: Medium]
Could this struct also be annotated with #[must_use]?
Without it, if a caller writes DmaResvGuard::new(obj); instead of let _guard = DmaResvGuard::new(obj);, the lock is acquired and immediately released at the end of the statement, leaving the rest of the scope unprotected without triggering a compiler warning.
rust/kernel/drm/gem/shmem.rs impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> { · line 259
Quoted 3 lines
rust/kernel/drm/gem/shmem.rs +3
-0+ +impl <'a,T :DriverObject ,C :DeviceContext >DmaResvGuard <'a,T ,C > { +#[inline(always)]
[Severity: Low]
This isn't a bug, but according to the Rust coding guidelines, shouldn't small abstraction functions or binding wrappers like this be annotated with #[inline] rather than #[inline(always)]?
#[inline(always)] is generally reserved for functions using build_assert!() that depend on function parameters.
rust/kernel/drm/gem/shmem.rs impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> { · line 270
Quoted 11 lines
rust/kernel/drm/gem/shmem.rs +11
-0+#[expect(unused)] +fn new (obj: &'aObject <T ,C >) ->Self { +// SAFETY: This lock is initialized throughout the lifetime of `object`. +unsafe { bindings::dma_resv_lock (obj.raw_dma_resv (), ptr::null_mut ()) }; + +Self (obj) + } +} + +impl <'a,T :DriverObject ,C :DeviceContext >Drop for DmaResvGuard <'a,T ,C > { +#[inline(always)]
[Severity: Low]
This isn't a bug, but similar to the new() function above, should this be #[inline] instead of #[inline(always)]?
Quoted 5 lines
rust/kernel/drm/gem/shmem.rs +5
-0+fn drop (&mut self) { +// SAFETY: We are releasing the lock grabbed during the creation of this object. +unsafe { bindings::dma_resv_unlock (self.0 .raw_dma_resv ()) }; + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Lyude Paul
reviewerPATCH v17 3/6 rust: drm: gem: shmem: Add vmap functions
In reply to Lyude PaulOne of the more obvious use cases for gem shmem objects is the ability to create mappings into their contents. So, let's hook this up in our rust bindings.
---
V7:
* Switch over to the new iosys map bindings that use the Io trait V8:
* Get rid of iosys_map bindings for now, only support non-iomem types
* s/as_shmem()/as_raw_shmem()
V9:
* Get rid of some outdated comments I missed
* Add missing SIZE check to raw_vmap()
* Add a proper unit test that ensures that we actually validate SIZE at compile-time.
Turns out it takes only 34 lines to make a boilerplate DRM driver for a kunit test :)
* Add unit tests
* Add some missing #[inline]s
V10:
* Correct issue with iomem error path
We previously called raw_vunmap() if we got an iomem allocation, but raw_vunmap() was written such that it assumed all allocations were sysmem allocations. Fix this by just making raw_vunmap() accept a iosys_map.
V11:
* Use Alexandre's clever solution to remove the macros we were using for maintaining two different VMap types.
* Change the order of items in Object<T> to ensure that sgt_res is always dropped before obj.
* Fix typo in Object.raw_vmap()
* s/raw_vmap()/make_vmap()/
Deduplicate code a bit more as well by using more generics here
V15:
* Add these patches back
* We only have one VMap type now!
* Use ObjectConfig::default() in unit tests since we unbroke it. V16:
* Fix huge rebase error I made and did not notice that squashed 1.5 patches together that were definitely not supposed to be squashed
* Update old commit message
V17:
* Rebase
* Fix format of commit message title
rust/kernel/drm/gem/shmem.rs | 312 ++++++++++++++++++++++++++++++++++- 1 file changed, 311 insertions(+), 1 deletion(-)
rust/kernel/drm/gem/shmem.rs +311 -1
@@ -20,12 +20,19 @@ Registered, // }, error::to_result, + io::{ + Io, + IoCapable, + IoKnownSize, // + }, prelude::*, sync::aref::ARef, types::Opaque, // };use core::{ + ffi::c_void, marker::PhantomData , + mem::MaybeUninit ,// ops::{Deref ,DerefMut ,//
@@ -36,6 +43,7 @@ }, };use gem::{ +BaseObject ,BaseObjectPrivate ,DriverObject ,IntoGEMObject ,//
@@ -197,6 +205,80 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {// SAFETY: We're recovering the Kbox<> we created in gem_create_object() let _ =unsafe {KBox ::from_raw (this) }; } + +/// Attempt to create a vmap from the gem object, and confirm the size of said vmap. +fn make_vmap <'a,R ,const SIZE :usize >(&'a self) ->Result <VMap <T ,R ,C ,SIZE >> +where +R :Deref <Target =Self > +From <&'aSelf >, + { +// INVARIANT: We check here that the gem object is at least as large as `SIZE`. +if self.size () <SIZE { +return Err (ENOSPC ); + } + +let mut map:MaybeUninit <bindings::iosys_map > =MaybeUninit ::uninit (); +let guard =DmaResvGuard ::new (self); + +// SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held +to_result (unsafe { + bindings::drm_gem_shmem_vmap_locked (self.as_raw_shmem (), map.as_mut_ptr ()) + })?; + +// Drop the guard explicitly here, since we may need to call raw_vunmap() (which re-acquires +// the lock). +drop (guard); + +// SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed that +// map is properly initialized. +let map =unsafe { map.assume_init () }; + +// XXX: We don't currently support iomem allocations +if map.is_iomem { +// SAFETY: +// - The vmap operation above succeeded, guaranteeing that `map` points to a valid +// memory mapping. +// - We checked that this is an iomem allocation, making it safe to read vaddr_iomem +unsafe { self.raw_vunmap (map) }; + +Err (ENOTSUPP ) + }else { +Ok (VMap { +// SAFETY: We checked that this is not an iomem allocation, making it safe to read +// vaddr + addr:unsafe { map.__bindgen_anon_1.vaddr }, + owner: self.into (), + }) + } + } + +/// Unmap a vmap from the gem object. +/// +/// # Safety +/// +/// - The caller promises that `map` is a valid vmap on this gem object. +/// - The caller promises that the memory pointed to by map will no longer be accesed through +/// this instance. +unsafe fn raw_vunmap (&self,mut map: bindings::iosys_map ) { +let _guard =DmaResvGuard ::new (self); + +// SAFETY: +// - This function is safe to call with the DMA reservation lock held. +// - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to +// dereference. +unsafe { bindings::drm_gem_shmem_vunmap_locked (self.as_raw_shmem (), &mut map) }; + } + +/// Creates and returns a virtual kernel memory mapping for this object. +#[inline] +pub fn vmap <const SIZE :usize >(&self) ->Result <VMapRef <'_,T ,C ,SIZE >> { + self.make_vmap () + } + +/// Creates and returns an owned reference to a virtual kernel memory mapping for this object. +#[inline] +pub fn owned_vmap <const SIZE :usize >(&self) ->Result <VMapOwned <T ,C ,SIZE >> { + self.make_vmap () + } }impl <T :DriverObject ,C :DeviceContext >Deref for Object <T ,C > {
@@ -257,7 +339,6 @@ impl<T: DriverObject, C: DeviceContext> driver::AllocImpl for Object<T, C> {impl <'a,T :DriverObject ,C :DeviceContext >DmaResvGuard <'a,T ,C > {#[inline(always)] -#[expect(unused)] fn new (obj: &'aObject <T ,C >) ->Self {// SAFETY: This lock is initialized throughout the lifetime of `object`. unsafe { bindings::dma_resv_lock (obj.raw_dma_resv (), ptr::null_mut ()) };
@@ -273,3 +354,232 @@ fn drop(&mut self) {unsafe { bindings::dma_resv_unlock (self.0 .raw_dma_resv ()) }; } } + +macro_rules! impl_vmap_io_capable { + ($impl:ident, $ty:ty) => { +impl <D ,R ,C ,const SIZE :usize >IoCapable <$ty>for $impl<D ,R ,C ,SIZE > +where +D :DriverObject , +C :DeviceContext , +R :Deref <Target =Object <D ,C >>, + { +#[inline(always)] +unsafe fn io_read (&self, address:usize ) -> $ty { +let ptr = addressas *mut $ty; + +// SAFETY: The safety contract of `io_read` guarantees that address is a valid +// address within the bounds of `Self` of at least the size of $ty, and is properly +// aligned. +unsafe { ptr::read (ptr) } + } + +#[inline(always)] +unsafe fn io_write (&self, value: $ty, address:usize ) { +let ptr = addressas *mut $ty; + +// SAFETY: The safety contract of `io_write` guarantees that address is a valid +// address within the bounds of `Self` of at least the size of $ty, and is properly +// aligned. +unsafe { ptr::write (ptr, value) } + } + } + }; +} + +/// A reference to a virtual mapping for an shmem-based GEM object in kernel address space. +/// +/// # Invariants +/// +/// - The size of `owner` is >= SIZE. +/// - The memory pointed to by addr remains valid at least until this object is dropped. +pub struct VMap <D ,R ,C =Registered ,const SIZE :usize =0 > +where +D :DriverObject , +C :DeviceContext , +R :Deref <Target =Object <D ,C >>, +{ + addr: *mut c_void , + owner:R , +} + +/// An alias type for a reference to a shmem-based GEM object's VMap. +pub type VMapRef <'a,D ,C ,const SIZE :usize =0 > =VMap <D , &'aObject <D ,C >,C ,SIZE >; + +/// An alias type for an owned reference to a shmem-based GEM object's VMap. +pub type VMapOwned <D ,C ,const SIZE :usize =0 > =VMap <D ,ARef <Object <D ,C >>,C ,SIZE >; + +impl <D ,R ,C ,const SIZE :usize >VMap <D ,R ,C ,SIZE > +where +D :DriverObject , +C :DeviceContext , +R :Deref <Target =Object <D ,C >>, +{ +/// Borrows a reference to the object that owns this virtual mapping. +#[inline(always)] +pub fn owner (&self) -> &Object <D ,C > { + &self.owner + } +} + +impl <D ,R ,C ,const SIZE :usize >Drop for VMap <D ,R ,C ,SIZE > +where +D :DriverObject , +C :DeviceContext , +R :Deref <Target =Object <D ,C >>, +{ +#[inline(always)] +fn drop (&mut self) { +// SAFETY: +// - Our existence is proof that this map was previously created using self.owner. +// - Since we are in Drop, we are guaranteed that no one will access the memory +// through this mapping after calling this. +unsafe { + self.owner.raw_vunmap (bindings::iosys_map { + is_iomem:false , + __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr }, + }) + }; + } +} + +impl <D ,R ,C ,const SIZE :usize >Io for VMap <D ,R ,C ,SIZE > +where +D :DriverObject , +C :DeviceContext , +R :Deref <Target =Object <D ,C >>, +{ +#[inline(always)] +fn addr (&self) ->usize { + self.addras usize + } + +#[inline(always)] +fn maxsize (&self) ->usize { + self.owner.size () + } +} + +impl <D ,R ,C ,const SIZE :usize >IoKnownSize for VMap <D ,R ,C ,SIZE > +where +D :DriverObject , +C :DeviceContext , +R :Deref <Target =Object <D ,C >>, +{ +const MIN_SIZE :usize =SIZE ; +} + +impl_vmap_io_capable! (VMap ,u8 ); +impl_vmap_io_capable! (VMap ,u16 ); +impl_vmap_io_capable! (VMap ,u32 ); +#[cfg( CONFIG_64BIT )] +impl_vmap_io_capable! (VMap ,u64 ); + +#[kunit_tests(rust_drm_gem_shmem)] +mod tests { +use super ::*; +use crate ::{ + drm::{ + self, +UnregisteredDevice ,// + }, + faux, + page::PAGE_SIZE ,// + }; + +// The bare minimum needed to create a fake drm driver for kunit + +#[pin_data] +struct KunitData {} +struct KunitDriver ; +struct KunitFile ; +#[pin_data] +struct KunitObject {} + +const INFO : drm::DriverInfo = drm::DriverInfo { + major:0 , + minor:0 , + patchlevel:0 , + name:c"kunit" , + desc:c"Kunit" , + }; + +impl drm::file::DriverFile for KunitFile { +type Driver =KunitDriver ; + +fn open (_dev: &drm::Device <KunitDriver >) ->Result <Pin <KBox <Self >>> { +Ok (KBox ::new (Self ,GFP_KERNEL )?.into ()) + } + } + +impl gem::DriverObject for KunitObject { +type Driver =KunitDriver ; +type Args = (); + +fn new <C :DeviceContext >( + _dev: &drm::Device <KunitDriver ,C >, + _size:usize , + _args:Self ::Args , + ) ->impl PinInit <Self ,Error > { +try_pin_init! (KunitObject {}) + } + } + +#[vtable] +impl drm::Driver for KunitDriver { +type Data =KunitData ; +type File =KunitFile ; +type Object <Ctx :DeviceContext > =Object <KunitObject ,Ctx >; + +const INFO : drm::DriverInfo =INFO ; +const IOCTLS : &'static [drm::ioctl::DrmIoctlDescriptor ] = &[]; + } + +fn create_drm_dev () ->Result <(faux::Registration ,UnregisteredDevice <KunitDriver >)> { +// Create a faux DRM device so we can test gem object creation. +let data =try_pin_init! (KunitData {}); +let dev = faux::Registration ::new (c"Kunit" ,None )?; +let drm =UnregisteredDevice ::new (dev.as_ref (), data)?; + +Ok ((dev, drm)) + } + +#[test] +fn compile_time_vmap_sizes () ->Result { +let (_dev, drm) =create_drm_dev ()?; + +let obj =Object ::<KunitObject ,_ >::new (&drm,PAGE_SIZE ,ObjectConfig ::default (), ())?; + +// Try creating a normal vmap + obj.vmap ::<PAGE_SIZE >()?; + +// Try creating a vmap that's smaller then the size we specified + obj.vmap ::<{PAGE_SIZE -100 }>()?; + +// Make sure creating a vmap that's too large fails +assert! (obj.vmap ::<{PAGE_SIZE +200 }>().is_err ()); + +Ok (()) + } + +#[test] +fn vmap_io () ->Result { +let (_dev, drm) =create_drm_dev ()?; + +let obj =Object ::<KunitObject ,_ >::new (&drm,PAGE_SIZE ,ObjectConfig ::default (), ())?; + +let vmap = obj.vmap ::<PAGE_SIZE >()?; + + vmap.write8 (0xDE ,0x0 ); +assert_eq! (vmap.read8 (0x0 ),0xDE ); + vmap.write32 (0xFFFFFFFF ,0x20 ); + +assert_eq! (vmap.read32 (0x20 ),0xFFFFFFFF ); + +assert_eq! (vmap.read8 (0x20 ),0xFF ); +assert_eq! (vmap.read8 (0x21 ),0xFF ); +assert_eq! (vmap.read8 (0x22 ),0xFF ); +assert_eq! (vmap.read8 (0x23 ),0xFF ); + +Ok (()) + } +}
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v17 3/6] rust: drm: gem: shmem: Add vmap functions
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Missing volatile memory accesses in `IoCapable` implementation for `VMap`. - [Medium]…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing volatile memory accesses in IoCapable implementation for VMap.
- [Medium] Unconditional reservation lock acquisition causes runtime deadlocks when vmapping from locked contexts.
- [Low] Missing // INVARIANT: comment when constructing VMap.
- [Low] Incorrect safety comment referencing ARef in raw_vunmap.
--
commit c7d123d2fcea8aeccc045288b1eb2a5cc650d43c
Author: Lyude Paul <[email protected]>
rust: drm: gem: shmem: Add vmap functions
This commit adds vmap bindings for GEM shmem objects in Rust. It introduces functions to create and manage virtual memory mappings for these objects, including IoCapable implementations for the VMap structure.
Quoted 4 lines
rust/kernel/drm/gem/shmem.rs +0
-0
[ ... ]
Quoted 7 lines
rust/kernel/drm/gem/shmem.rs +3
-0@@ -197,6 +205,80 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {// SAFETY: We're recovering the Kbox<> we created in gem_create_object() let _ =unsafe {KBox ::from_raw (this) }; } + +/// Attempt to create a vmap from the gem object, and confirm the size of said vmap. +fn make_vmap <'a,R ,const SIZE :usize >(&'a self) ->Result <VMap <T ,R ,C ,SIZE >>
rust/kernel/drm/gem/shmem.rs extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { · line 220
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +10
-0+ where + R: Deref<Target = Self> + From<&'a Self>, + { +// INVARIANT: We check here that the gem object is at least as large as `SIZE`. +if self.size () <SIZE { +return Err (ENOSPC ); + } + +let mut map:MaybeUninit <bindings::iosys_map > =MaybeUninit ::uninit (); +let guard =DmaResvGuard ::new (self);
[Severity: Medium]
Could unconditionally acquiring the DMA reservation lock here lead to a deadlock if make_vmap() is called while the lock is already held? In the DRM subsystem, it is common to vmap a GEM object while its lock is already held, such as during a commit pipeline or inside drm_exec().
Quoted 15 lines
rust/kernel/drm/gem/shmem.rs +15
-0+ +// SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held +to_result (unsafe { + bindings::drm_gem_shmem_vmap_locked (self.as_raw_shmem (), map.as_mut_ptr ()) + })?; + +// Drop the guard explicitly here, since we may need to call raw_vunmap() (which re-acquires +// the lock). +drop (guard); + +// SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed that +// map is properly initialized. +let map =unsafe { map.assume_init () }; + +// XXX: We don't currently support iomem allocations
rust/kernel/drm/gem/shmem.rs extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { · line 245
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +10
-0+if map.is_iomem { +// SAFETY: +// - The vmap operation above succeeded, guaranteeing that `map` points to a valid +// memory mapping. +// - We checked that this is an iomem allocation, making it safe to read vaddr_iomem +unsafe { self.raw_vunmap (map) }; + +Err (ENOTSUPP ) + }else { +Ok (VMap {
[Severity: Low]
Should there be an INVARIANT comment here explaining why the memory validity invariant is satisfied? The VMap struct documentation has an Invariants section stating that the memory remains valid until dropped, so constructing it requires an explanation of how the invariant is met.
Quoted 7 lines
rust/kernel/drm/gem/shmem.rs +7
-0+// SAFETY: We checked that this is not an iomem allocation, making it safe to read +// vaddr + addr: unsafe { map.__bindgen_anon_1.vaddr }, + owner: self.into(), + }) + } + }
rust/kernel/drm/gem/shmem.rs extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { · line 262
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +10
-0+ +/// Unmap a vmap from the gem object. +/// +/// # Safety +/// +/// - The caller promises that `map` is a valid vmap on this gem object. +/// - The caller promises that the memory pointed to by map will no longer be accesed through +/// this instance. +unsafe fn raw_vunmap (&self,mut map: bindings::iosys_map ) { +let _guard =DmaResvGuard ::new (self);
[Severity: Medium]
Will unconditionally acquiring the lock here cause a deadlock if a VMap object is dropped while the reservation lock is already held?
rust/kernel/drm/gem/shmem.rs extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { · line 267
Quoted 5 lines
rust/kernel/drm/gem/shmem.rs +5
-0+ +// SAFETY: +// - This function is safe to call with the DMA reservation lock held. +// - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to +// dereference.
[Severity: Low]
Does this safety comment incorrectly reference an ARef? The raw_vunmap function takes a standard shared reference to self, so there is no ARef involved in its signature, and the safety actually relies on the reference itself guaranteeing the object is alive.
Quoted 2 lines
rust/kernel/drm/gem/shmem.rs +2
-0+unsafe { bindings::drm_gem_shmem_vunmap_locked (self.as_raw_shmem (), &mut map) }; + }
[ ... ]
Quoted 6 lines
rust/kernel/drm/gem/shmem.rs +6
-0+macro_rules! impl_vmap_io_capable { + ($impl:ident, $ty:ty) => { +impl <D ,R ,C ,const SIZE :usize >IoCapable <$ty>for $impl<D ,R ,C ,SIZE > +where +D :DriverObject , +C :DeviceContext ,
rust/kernel/drm/gem/shmem.rs fn drop(&mut self) { · line 373
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +10
-0+ R: Deref<Target = Object<D, C>>, + { +#[inline(always)] +unsafe fn io_read (&self, address:usize ) -> $ty { +let ptr = addressas *mut $ty; + +// SAFETY: The safety contract of `io_read` guarantees that address is a valid +// address within the bounds of `Self` of at least the size of $ty, and is properly +// aligned. +unsafe { ptr::read (ptr) }
[Severity: High]
Since GEM shmem buffers are mapped into both CPU and GPU address spaces, can the compiler optimize away or reorder these standard non-volatile memory accesses?
The equivalent C API uses READ_ONCE for system memory. Would using ptr::read_volatile enforce the necessary volatile semantics to prevent potential issues when interacting with the GPU?
rust/kernel/drm/gem/shmem.rs fn drop(&mut self) { · line 383
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +10
-0+ } + +#[inline(always)] +unsafe fn io_write (&self, value: $ty, address:usize ) { +let ptr = addressas *mut $ty; + +// SAFETY: The safety contract of `io_write` guarantees that address is a valid +// address within the bounds of `Self` of at least the size of $ty, and is properly +// aligned. +unsafe { ptr::write (ptr, value) }
[Severity: High]
Similarly to io_read, can the compiler optimize or delay this non-volatile write?
Would using ptr::write_volatile match the WRITE_ONCE semantics from the C API and prevent the compiler from caching values or reordering the accesses?
Quoted 4 lines
rust/kernel/drm/gem/shmem.rs +4
-0+ } + } + }; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Lyude Paul
reviewerPATCH v17 4/6 rust: faux: Allow retrieving a bound Device
In reply to Lyude PaulWhen writing up some rust code that used faux devices for unit testing, I noticed that we never actually added the Bound device context to faux::Registration's AsRef<device::Device> implementation. This being said:
the Registration object itself is proof that a driver is bound to the device - so this should be safe.
--- rust/kernel/faux.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
rust/kernel/faux.rs +4 -3
@@ -25,7 +25,8 @@/// /// # Invariants /// -/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// - This object is proof that the object described by this `Registration` is bound to a device. /// /// [`struct faux_device`]: srctree/include/linux/device/faux.h pub struct Registration (NonNull <bindings::faux_device >);
@@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device { } } -impl AsRef <device::Device >for Registration { -fn as_ref (&self) -> &device::Device { +impl AsRef <device::Device <device::Bound >>for Registration { +fn as_ref (&self) -> &device::Device <device::Bound > {// SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be // a valid initialized `device`. unsafe { device::Device ::from_raw (addr_of_mut! ((*self.as_raw ()).dev)) }
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v17 4/6] rust: faux: Allow retrieving a bound Device
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Low] The patch adds a new invariant but fails to provide a corresponding `// INVARIANT:` comment in the…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] The patch adds a new invariant but fails to provide a corresponding // INVARIANT: comment in the constructor Registration::new.
- [Low] The newly added invariant text incorrectly states that the object is 'bound to a device'.
- [Low] The SAFETY comment in as_ref fails to address the new Device<Bound> guarantee.
--
commit dc0de65744def0c2e91abb89c0b847d0fc4819f1
Author: Lyude Paul <[email protected]>
rust: faux: Allow retrieving a bound Device
This commit updates the Registration's AsRef implementation to return a device::Device<device::Bound> instead of device::Device. This reflects the guarantee that the Registration object acts as proof that a driver is bound to the device.
rust/kernel/faux.rs line 29
Quoted 11 lines
rust/kernel/faux.rs +2
-1@@ -25,7 +25,8 @@/// /// # Invariants /// -/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// - This object is proof that the object described by this `Registration` is bound to a device.
[Severity: Low]
Is this description completely accurate? Conceptually, devices are bound to drivers, not to devices. The commit message states "a driver is bound to the device," so could this comment be updated to say "bound to a driver" instead?
[Severity: Low]
Since a new invariant was added here, does Registration::new() also need a new // INVARIANT: comment explaining why this condition holds, to match the Rust subsystem coding guidelines?
rust/kernel/faux.rs:Registration::new() {
...
// The above function will return either a valid device, or NULL on failure
// INVARIANT: The device will remain registered until faux_device_destroy() is called, which
// happens in our Drop implementation.
...
}[ ... ]
rust/kernel/faux.rs fn as_raw(&self) -> *mut bindings::faux_device { · line 66
Quoted 10 lines
rust/kernel/faux.rs +2
-2@@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device { } } -impl AsRef <device::Device >for Registration { -fn as_ref (&self) -> &device::Device { +impl AsRef <device::Device <device::Bound >>for Registration { +fn as_ref (&self) -> &device::Device <device::Bound > {// SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be // a valid initialized `device`.
[Severity: Low]
Does this safety comment need to be updated to address the new Bound context?
The device::Device::from_raw() documentation requires the caller to justify the chosen DeviceContext (Bound), ensuring the device is bound to a driver for the duration of the reference. While faux_device_create() in C guarantees driver binding upon success, the safety comment currently doesn't document why this new requirement is met.
Quoted 1 line
rust/kernel/faux.rs +0
-0unsafe { device::Device ::from_raw (addr_of_mut! ((*self.as_raw ()).dev)) }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Danilo Krummrich
maintainerRe: [PATCH v17 4/6] rust: faux: Allow retrieving a bound Device
In reply to Lyude PaulQuoted 5 lines · Lyude Paul
When writing up some rust code that used faux devices for unit testing, I noticed that we never actually added the Bound device context to faux::Registration's AsRef<device::Device> implementation. This being said:
the Registration object itself is proof that a driver is bound to the device - so this should be safe.
Yes, it should be, but the reasons are non-trivial and should be part of the safety comment below.
- faux_match() always returns 1, and probe runs synchronously (PROBE_FORCE_SYNCHRONOUS)
- suppress_bind_attrs = true on faux_driver prevents userspace-triggered unbind via sysfs
- mem::forget(Registration) is not a problem; if the Registration is leaked, the faux device stays bound forever
Quoted 1 line
Signed-off-by Lyude Paul <[email protected]>
I can pick this through the driver-core tree already, as it seems this it not needed by this series. Feel free to resend as individual patch for thus purpose.
Quoted 31 lines
--- rust/kernel/faux.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)rust/kernel/faux.rs +4
-3@@ -25,7 +25,8 @@/// /// # Invariants /// -/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// - This object is proof that the object described by this `Registration` is bound to a device. /// /// [`struct faux_device`]: srctree/include/linux/device/faux.h pub struct Registration (NonNull <bindings::faux_device >);@@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device { } } -impl AsRef <device::Device >for Registration { -fn as_ref (&self) -> &device::Device { +impl AsRef <device::Device <device::Bound >>for Registration { +fn as_ref (&self) -> &device::Device <device::Bound > {// SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be // a valid initialized `device`. unsafe { device::Device ::from_raw (addr_of_mut! ((*self.as_raw ()).dev)) }Signature · 1 line
2.54.0
[email protected]
reviewerRe: [PATCH v17 4/6] rust: faux: Allow retrieving a bound Device
I mentioned this already to Danilo via alternate channels, but to clarify - this patch -is- actually needed, as we make use of AsRef<Device<Bound>> in the kunit test for SGTable
Quoted 68 lines · Danilo Krummrich
Quoted 8 lines · Lyude Paul
When writing up some rust code that used faux devices for unit testing, I
noticed that we never actually added the Bound device context to faux::Registration's AsRef<device::Device> implementation. This being said:
the Registration object itself is proof that a driver is bound to the
device - so this should be safe.Yes, it should be, but the reasons are non-trivial and should be part of the
safety comment below.-
faux_match()always returns 1, and probe runs synchronously (PROBE_FORCE_SYNCHRONOUS)-
suppress_bind_attrs= true onfaux_driverprevents userspace- triggered
unbind via sysfs-
mem::forget(Registration)is not a problem; if the Registration is leaked,
the faux device stays bound foreverQuoted 1 line
Signed-off-by Lyude Paul <[email protected]>I can pick this through the driver-core tree already, as it seems this it not
needed by this series. Feel free to resend as individual patch for thus purpose.Quoted 36 lines
---
rust/kernel/faux.rs | 7 ++++---1 file changed, 4 insertions(+), 3 deletions(-)rust/kernel/faux.rs +0
-1@@ -25,7 +25,8 @@ /// /// # Invariants /// -/// `self.0` always holds a valid pointer to an initialized and registered [
struct faux_device].
+/// -self.0always holds a valid pointer to an initialized and registered [struct faux_device].
+/// - This object is proof that the object described by thisRegistrationis bound to a device.
///
/// [struct faux_device]:srctree/include/linux/device/faux.hpub structRegistration(NonNull<bindings::faux_device>);rust/kernel/faux.rs +0
-0@@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device { } }rust/kernel/faux.rs +2
-2-impl AsRef <device::Device >for Registration { - fn as_ref(&self) -> &device::Device { +impl AsRef <device::Device <device::Bound >>for Registration { + fn as_ref(&self) -> &device::Device<device::Bound> {// SAFETY: The underlying
deviceinfaux_deviceis guaranteed by the C API to be
// a valid initializeddevice.
unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }Signature · 1 line
2.54.0
Lyude Paul
reviewerPATCH v17 5/6 rust: sync: Add SetOnce::reset()
In reply to Lyude PaulThis 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.
--- rust/kernel/sync/set_once.rs | 60 +++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 8 deletions(-)
rust/kernel/sync/set_once.rs +52 -8
@@ -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`. /// - `init == 0` if and only if `value` is uninitialized. /// - `init == 1` if and only if there is exactly one thread with exclusive
@@ -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 { +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 +/// mainly intended for usage in destructors. +/// +/// Returns `true` if `self` was previously initialized. +/// +/// # Example +/// +/// ``` +/// # use kernel::sync::SetOnce; +/// let mut value = SetOnce::new(); +/// assert_eq!(value.populate(67), true); +/// +/// assert_eq!(value.reset(), true); +/// assert!(value.as_ref().is_none()); +/// assert_eq!(value.reset(), false); +/// +/// assert_eq!(value.populate(69), true); +/// ``` +pub fn reset (&mut self) ->bool { +// 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 + } }impl <T >Drop for SetOnce <T > { +#[inline(always)] fn drop (&mut self) { -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 () }; } }
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v17 5/6] rust: sync: Add SetOnce::reset()
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…
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 <[email protected]>
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.
rust/kernel/sync/set_once.rs line 19
Quoted 11 lines
rust/kernel/sync/set_once.rs +1
-1@@ -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.
[ ... ]
rust/kernel/sync/set_once.rs pub fn copy(&self) -> Option<T> · line 117
Quoted 9 lines
rust/kernel/sync/set_once.rs +5
-0@@ -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?
Quoted 14 lines
rust/kernel/sync/set_once.rs +14
-0+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
[ ... ]
Quoted 1 line
+ 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?
rust/kernel/sync/set_once.rs pub fn copy(&self) -> Option<T> · line 160
Quoted 11 lines
rust/kernel/sync/set_once.rs +11
-0+// 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.
Quoted 3 lines
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?
Quoted 11 lines
rust/kernel/sync/set_once.rs +2
-7-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 () }; } }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Alice Ryhl
maintainerRe: [PATCH v17 5/6] rust: sync: Add SetOnce::reset()
In reply to Lyude PaulQuoted 6 lines · Lyude Paul
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.
Signed-off-by Lyude Paul <[email protected]>
This isn't needed, you can just do:
unsafe { (*this).sgt_res = SetOnce::new() };
[email protected]
reviewerRe: [PATCH v17 5/6] rust: sync: Add SetOnce::reset()
Quoted 9 lines · Alice Ryhl
Quoted 8 lines · Lyude Paul
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.
Quoted 5 lines
Quoted 1 line
Signed-off-by Lyude Paul <[email protected]>This isn't needed, you can just do:
unsafe { (*this).sgt_res =
SetOnce::new()};
I realized after sending this I could also just be using ManuallyDrop here instead of adding a reset function, which might be a bit clearer anyhow
Lyude Paul
reviewerPATCH v17 6/6 rust: drm: gem: Introduce shmem::Object::sg_table()
In reply to Lyude PaulIn order to do this, we need to be careful to ensure that any interface we expose for scatterlists ensures that any mappings created from one are destroyed on driver-unbind. To do this, we introduce a Devres resource into shmem::Object that we use in order to ensure that we release any SGTable mappings on driver-unbind.
There's some other slightly unfortunate caveats of this:
* Drivers don't have explicit control at the moment over when unmapping happens (which is exactly the same as the C side atm, so it might not be a problem).
* We can't just return SGTableMap to the user through an Arc to attempt to fix the last caveat - because that implies the gem object would need to hold a reference count to the scatterlist mapping, which just leaves us with the same problem.
---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the SGTable abstractions now has a Owned and Borrowed variant, I think renaming this to shmem::SGTable makes things less confusing. We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous ret = in shmem::Object::get_sg_table() V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
+ SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to sg_table().
* Implement Deborah's suggestion to fix double-free in free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before) using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment V16:
* Get rid of LazyInit, go back to SetOnce, use trick that Alice recommended that is a lot cleaner.
* Fix horrid rebasing mistake
V17:
* Rebase
* Fix missing safety comment in free_callback() (we forgot to justify why &mut is safe in unsafe { &mut (*this).sgt_res }.reset())
rust/kernel/drm/gem/shmem.rs | 166 +++++++++++++++++++++++++++++++++-- 1 file changed, 157 insertions(+), 9 deletions(-)
rust/kernel/drm/gem/shmem.rs +157 -9
@@ -11,6 +11,11 @@use crate ::{ container_of, + device::{ + self, +Bound ,// + }, + devres::*, drm::{ driver, gem,
@@ -19,14 +24,23 @@ DeviceContext, Registered, // }, - error::to_result, + error::{ + from_err_ptr, + to_result, // + }, io::{ Io, IoCapable, IoKnownSize, // }, prelude::*, - sync::aref::ARef, + scatterlist, + sync::{ + aref::ARef, + new_mutex, + Mutex, + SetOnce, // + }, types::Opaque, // }; use core::{
@@ -87,6 +101,11 @@ pub struct Object<T: DriverObject, C: DeviceContext = Registered> { obj: Opaque<bindings::drm_gem_shmem_object>, /// Parent object that owns this object's DMA reservation object. parent_resv_obj: Option<ARef<Object<T, C>>>, + /// Devres object for unmapping any SGTable on driver-unbind. + sgt_res: SetOnce<Devres<SGTableMap<T, C>>>, +#[pin] +/// Lock for protecting initialization of `sgt_res`. + sgt_lock: Mutex<()>,#[pin] inner: T, _ctx: PhantomData<C>,
@@ -145,6 +164,8 @@ pub fn new(try_pin_init! (Self { obj <-Opaque ::init_zeroed (), parent_resv_obj: config.parent_resv_obj.map (|p| p.into ()), + sgt_res:SetOnce ::new (), + sgt_lock <-new_mutex! (()), inner <-T ::new (dev, size, args), _ctx:PhantomData ::<C >, }),
@@ -189,18 +210,25 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {// - DRM always passes a valid gem object here // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that // `obj` is contained within a drm_gem_shmem_object -let this =unsafe {container_of! (obj, bindings::drm_gem_shmem_object, base) }; - -// SAFETY: -// - We're in free_callback - so this function is safe to call. -// - We won't be using the gem resources on `this` after this call. -unsafe { bindings::drm_gem_shmem_release (this) }; +let base =unsafe {container_of! (obj, bindings::drm_gem_shmem_object, base) };// SAFETY: // - We verified above that `obj` is valid, which makes `this` valid // - This function is set in AllocOps, so we know that `this` is contained within a // `Object<T, C>` -let this =unsafe {container_of! (Opaque ::cast_from (this),Self , obj) }.cast_mut (); +let this =unsafe {container_of! (Opaque ::cast_from (base),Self , obj) }.cast_mut (); + +// We need to drop `sgt_res` first, since doing so requires that the GEM object is still +// alive. +// SAFETY: +// - We verified above that `this` is valid. +// - We are in free_callback, guaranteeing we have exclusive access to `this`. +unsafe { &mut (*this).sgt_res }.reset (); + +// SAFETY: +// - We're in free_callback - so this function is safe to call. +// - We won't be using the gem resources on `this` after this call. +unsafe { bindings::drm_gem_shmem_release (base) };// SAFETY: We're recovering the Kbox<> we created in gem_create_object() let _ =unsafe {KBox ::from_raw (this) };
@@ -279,6 +307,45 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {pub fn owned_vmap <const SIZE :usize >(&self) ->Result <VMapOwned <T ,C ,SIZE >> { self.make_vmap () } + +/// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA +/// pages for this object. +/// +/// This will pin the object in memory. It is expected that `dev` should be a pointer to the +/// same [`device::Device`] which `self` belongs to, otherwise this function will return +/// `Err(EINVAL)`. +pub fn sg_table <'a>( + &'a self, + dev: &'a device::Device <Bound >, + ) ->Result <&'a scatterlist::SGTable > { +if dev.as_raw () != self.dev ().as_ref ().as_raw () { +return Err (EINVAL ); + } + +let sgt_res = 'out: { +// Fast path: sgt_res is already initialized +if let Some (sgt_res) = self.sgt_res.as_ref () { +break 'out sgt_res; + } + +// Slow path: Grab the lock and see if we need to initialize sgt_res. +let _guard = self.sgt_lock.lock (); + +// If someone initialized it while we were waiting, we can exit early. +if let Some (sgt_res) = self.sgt_res.as_ref () { +break 'out sgt_res; + } + +// If not, finish initializing and return. + self.sgt_res + .populate (Devres ::new (dev,SGTableMap ::new (self))?); + +// SAFETY: We just populated sgt_res above. +unsafe { self.sgt_res.as_ref ().unwrap_unchecked () } + }; + +Ok (sgt_res.access (dev)?) + } }impl <T :DriverObject ,C :DeviceContext >Deref for Object <T ,C > {
@@ -474,6 +541,63 @@ impl<D, R, C, const SIZE: usize> IoKnownSize for VMap<D, R, C, SIZE>#[cfg( CONFIG_64BIT )] impl_vmap_io_capable! (VMap ,u64 ); +/// A reference to a GEM object that is known to have a mapped [`SGTable`]. +/// +/// This is used by the Rust bindings with [`Devres`] in order to ensure that mappings for SGTables +/// on GEM shmem objects are revoked on driver-unbind. +/// +/// # Invariants +/// +/// - `self.obj` always points to a valid GEM object. +/// - This object is proof that `self.obj.owner.sgt` has an initialized and valid +/// [`scatterlist::SGTable`]. +pub struct SGTableMap <T :DriverObject ,C :DeviceContext > { + obj:NonNull <Object <T ,C >>, +} + +impl <T :DriverObject ,C :DeviceContext >Deref for SGTableMap <T ,C > { +type Target = scatterlist::SGTable ; + +fn deref (&self) -> &Self ::Target { +// SAFETY: +// - The NonNull is guaranteed to be valid via our type invariants. +// - The sgt field is guaranteed to be initialized and valid via our type invariants. +unsafe { scatterlist::SGTable ::from_raw ((*self.obj.as_ref ().as_raw_shmem ()).sgt) } + } +} + +impl <T :DriverObject ,C :DeviceContext >Drop for SGTableMap <T ,C > { +fn drop (&mut self) { +// SAFETY: `obj` is always valid via our type invariants +let obj =unsafe { self.obj.as_ref () }; +let _lock =DmaResvGuard ::new (obj); + +// SAFETY: We acquired the lock needed for calling this function above +unsafe { bindings::__drm_gem_shmem_free_sgt_locked (obj.as_raw_shmem ()) }; + } +} + +impl <T :DriverObject ,C :DeviceContext >SGTableMap <T ,C > { +fn new (obj: &Object <T ,C >) ->impl Init <Self ,Error > { +// INVARIANT: +// - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it +// succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is +// initialized. +// SAFETY: +// - `obj` is fully initialized, making this function safe to call. +from_err_ptr (unsafe { bindings::drm_gem_shmem_get_pages_sgt (obj.as_raw_shmem ()) })?; + +Ok (Self { obj: obj.into () }) + } +} + +// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object +// it points to is guaranteed to be thread-safe. +unsafe impl <T :DriverObject ,C :DeviceContext >Send for SGTableMap <T ,C > {} +// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object +// it points to is guaranteed to be thread-safe. +unsafe impl <T :DriverObject ,C :DeviceContext >Sync for SGTableMap <T ,C > {} +#[kunit_tests(rust_drm_gem_shmem)] mod tests {use super ::*;
@@ -582,4 +706,28 @@ fn vmap_io() -> Result {Ok (()) } + +// TODO: I would love to actually test the success paths of sg_table(), but that would require +// also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave +// that for someone else. + +// Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it +// skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device +// bound to it. +#[test] +fn fail_sg_table_on_wrong_dev () ->Result { +let (_dev, drm) =create_drm_dev ()?; +let wrong_dev = faux::Registration ::new (c"EvilKunit" ,None )?; + +let obj =Object ::<KunitObject ,_ >::new (&drm,PAGE_SIZE ,ObjectConfig ::default (), ())?; + +assert_eq! (obj.sg_table (wrong_dev.as_ref ()).err ().unwrap (),EINVAL ); + +// If sgt_res was not initialized mistakenly with the wrong device, this should still fail. +assert_eq! (obj.sg_table (wrong_dev.as_ref ()).err ().unwrap (),EINVAL ); + +// TODO: Someday, we should test that creating an sg_table here still succeeds. + +Ok (()) + } }
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v17 6/6] rust: drm: gem: Introduce shmem::Object::sg_table()
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] `SGTableMap::drop` unconditionally frees the scatter/gather table, which corrupts memory for…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] SGTableMap::drop unconditionally frees the scatter/gather table, which corrupts memory for imported dma-buf GEM objects.
- [Critical] Concurrent device unbind and GEM object destruction causes a Use-After-Free of the GEM object and its dma_resv lock in SGTableMap::drop.
- [High] SGTableMap fails to pin the shmem GEM object backing pages, exposing the SG table to being purged by the shrinker and causing a NULL pointer dereference. It also leaks the pages_use_count on drop. --
commit 8870abbe2654a7715fcd51335a14b62f57e22680
Author: Lyude Paul <[email protected]>
rust: drm: gem: Introduce shmem::Object::sg_table()
This commit introduces a Devres-managed SGTableMap to ensure that scatterlist mappings for GEM shmem objects are released upon driver unbind. It adds the sg_table() method to shmem::Object which is intended to pin the object and return its scatter/gather table.
Quoted 4 lines
rust/kernel/drm/gem/shmem.rs +0
-0
[ ... ]
Quoted 15 lines
rust/kernel/drm/gem/shmem.rs +1
-6@@ -189,18 +210,25 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {// - DRM always passes a valid gem object here // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that // `obj` is contained within a drm_gem_shmem_object -let this =unsafe {container_of! (obj, bindings::drm_gem_shmem_object, base) }; - -// SAFETY: -// - We're in free_callback - so this function is safe to call. -// - We won't be using the gem resources on `this` after this call. -unsafe { bindings::drm_gem_shmem_release (this) }; +let base =unsafe {container_of! (obj, bindings::drm_gem_shmem_object, base) };// SAFETY: // - We verified above that `obj` is valid, which makes `this` valid // - This function is set in AllocOps, so we know that `this` is contained within a
rust/kernel/drm/gem/shmem.rs extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) { · line 226
Quoted 10 lines
rust/kernel/drm/gem/shmem.rs +8
-1// `Object<T, C>` -let this =unsafe {container_of! (Opaque ::cast_from (this),Self , obj) }.cast_mut (); +let this =unsafe {container_of! (Opaque ::cast_from (base),Self , obj) }.cast_mut (); + +// We need to drop `sgt_res` first, since doing so requires that the GEM object is still +// alive. +// SAFETY: +// - We verified above that `this` is valid. +// - We are in free_callback, guaranteeing we have exclusive access to `this`. +unsafe { &mut (*this).sgt_res }.reset ();
[Severity: Critical]
Does this leave a window for a use-after-free if the device unbinds concurrently?
If the devres subsystem is already running SGTableMap::drop() in another thread during device unbind, sgt_res.reset() could silently fail to remove the action. The callback would then proceed to free the GEM object while SGTableMap::drop() is still dereferencing and locking it.
Quoted 5 lines
rust/kernel/drm/gem/shmem.rs +5
-0+ +// SAFETY: +// - We're in free_callback - so this function is safe to call. +// - We won't be using the gem resources on `this` after this call. +unsafe { bindings::drm_gem_shmem_release (base) };
[ ... ]
rust/kernel/drm/gem/shmem.rs impl<D, R, C, const SIZE: usize> IoKnownSize for VMap<D, R, C, SIZE> · line 576
Quoted 8 lines
rust/kernel/drm/gem/shmem.rs +8
-0+impl <T :DriverObject ,C :DeviceContext >Drop for SGTableMap <T ,C > { +fn drop (&mut self) { +// SAFETY: `obj` is always valid via our type invariants +let obj =unsafe { self.obj.as_ref () }; +let _lock =DmaResvGuard ::new (obj); + +// SAFETY: We acquired the lock needed for calling this function above +unsafe { bindings::__drm_gem_shmem_free_sgt_locked (obj.as_raw_shmem ()) };
[Severity: Critical]
Is it safe to unconditionally free the scatter/gather table here?
If the GEM object was imported from a dma-buf, the scatter/gather table belongs to the exporter. Calling __drm_gem_shmem_free_sgt_locked() directly without checking drm_gem_is_imported() seems like it might illegally free the exporter's memory.
[Severity: High]
Could this drop implementation also leak the pages_use_count?
Calling __drm_gem_shmem_free_sgt_locked() alone doesn't seem to drop the page use count acquired during SGTableMap::new(). Should this also call drm_gem_shmem_put_pages_locked() to balance the counts?
rust/kernel/drm/gem/shmem.rs impl<D, R, C, const SIZE: usize> IoKnownSize for VMap<D, R, C, SIZE> · line 588
Quoted 12 lines
rust/kernel/drm/gem/shmem.rs +12
-0+ } +} + +impl <T :DriverObject ,C :DeviceContext >SGTableMap <T ,C > { +fn new (obj: &Object <T ,C >) ->impl Init <Self ,Error > { +// INVARIANT: +// - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it +// succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is +// initialized. +// SAFETY: +// - `obj` is fully initialized, making this function safe to call. +from_err_ptr (unsafe { bindings::drm_gem_shmem_get_pages_sgt (obj.as_raw_shmem ()) })?;
[Severity: High]
Does this expose the object to being purged by the DRM LRU shrinker?
The Object::sg_table() documentation mentions this will pin the object in memory, but drm_gem_shmem_get_pages_sgt() only increments the pages_use_count, not the pages_pin_count. If userspace marks the object as MADV_DONTNEED, the shrinker could purge it and set shmem->sgt to NULL.
Could this lead to a NULL pointer dereference later in SGTableMap::drop() when it passes the pointer to __drm_gem_shmem_free_sgt_locked()?
Quoted 4 lines
rust/kernel/drm/gem/shmem.rs +4
-0+ +Ok (Self { obj: obj.into () }) + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6