Re: [PATCH v19 2/4] rust: drm: gem: shmem: Add vmap functions

[sashiko-bot@kernel.org]

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 <lyude@redhat.com>

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/20260608183057.2001376-1-lyude@redhat.com?part=2