Skip to contentSkip to conversation

Patch "rust: dma: use pointer projection infra for `dma_{read,write}` macro" has been added to the 6.19-stable tree

DISCUSSION Gary Guo 3 participants 4 messages Mar 15 – Mar 16 1 message not in the archive Stable branch

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Gary Guo

author
#1 lore raw
Quoted 10 lines · Sasha Levin

This is a note to let you know that I've just added the patch titled

rust: dma: use pointer projection infra for dma_{read,write} macro

to the 6.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
rust-dma-use-pointer-projection-infra-for-dma_-read-.patch
and it can be found in the queue-6.19 subdirectory.

Hi Sasha,

commit 08da98f18f4f ("rust: ptr: add KnownSize trait to support DST size info extraction") and commit f41941aab3ac ("rust: ptr: add projection infrastructure") are dependencies of this fix.

It doesn't look like these commits are currently being picked. They're needed for building.

They're part of the same series: https://lore.kernel.org/rust-for-linux/[email protected]/

Best,
Gary

Quoted 345 lines

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

commit 26dd613c37a79884da7fd824263d4c93123688c3

Author: Gary Guo <[email protected]>
Date:   Mon Mar 2 16:42:36 2026 +0000

rust: dma: use pointer projection infra for dma_{read,write} macro

[ Upstream commit 4da879a0d3fd170a70994b73baa554c6913918b5 ]

Current dma_read!, dma_write! macros also use a custom addr_of!()-based implementation for projecting pointers, which has soundness issue as it relies on absence of Deref implementation on types. It also has a soundness issue where it does not protect against unaligned fields (when #[repr(packed)] is used) so it can generate misaligned accesses.

This commit migrates them to use the general pointer projection infrastructure, which handles these cases correctly.

As part of migration, the macro is updated to have an improved surface syntax. The current macro have

dma_read!(a.b.c[d].e.f)

to mean a.b.c is a DMA coherent allocation and it should project into it with [d].e.f and do a read, which is confusing as it makes the indexing operator integral to the macro (so it will break if you have an array of CoherentAllocation, for example).

This also is problematic as we would like to generalize CoherentAllocation from just slices to arbitrary types.

Make the macro expects dma_read!(path.to.dma, .path.inside.dma) as the canonical syntax. The index operator is no longer special and is just one type of projection (in additional to field projection). Similarly, make dma_write!(path.to.dma, .path.inside.dma, value) become the canonical syntax for writing.

Another issue of the current macro is that it is always fallible. This makes sense with existing design of CoherentAllocation, but once we support fixed size arrays with CoherentAllocation, it is desirable to have the ability to perform infallible indexing as well, e.g. doing a [0] index of [Foo; 2] is okay and can be checked at build-time, so forcing falliblity is non-ideal. To capture this, the macro is changed to use [idx] as infallible projection and [idx]? as fallible index projection (those syntax are part of the general projection infra). A benefit of this is that while individual indexing operation may fail, the overall read/write operation is not fallible.

Fixes ad2907b4e308 ("rust: add dma coherent allocator abstraction")

[ Capitalize safety comments; slightly improve wording in doc-comments.
- Danilo ]

drivers/gpu/nova-core/gsp.rs +7 -7
index 174feaca0a6b9..25cd48514c777 100644
@@ -143,14 +143,14 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                     // _kgspInitLibosLoggingStructures (allocates memory for buffers)
                     // kgspSetupLibosInitArgs_IMPL (creates pLibosInitArgs[] array)
                     dma_write!(
-                        libos[0] = LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0)
-                    )?;
+                        libos, [0]?, LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0)
+                    );
                     dma_write!(
-                        libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
-                    )?;
-                    dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?;
-                    dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(cmdq))?;
-                    dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
+                        libos, [1]?, LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
+                    );
+                    dma_write!(libos, [2]?, LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0));
+                    dma_write!(rmargs, [0]?.inner, fw::GspArgumentsCached::new(cmdq));
+                    dma_write!(libos, [3]?, LibosMemoryRegionInitArgument::new("RMARGS", rmargs));
                 },
             }))
         })
drivers/gpu/nova-core/gsp/boot.rs +1 -1
index 54937606b5b0a..38b4682ff01be 100644
@@ -160,7 +160,7 @@ pub(crate) fn boot(
 
         let wpr_meta =
             CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
-        dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
+        dma_write!(wpr_meta, [0]?, GspFwWprMeta::new(&gsp_fw, &fb_layout));
 
         self.cmdq
             .send_command(bar, commands::SetSystemInfo::new(pdev))?;
drivers/gpu/nova-core/gsp/cmdq.rs +7 -3
index 3991ccc0c10f1..1cdd1ccfe5702 100644
@@ -201,9 +201,13 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
 
         let gsp_mem =
             CoherentAllocation::<GspMem>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
-        dma_write!(gsp_mem[0].ptes = PteArray::new(gsp_mem.dma_handle())?)?;
-        dma_write!(gsp_mem[0].cpuq.tx = MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES))?;
-        dma_write!(gsp_mem[0].cpuq.rx = MsgqRxHeader::new())?;
+        dma_write!(gsp_mem, [0]?.ptes, PteArray::new(gsp_mem.dma_handle())?);
+        dma_write!(
+            gsp_mem,
+            [0]?.cpuq.tx,
+            MsgqTxHeader::new(MSGQ_SIZE, RX_HDR_OFF, MSGQ_NUM_PAGES)
+        );
+        dma_write!(gsp_mem, [0]?.cpuq.rx, MsgqRxHeader::new());
 
         Ok(Self(gsp_mem))
     }
rust/kernel/dma.rs +50 -64
index acc65b1e0f245..37e125bb423ad 100644
@@ -444,6 +444,19 @@ pub fn size(&self) -> usize {
         self.count * core::mem::size_of::<T>()
     }
 
+    /// Returns the raw pointer to the allocated region in the CPU's virtual address space.
+    #[inline]
+    pub fn as_ptr(&self) -> *const [T] {
+        core::ptr::slice_from_raw_parts(self.cpu_addr.as_ptr(), self.count)
+    }
+
+    /// Returns the raw pointer to the allocated region in the CPU's virtual address space as
+    /// a mutable pointer.
+    #[inline]
+    pub fn as_mut_ptr(&self) -> *mut [T] {
+        core::ptr::slice_from_raw_parts_mut(self.cpu_addr.as_ptr(), self.count)
+    }
+
     /// Returns the base address to the allocated region in the CPU's virtual address space.
     pub fn start_ptr(&self) -> *const T {
         self.cpu_addr.as_ptr()
@@ -564,23 +577,6 @@ pub unsafe fn write(&mut self, src: &[T], offset: usize) -> Result {
         Ok(())
     }
 
-    /// Returns a pointer to an element from the region with bounds checking. `offset` is in
-    /// units of `T`, not the number of bytes.
-    ///
-    /// Public but hidden since it should only be used from [`dma_read`] and [`dma_write`] macros.
-    #[doc(hidden)]
-    pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
-        if offset >= self.count {
-            return Err(EINVAL);
-        }
-        // SAFETY:
-        // - The pointer is valid due to type invariant on `CoherentAllocation`
-        // and we've just checked that the range and index is within bounds.
-        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
-        // that `self.count` won't overflow early in the constructor.
-        Ok(unsafe { self.cpu_addr.as_ptr().add(offset) })
-    }
-
     /// Reads the value of `field` and ensures that its type is [`FromBytes`].
     ///
     /// # Safety
@@ -653,6 +649,9 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
 
 /// Reads a field of an item from an allocated region of structs.
 ///
+/// The syntax is of the form `kernel::dma_read!(dma, proj)` where `dma` is an expression evaluating
+/// to a [`CoherentAllocation`] and `proj` is a [projection specification](kernel::ptr::project!).
+///
 /// # Examples
 ///
 /// ```
@@ -667,36 +666,29 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
 /// unsafe impl kernel::transmute::AsBytes for MyStruct{};
 ///
 /// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
-/// let whole = kernel::dma_read!(alloc[2]);
-/// let field = kernel::dma_read!(alloc[1].field);
+/// let whole = kernel::dma_read!(alloc, [2]?);
+/// let field = kernel::dma_read!(alloc, [1]?.field);
 /// # Ok::<(), Error>(()) }
 /// ```
 #[macro_export]
 macro_rules! dma_read {
-    ($dma:expr, $idx: expr, $($field:tt)*) => {{
-        (|| -> ::core::result::Result<_, $crate::error::Error> {
-            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-            // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
-            // dereferenced. The compiler also further validates the expression on whether `field`
-            // is a member of `item` when expanded by the macro.
-            unsafe {
-                let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
-                ::core::result::Result::Ok(
-                    $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
-                )
-            }
-        })()
+    ($dma:expr, $($proj:tt)*) => {{
+        let dma = &$dma;
+        let ptr = $crate::ptr::project!(
+            $crate::dma::CoherentAllocation::as_ptr(dma), $($proj)*
+        );
+        // SAFETY: The pointer created by the projection is within the DMA region.
+        unsafe { $crate::dma::CoherentAllocation::field_read(dma, ptr) }
     }};
-    ($dma:ident [ $idx:expr ] $($field:tt)* ) => {
-        $crate::dma_read!($dma, $idx, $($field)*)
-    };
-    ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
-        $crate::dma_read!($($dma).*, $idx, $($field)*)
-    };
 }
 
 /// Writes to a field of an item from an allocated region of structs.
 ///
+/// The syntax is of the form `kernel::dma_write!(dma, proj, val)` where `dma` is an expression
+/// evaluating to a [`CoherentAllocation`], `proj` is a
+/// [projection specification](kernel::ptr::project!), and `val` is the value to be written to the
+/// projected location.
+///
 /// # Examples
 ///
 /// ```
@@ -711,37 +703,31 @@ macro_rules! dma_read {
 /// unsafe impl kernel::transmute::AsBytes for MyStruct{};
 ///
 /// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
-/// kernel::dma_write!(alloc[2].member = 0xf);
-/// kernel::dma_write!(alloc[1] = MyStruct { member: 0xf });
+/// kernel::dma_write!(alloc, [2]?.member, 0xf);
+/// kernel::dma_write!(alloc, [1]?, MyStruct { member: 0xf });
 /// # Ok::<(), Error>(()) }
 /// ```
 #[macro_export]
 macro_rules! dma_write {
-    ($dma:ident [ $idx:expr ] $($field:tt)*) => {{
-        $crate::dma_write!($dma, $idx, $($field)*)
-    }};
-    ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
-        $crate::dma_write!($($dma).*, $idx, $($field)*)
+    (@parse [$dma:expr] [$($proj:tt)*] [, $val:expr]) => {{
+        let dma = &$dma;
+        let ptr = $crate::ptr::project!(
+            mut $crate::dma::CoherentAllocation::as_mut_ptr(dma), $($proj)*
+        );
+        let val = $val;
+        // SAFETY: The pointer created by the projection is within the DMA region.
+        unsafe { $crate::dma::CoherentAllocation::field_write(dma, ptr, val) }
     }};
-    ($dma:expr, $idx: expr, = $val:expr) => {
-        (|| -> ::core::result::Result<_, $crate::error::Error> {
-            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-            // SAFETY: `item_from_index` ensures that `item` is always a valid item.
-            unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
-            ::core::result::Result::Ok(())
-        })()
+    (@parse [$dma:expr] [$($proj:tt)*] [.$field:tt $($rest:tt)*]) => {
+        $crate::dma_write!(@parse [$dma] [$($proj)* .$field] [$($rest)*])
+    };
+    (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr]? $($rest:tt)*]) => {
+        $crate::dma_write!(@parse [$dma] [$($proj)* [$index]?] [$($rest)*])
+    };
+    (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr] $($rest:tt)*]) => {
+        $crate::dma_write!(@parse [$dma] [$($proj)* [$index]] [$($rest)*])
     };
-    ($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
-        (|| -> ::core::result::Result<_, $crate::error::Error> {
-            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-            // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
-            // dereferenced. The compiler also further validates the expression on whether `field`
-            // is a member of `item` when expanded by the macro.
-            unsafe {
-                let ptr_field = ::core::ptr::addr_of_mut!((*item) $(.$field)*);
-                $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
-            }
-            ::core::result::Result::Ok(())
-        })()
+    ($dma:expr, $($rest:tt)*) => {
+        $crate::dma_write!(@parse [$dma] [] [$($rest)*])
     };
 }
samples/rust/rust_dma.rs +16 -14
index f53bce2a73e3b..bcba1a6e6aaf4 100644
@@ -68,7 +68,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
                 CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
 
             for (i, value) in TEST_VALUES.into_iter().enumerate() {
-                kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
+                kernel::dma_write!(ca, [i]?, MyStruct::new(value.0, value.1));
             }
 
             let size = 4 * page::PAGE_SIZE;
@@ -85,6 +85,20 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
     }
 }
 
+impl DmaSampleDriver {
+    fn check_dma(&self) -> Result {
+        for (i, value) in TEST_VALUES.into_iter().enumerate() {
+            let val0 = kernel::dma_read!(self.ca, [i]?.h);
+            let val1 = kernel::dma_read!(self.ca, [i]?.b);
+
+            assert_eq!(val0, value.0);
+            assert_eq!(val1, value.1);
+        }
+
+        Ok(())
+    }
+}
+
 #[pinned_drop]
 impl PinnedDrop for DmaSampleDriver {
     fn drop(self: Pin<&mut Self>) {
@@ -92,19 +106,7 @@ fn drop(self: Pin<&mut Self>) {
 
         dev_info!(dev, "Unload DMA test driver.\n");
 
-        for (i, value) in TEST_VALUES.into_iter().enumerate() {
-            let val0 = kernel::dma_read!(self.ca[i].h);
-            let val1 = kernel::dma_read!(self.ca[i].b);
-            assert!(val0.is_ok());
-            assert!(val1.is_ok());
-
-            if let Ok(val0) = val0 {
-                assert_eq!(val0, value.0);
-            }
-            if let Ok(val1) = val1 {
-                assert_eq!(val1, value.1);
-            }
-        }
+        assert!(self.check_dma().is_ok());
 
         for (i, entry) in self.sgt.iter().enumerate() {
             dev_info!(dev, "Entry[{}]: DMA address: {:#x}", i, entry.dma_address());
maintainer
#2 lore raw
Quoted 14 lines · Gary Guo
Quoted 10 lines · Sasha Levin

This is a note to let you know that I've just added the patch titled

rust: dma: use pointer projection infra for dma_{read,write} macro

to the 6.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
rust-dma-use-pointer-projection-infra-for-dma_-read-.patch
and it can be found in the queue-6.19 subdirectory.

Hi Sasha,

Quoted 8 lines

commit 08da98f18f4f ("rust: ptr: add KnownSize trait to support DST size info extraction") and commit f41941aab3ac ("rust: ptr: add projection infrastructure") are dependencies of this fix.

It doesn't look like these commits are currently being picked. They're needed for building.

They're part of the same series: https://lore.kernel.org/rust-for-linux/[email protected]/

Yeah, this breaks the build on my systems. I'll go drop this patch for now, and if you want these in the stable trees, can you provide a backported series of them?

thanks,

greg k-h

#3 lore raw
Quoted 19 lines · Greg KH
Quoted 18 lines · Gary Guo
Quoted 10 lines · Sasha Levin

This is a note to let you know that I've just added the patch titled

rust: dma: use pointer projection infra for dma_{read,write} macro

to the 6.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
rust-dma-use-pointer-projection-infra-for-dma_-read-.patch
and it can be found in the queue-6.19 subdirectory.

Hi Sasha,

commit 08da98f18f4f ("rust: ptr: add KnownSize trait to support DST size info extraction") and commit f41941aab3ac ("rust: ptr: add projection infrastructure") are dependencies of this fix.

Quoted 8 lines
Quoted 4 lines

It doesn't look like these commits are currently being picked. They're needed for building.

They're part of the same series: https://lore.kernel.org/rust-for-linux/[email protected]/

Yeah, this breaks the build on my systems. I'll go drop this patch for now, and if you want these in the stable trees, can you provide a backported series of them?

The DMA soundness fix, as a potential bug, is probably not crucial to backport.

There are two nova-core fixes on top of it. One of them fixes a UB condition reading from and writing to a DMA buffer, the other one is a potential stack overflow.

nova-core is still work in progress, so I'm not too worried about this as far as stable trees are concerned.

I think we usually backport soundness fixes anyway, plus everything should apply without conflicts, so it probably doesn't hurt to pick them up regardless.

I can send a separate series if preferred.

maintainer
#4 lore raw
Quoted 35 lines · Danilo Krummrich
Quoted 19 lines · Greg KH
Quoted 18 lines · Gary Guo
Quoted 10 lines · Sasha Levin

This is a note to let you know that I've just added the patch titled

rust: dma: use pointer projection infra for dma_{read,write} macro

to the 6.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
rust-dma-use-pointer-projection-infra-for-dma_-read-.patch
and it can be found in the queue-6.19 subdirectory.

Hi Sasha,

commit 08da98f18f4f ("rust: ptr: add KnownSize trait to support DST size info extraction") and commit f41941aab3ac ("rust: ptr: add projection infrastructure") are dependencies of this fix.

Quoted 8 lines
Quoted 4 lines

It doesn't look like these commits are currently being picked. They're needed for building.

They're part of the same series: https://lore.kernel.org/rust-for-linux/[email protected]/

Yeah, this breaks the build on my systems. I'll go drop this patch for now, and if you want these in the stable trees, can you provide a backported series of them?

The DMA soundness fix, as a potential bug, is probably not crucial to backport.

There are two nova-core fixes on top of it. One of them fixes a UB condition reading from and writing to a DMA buffer, the other one is a potential stack overflow.

Quoted 7 lines

nova-core is still work in progress, so I'm not too worried about this as far as stable trees are concerned.

I think we usually backport soundness fixes anyway, plus everything should apply without conflicts, so it probably doesn't hurt to pick them up regardless.

I can send a separate series if preferred.

I've dropped all the nova-core patches from the queue as well. If you think they are needed in 6.19.y, please send backports.

thanks,

greg k-h