Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] The `vmap_io` KUnit test assumes little-endian byte ordering, causing the test to fail on big-endian architectures. - [Medium] The `VMap` struct lacks explicit `Send` and `Sync` implementations, preventing users from safely sharing or transferring owned mappings across threads. - [Low] The `VMap` struct is constructed without an `// INVARIANT:` comment explaining how its pointer validity invariant is satisfied. --
commit ceea14411fbb1d31f384d7a4decb9b7c0d4302da Author: Lyude Paul <[email protected]> rust: drm: gem: shmem: Add vmap functions This commit adds virtual memory mapping capabilities for GEM shmem objects to the Rust DRM bindings. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index 79ebd746e9611..e0bd0695ba1f8 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs [ ... ] > @@ -191,6 +199,77 @@ 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>> [ ... ] > + // 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. > + 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(), > + }) [Severity: Low] this isn't a bug, but should there be an // INVARIANT: comment here? According to the Rust subsystem guidelines, when a struct with an Invariants documentation section is constructed, the code must have a comment explaining why the invariants are satisfied. The pointer validity invariant doesn't appear to be documented during this instantiation. > + } > + } [ ... ] > @@ -270,3 +348,238 @@ fn drop(&mut self) { > +/// 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, > +} [Severity: Medium] Should VMap explicitly implement Send and Sync? Since the addr field is a raw pointer, VMap and its aliases will default to being !Send and !Sync. Would this prevent users from safely passing owned mappings across threads or into multithreaded workqueues? [ ... ] > + #[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(0xFEDCBA98, 0x20); > + > + assert_eq!(vmap.read32(0x20), 0xFEDCBA98); > + > + assert_eq!(vmap.read8(0x20), 0x98); > + assert_eq!(vmap.read8(0x21), 0xBA); > + assert_eq!(vmap.read8(0x22), 0xDC); > + assert_eq!(vmap.read8(0x23), 0xFE); [Severity: Medium] Does this test assume little-endian byte ordering? The test writes a 32-bit integer (0xFEDCBA98) using native byte ordering, but the byte-by-byte assertions check that the first byte at the lowest address is 0x98. On big-endian architectures, wouldn't the most significant byte (0xFE) be stored at the lowest address, causing this test to fail? > + > + Ok(()) > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
