[PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

The DmaGspMem pointer accessor methods (gsp_write_ptr, gsp_read_ptr,
cpu_read_ptr, cpu_write_ptr, advance_cpu_read_ptr,
advance_cpu_write_ptr) dereference a raw pointer to DMA memory, creating
an intermediate reference before calling volatile read/write methods.

This is undefined behavior since DMA memory can be concurrently modified
by the device.

Fix this by moving the implementations into a gsp_mem module in fw.rs
that uses the dma_read!() / dma_write!() macros, making the original
methods on DmaGspMem thin forwarding wrappers.

An alternative approach would have been to wrap the shared memory in
Opaque, but that would have required even more unsafe code.

Since the gsp_mem module lives in fw.rs (to access firmware-specific
binding field names), GspMem, Msgq, DmaGspMem and their relevant fields
are temporarily widened to pub(in crate::gsp). This will be reverted
once IoView projections are available.

Cc: Gary Guo <gary@garyguo.net>
Closes: https://lore.kernel.org/nouveau/DGUT14ILG35P.1UMNRKU93JUM1@kernel.org/
Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/nova-core/gsp/cmdq.rs |  71 +++++----------------
 drivers/gpu/nova-core/gsp/fw.rs   | 101 ++++++++++++++++++++----------
 2 files changed, 84 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index ae54708c38eb..00a4062a47dc 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -2,11 +2,7 @@
 
 use core::{
     cmp,
-    mem,
-    sync::atomic::{
-        fence,
-        Ordering, //
-    }, //
+    mem, //
 };
 
 use kernel::{
@@ -146,30 +142,32 @@ struct MsgqData {
 #[repr(C)]
 // There is no struct defined for this in the open-gpu-kernel-source headers.
 // Instead it is defined by code in `GspMsgQueuesInit()`.
-struct Msgq {
+// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
+pub(in crate::gsp) struct Msgq {
     /// Header for sending messages, including the write pointer.
-    tx: MsgqTxHeader,
+    pub(in crate::gsp) tx: MsgqTxHeader,
     /// Header for receiving messages, including the read pointer.
-    rx: MsgqRxHeader,
+    pub(in crate::gsp) rx: MsgqRxHeader,
     /// The message queue proper.
     msgq: MsgqData,
 }
 
 /// Structure shared between the driver and the GSP and containing the command and message queues.
 #[repr(C)]
-struct GspMem {
+// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
+pub(in crate::gsp) struct GspMem {
     /// Self-mapping page table entries.
     ptes: PteArray<{ GSP_PAGE_SIZE / size_of::<u64>() }>,
     /// CPU queue: the driver writes commands here, and the GSP reads them. It also contains the
     /// write and read pointers that the CPU updates.
     ///
     /// This member is read-only for the GSP.
-    cpuq: Msgq,
+    pub(in crate::gsp) cpuq: Msgq,
     /// GSP queue: the GSP writes messages here, and the driver reads them. It also contains the
     /// write and read pointers that the GSP updates.
     ///
     /// This member is read-only for the driver.
-    gspq: Msgq,
+    pub(in crate::gsp) gspq: Msgq,
 }
 
 // SAFETY: These structs don't meet the no-padding requirements of AsBytes but
@@ -321,12 +319,7 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
     //
     // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
     fn gsp_write_ptr(&self) -> u32 {
-        let gsp_mem = self.0.start_ptr();
-
-        // SAFETY:
-        //  - The 'CoherentAllocation' contains at least one object.
-        //  - By the invariants of `CoherentAllocation` the pointer is valid.
-        (unsafe { (*gsp_mem).gspq.tx.write_ptr() } % MSGQ_NUM_PAGES)
+        super::fw::gsp_mem::gsp_write_ptr(&self.0)
     }
 
     // Returns the index of the memory page the GSP will read the next command from.
@@ -335,12 +328,7 @@ fn gsp_write_ptr(&self) -> u32 {
     //
     // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
     fn gsp_read_ptr(&self) -> u32 {
-        let gsp_mem = self.0.start_ptr();
-
-        // SAFETY:
-        //  - The 'CoherentAllocation' contains at least one object.
-        //  - By the invariants of `CoherentAllocation` the pointer is valid.
-        (unsafe { (*gsp_mem).gspq.rx.read_ptr() } % MSGQ_NUM_PAGES)
+        super::fw::gsp_mem::gsp_read_ptr(&self.0)
     }
 
     // Returns the index of the memory page the CPU can read the next message from.
@@ -349,27 +337,12 @@ fn gsp_read_ptr(&self) -> u32 {
     //
     // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
     fn cpu_read_ptr(&self) -> u32 {
-        let gsp_mem = self.0.start_ptr();
-
-        // SAFETY:
-        //  - The ['CoherentAllocation'] contains at least one object.
-        //  - By the invariants of CoherentAllocation the pointer is valid.
-        (unsafe { (*gsp_mem).cpuq.rx.read_ptr() } % MSGQ_NUM_PAGES)
+        super::fw::gsp_mem::cpu_read_ptr(&self.0)
     }
 
     // Informs the GSP that it can send `elem_count` new pages into the message queue.
     fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
-        let rptr = self.cpu_read_ptr().wrapping_add(elem_count) % MSGQ_NUM_PAGES;
-
-        // Ensure read pointer is properly ordered.
-        fence(Ordering::SeqCst);
-
-        let gsp_mem = self.0.start_ptr_mut();
-
-        // SAFETY:
-        //  - The 'CoherentAllocation' contains at least one object.
-        //  - By the invariants of `CoherentAllocation` the pointer is valid.
-        unsafe { (*gsp_mem).cpuq.rx.set_read_ptr(rptr) };
+        super::fw::gsp_mem::advance_cpu_read_ptr(&self.0, elem_count)
     }
 
     // Returns the index of the memory page the CPU can write the next command to.
@@ -378,26 +351,12 @@ fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
     //
     // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
     fn cpu_write_ptr(&self) -> u32 {
-        let gsp_mem = self.0.start_ptr();
-
-        // SAFETY:
-        //  - The 'CoherentAllocation' contains at least one object.
-        //  - By the invariants of `CoherentAllocation` the pointer is valid.
-        (unsafe { (*gsp_mem).cpuq.tx.write_ptr() } % MSGQ_NUM_PAGES)
+        super::fw::gsp_mem::cpu_write_ptr(&self.0)
     }
 
     // Informs the GSP that it can process `elem_count` new pages from the command queue.
     fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
-        let wptr = self.cpu_write_ptr().wrapping_add(elem_count) & MSGQ_NUM_PAGES;
-        let gsp_mem = self.0.start_ptr_mut();
-
-        // SAFETY:
-        //  - The 'CoherentAllocation' contains at least one object.
-        //  - By the invariants of `CoherentAllocation` the pointer is valid.
-        unsafe { (*gsp_mem).cpuq.tx.set_write_ptr(wptr) };
-
-        // Ensure all command data is visible before triggering the GSP read.
-        fence(Ordering::SeqCst);
+        super::fw::gsp_mem::advance_cpu_write_ptr(&self.0, elem_count)
     }
 }
 
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 83ff91614e36..ca03d497246d 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -40,6 +40,75 @@
     },
 };
 
+// TODO: Replace with `IoView` projections once available; the `unwrap()` calls go away once we
+// switch to the new `dma::Coherent` API.
+pub(in crate::gsp) mod gsp_mem {
+    use core::sync::atomic::{
+        fence,
+        Ordering, //
+    };
+
+    use kernel::{
+        dma::CoherentAllocation,
+        dma_read,
+        dma_write,
+        prelude::*, //
+    };
+
+    use crate::gsp::cmdq::{
+        GspMem,
+        MSGQ_NUM_PAGES, //
+    };
+
+    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
+        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
+        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
+    }
+
+    pub(in crate::gsp) fn gsp_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
+        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
+        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
+    }
+
+    pub(in crate::gsp) fn cpu_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
+        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
+        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
+    }
+
+    pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
+        let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
+
+        // Ensure read pointer is properly ordered.
+        fence(Ordering::SeqCst);
+
+        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
+        || -> Result {
+            dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
+            Ok(())
+        }()
+        .unwrap()
+    }
+
+    pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
+        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
+        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
+    }
+
+    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
+        let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;
+
+        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
+        || -> Result {
+            dma_write!(qs, [0]?.cpuq.tx.0.writePtr, wptr);
+            Ok(())
+        }()
+        .unwrap();
+
+        // Ensure all command data is visible before triggering the GSP read.
+        fence(Ordering::SeqCst);
+    }
+}
+
 /// Empty type to group methods related to heap parameters for running the GSP firmware.
 enum GspFwHeapParams {}
 
@@ -708,22 +777,6 @@ pub(crate) fn new(msgq_size: u32, rx_hdr_offset: u32, msg_count: u32) -> Self {
             entryOff: num::usize_into_u32::<GSP_PAGE_SIZE>(),
         })
     }
-
-    /// Returns the value of the write pointer for this queue.
-    pub(crate) fn write_ptr(&self) -> u32 {
-        let ptr = core::ptr::from_ref(&self.0.writePtr);
-
-        // SAFETY: `ptr` is a valid pointer to a `u32`.
-        unsafe { ptr.read_volatile() }
-    }
-
-    /// Sets the value of the write pointer for this queue.
-    pub(crate) fn set_write_ptr(&mut self, val: u32) {
-        let ptr = core::ptr::from_mut(&mut self.0.writePtr);
-
-        // SAFETY: `ptr` is a valid pointer to a `u32`.
-        unsafe { ptr.write_volatile(val) }
-    }
 }
 
 // SAFETY: Padding is explicit and does not contain uninitialized data.
@@ -739,22 +792,6 @@ impl MsgqRxHeader {
     pub(crate) fn new() -> Self {
         Self(Default::default())
     }
-
-    /// Returns the value of the read pointer for this queue.
-    pub(crate) fn read_ptr(&self) -> u32 {
-        let ptr = core::ptr::from_ref(&self.0.readPtr);
-
-        // SAFETY: `ptr` is a valid pointer to a `u32`.
-        unsafe { ptr.read_volatile() }
-    }
-
-    /// Sets the value of the read pointer for this queue.
-    pub(crate) fn set_read_ptr(&mut self, val: u32) {
-        let ptr = core::ptr::from_mut(&mut self.0.readPtr);
-
-        // SAFETY: `ptr` is a valid pointer to a `u32`.
-        unsafe { ptr.write_volatile(val) }
-    }
 }
 
 // SAFETY: Padding is explicit and does not contain uninitialized data.

base-commit: 4da879a0d3fd170a70994b73baa554c6913918b5
-- 
2.53.0

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Gary Guo]

On Mon Mar 9, 2026 at 10:53 PM GMT, Danilo Krummrich wrote:
> The DmaGspMem pointer accessor methods (gsp_write_ptr, gsp_read_ptr,
> cpu_read_ptr, cpu_write_ptr, advance_cpu_read_ptr,
> advance_cpu_write_ptr) dereference a raw pointer to DMA memory, creating
> an intermediate reference before calling volatile read/write methods.
>
> This is undefined behavior since DMA memory can be concurrently modified
> by the device.
>
> Fix this by moving the implementations into a gsp_mem module in fw.rs
> that uses the dma_read!() / dma_write!() macros, making the original
> methods on DmaGspMem thin forwarding wrappers.
>
> An alternative approach would have been to wrap the shared memory in
> Opaque, but that would have required even more unsafe code.
>
> Since the gsp_mem module lives in fw.rs (to access firmware-specific
> binding field names), GspMem, Msgq, DmaGspMem and their relevant fields
> are temporarily widened to pub(in crate::gsp). This will be reverted
> once IoView projections are available.
>
> Cc: Gary Guo <gary@garyguo.net>
> Closes: https://lore.kernel.org/nouveau/DGUT14ILG35P.1UMNRKU93JUM1@kernel.org/
> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs |  71 +++++----------------
>  drivers/gpu/nova-core/gsp/fw.rs   | 101 ++++++++++++++++++++----------
>  2 files changed, 84 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index ae54708c38eb..00a4062a47dc 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -2,11 +2,7 @@
>  
>  use core::{
>      cmp,
> -    mem,
> -    sync::atomic::{
> -        fence,
> -        Ordering, //
> -    }, //
> +    mem, //
>  };
>  
>  use kernel::{
> @@ -146,30 +142,32 @@ struct MsgqData {
>  #[repr(C)]
>  // There is no struct defined for this in the open-gpu-kernel-source headers.
>  // Instead it is defined by code in `GspMsgQueuesInit()`.
> -struct Msgq {
> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
> +pub(in crate::gsp) struct Msgq {

These could all be `(in super)`?

>      /// Header for sending messages, including the write pointer.
> -    tx: MsgqTxHeader,
> +    pub(in crate::gsp) tx: MsgqTxHeader,
>      /// Header for receiving messages, including the read pointer.
> -    rx: MsgqRxHeader,
> +    pub(in crate::gsp) rx: MsgqRxHeader,
>      /// The message queue proper.
>      msgq: MsgqData,
>  }
>  
>  /// Structure shared between the driver and the GSP and containing the command and message queues.
>  #[repr(C)]
> -struct GspMem {
> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
> +pub(in crate::gsp) struct GspMem {
>      /// Self-mapping page table entries.
>      ptes: PteArray<{ GSP_PAGE_SIZE / size_of::<u64>() }>,
>      /// CPU queue: the driver writes commands here, and the GSP reads them. It also contains the
>      /// write and read pointers that the CPU updates.
>      ///
>      /// This member is read-only for the GSP.
> -    cpuq: Msgq,
> +    pub(in crate::gsp) cpuq: Msgq,
>      /// GSP queue: the GSP writes messages here, and the driver reads them. It also contains the
>      /// write and read pointers that the GSP updates.
>      ///
>      /// This member is read-only for the driver.
> -    gspq: Msgq,
> +    pub(in crate::gsp) gspq: Msgq,
>  }
>  
>  // SAFETY: These structs don't meet the no-padding requirements of AsBytes but
> @@ -321,12 +319,7 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
>      //
>      // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
>      fn gsp_write_ptr(&self) -> u32 {
> -        let gsp_mem = self.0.start_ptr();
> -
> -        // SAFETY:
> -        //  - The 'CoherentAllocation' contains at least one object.
> -        //  - By the invariants of `CoherentAllocation` the pointer is valid.
> -        (unsafe { (*gsp_mem).gspq.tx.write_ptr() } % MSGQ_NUM_PAGES)
> +        super::fw::gsp_mem::gsp_write_ptr(&self.0)
>      }
>  
>      // Returns the index of the memory page the GSP will read the next command from.
> @@ -335,12 +328,7 @@ fn gsp_write_ptr(&self) -> u32 {
>      //
>      // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
>      fn gsp_read_ptr(&self) -> u32 {
> -        let gsp_mem = self.0.start_ptr();
> -
> -        // SAFETY:
> -        //  - The 'CoherentAllocation' contains at least one object.
> -        //  - By the invariants of `CoherentAllocation` the pointer is valid.
> -        (unsafe { (*gsp_mem).gspq.rx.read_ptr() } % MSGQ_NUM_PAGES)
> +        super::fw::gsp_mem::gsp_read_ptr(&self.0)
>      }
>  
>      // Returns the index of the memory page the CPU can read the next message from.
> @@ -349,27 +337,12 @@ fn gsp_read_ptr(&self) -> u32 {
>      //
>      // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
>      fn cpu_read_ptr(&self) -> u32 {
> -        let gsp_mem = self.0.start_ptr();
> -
> -        // SAFETY:
> -        //  - The ['CoherentAllocation'] contains at least one object.
> -        //  - By the invariants of CoherentAllocation the pointer is valid.
> -        (unsafe { (*gsp_mem).cpuq.rx.read_ptr() } % MSGQ_NUM_PAGES)
> +        super::fw::gsp_mem::cpu_read_ptr(&self.0)
>      }
>  
>      // Informs the GSP that it can send `elem_count` new pages into the message queue.
>      fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
> -        let rptr = self.cpu_read_ptr().wrapping_add(elem_count) % MSGQ_NUM_PAGES;
> -
> -        // Ensure read pointer is properly ordered.
> -        fence(Ordering::SeqCst);
> -
> -        let gsp_mem = self.0.start_ptr_mut();
> -
> -        // SAFETY:
> -        //  - The 'CoherentAllocation' contains at least one object.
> -        //  - By the invariants of `CoherentAllocation` the pointer is valid.
> -        unsafe { (*gsp_mem).cpuq.rx.set_read_ptr(rptr) };
> +        super::fw::gsp_mem::advance_cpu_read_ptr(&self.0, elem_count)
>      }
>  
>      // Returns the index of the memory page the CPU can write the next command to.
> @@ -378,26 +351,12 @@ fn advance_cpu_read_ptr(&mut self, elem_count: u32) {
>      //
>      // - The returned value is between `0` and `MSGQ_NUM_PAGES`.
>      fn cpu_write_ptr(&self) -> u32 {
> -        let gsp_mem = self.0.start_ptr();
> -
> -        // SAFETY:
> -        //  - The 'CoherentAllocation' contains at least one object.
> -        //  - By the invariants of `CoherentAllocation` the pointer is valid.
> -        (unsafe { (*gsp_mem).cpuq.tx.write_ptr() } % MSGQ_NUM_PAGES)
> +        super::fw::gsp_mem::cpu_write_ptr(&self.0)
>      }
>  
>      // Informs the GSP that it can process `elem_count` new pages from the command queue.
>      fn advance_cpu_write_ptr(&mut self, elem_count: u32) {
> -        let wptr = self.cpu_write_ptr().wrapping_add(elem_count) & MSGQ_NUM_PAGES;
> -        let gsp_mem = self.0.start_ptr_mut();
> -
> -        // SAFETY:
> -        //  - The 'CoherentAllocation' contains at least one object.
> -        //  - By the invariants of `CoherentAllocation` the pointer is valid.
> -        unsafe { (*gsp_mem).cpuq.tx.set_write_ptr(wptr) };
> -
> -        // Ensure all command data is visible before triggering the GSP read.
> -        fence(Ordering::SeqCst);
> +        super::fw::gsp_mem::advance_cpu_write_ptr(&self.0, elem_count)
>      }
>  }
>  
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 83ff91614e36..ca03d497246d 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -40,6 +40,75 @@
>      },
>  };
>  
> +// TODO: Replace with `IoView` projections once available; the `unwrap()` calls go away once we
> +// switch to the new `dma::Coherent` API.
> +pub(in crate::gsp) mod gsp_mem {
> +    use core::sync::atomic::{
> +        fence,
> +        Ordering, //
> +    };
> +
> +    use kernel::{
> +        dma::CoherentAllocation,
> +        dma_read,
> +        dma_write,
> +        prelude::*, //
> +    };
> +
> +    use crate::gsp::cmdq::{
> +        GspMem,
> +        MSGQ_NUM_PAGES, //
> +    };
> +
> +    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()

I wonder if I should add a panicking variant of index projection for this case.
Perhaps of syntax `[index]!`.

We could also make the existing `[index]` becoming a panicking one instead of
`build_error!` one. It is more consistent with Rust index operator that way.


> +    }
> +
> +    pub(in crate::gsp) fn gsp_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    }
> +
> +    pub(in crate::gsp) fn cpu_read_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.rx.0.readPtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    }
> +
> +    pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
> +        let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
> +
> +        // Ensure read pointer is properly ordered.
> +        fence(Ordering::SeqCst);
> +
> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> +        || -> Result {
> +            dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
> +            Ok(())
> +        }()
> +        .unwrap()
> +    }
> +
> +    pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
> +    }
> +
> +    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
> +        let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;

Not really related to your change, but this `&` probably require a comment, as
it has different behaviour compared to `%` given the `MSGQ_NUM_PAGES` is not
power of two. I suppose this is actually intended so there's a way to
distinguish between empty and full ring buffer?

Best,
Gary

> +
> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
> +        || -> Result {
> +            dma_write!(qs, [0]?.cpuq.tx.0.writePtr, wptr);
> +            Ok(())
> +        }()
> +        .unwrap();
> +
> +        // Ensure all command data is visible before triggering the GSP read.
> +        fence(Ordering::SeqCst);
> +    }
> +}
> +
>  /// Empty type to group methods related to heap parameters for running the GSP firmware.
>  enum GspFwHeapParams {}
>  
> @@ -708,22 +777,6 @@ pub(crate) fn new(msgq_size: u32, rx_hdr_offset: u32, msg_count: u32) -> Self {
>              entryOff: num::usize_into_u32::<GSP_PAGE_SIZE>(),
>          })
>      }
> -
> -    /// Returns the value of the write pointer for this queue.
> -    pub(crate) fn write_ptr(&self) -> u32 {
> -        let ptr = core::ptr::from_ref(&self.0.writePtr);
> -
> -        // SAFETY: `ptr` is a valid pointer to a `u32`.
> -        unsafe { ptr.read_volatile() }
> -    }
> -
> -    /// Sets the value of the write pointer for this queue.
> -    pub(crate) fn set_write_ptr(&mut self, val: u32) {
> -        let ptr = core::ptr::from_mut(&mut self.0.writePtr);
> -
> -        // SAFETY: `ptr` is a valid pointer to a `u32`.
> -        unsafe { ptr.write_volatile(val) }
> -    }
>  }
>  
>  // SAFETY: Padding is explicit and does not contain uninitialized data.
> @@ -739,22 +792,6 @@ impl MsgqRxHeader {
>      pub(crate) fn new() -> Self {
>          Self(Default::default())
>      }
> -
> -    /// Returns the value of the read pointer for this queue.
> -    pub(crate) fn read_ptr(&self) -> u32 {
> -        let ptr = core::ptr::from_ref(&self.0.readPtr);
> -
> -        // SAFETY: `ptr` is a valid pointer to a `u32`.
> -        unsafe { ptr.read_volatile() }
> -    }
> -
> -    /// Sets the value of the read pointer for this queue.
> -    pub(crate) fn set_read_ptr(&mut self, val: u32) {
> -        let ptr = core::ptr::from_mut(&mut self.0.readPtr);
> -
> -        // SAFETY: `ptr` is a valid pointer to a `u32`.
> -        unsafe { ptr.write_volatile(val) }
> -    }
>  }
>  
>  // SAFETY: Padding is explicit and does not contain uninitialized data.
>
> base-commit: 4da879a0d3fd170a70994b73baa554c6913918b5

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Eliot Courtney]

On Tue Mar 10, 2026 at 11:01 AM JST, Gary Guo wrote:
>> +    pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>> +        let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
>> +
>> +        // Ensure read pointer is properly ordered.
>> +        fence(Ordering::SeqCst);
>> +
>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>> +        || -> Result {
>> +            dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
>> +            Ok(())
>> +        }()
>> +        .unwrap()
>> +    }
>> +
>> +    pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>> +    }
>> +
>> +    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>> +        let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;
>
> Not really related to your change, but this `&` probably require a comment, as
> it has different behaviour compared to `%` given the `MSGQ_NUM_PAGES` is not
> power of two. I suppose this is actually intended so there's a way to
> distinguish between empty and full ring buffer?
>
> Best,
> Gary

This is actually incorrect and I have fixed it here[1]. I think it
should be merged in drm-rust-next now.

[1]: https://lore.kernel.org/all/20260129-nova-core-cmdq1-v3-0-2ede85493a27@nvidia.com/

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

On Tue Mar 10, 2026 at 3:01 AM CET, Gary Guo wrote:
>> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
>> +pub(in crate::gsp) struct Msgq {
>
> These could all be `(in super)`?

Yes, or just pub(super). However, that's not the case for the functions in the
gsp_mem module, they could be pub(in super::super) though. But I think I prefer
pub(in crate::gsp) for those.

>> +    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>
> I wonder if I should add a panicking variant of index projection for this case.
> Perhaps of syntax `[index]!`.
>
> We could also make the existing `[index]` becoming a panicking one instead of
> `build_error!` one. It is more consistent with Rust index operator that way.

I thought the same, as something like this `[n]?.ptes[i]` looks a bit odd.

However, I think we ideally want both variants (I like your `[i]!` proposal
above), since generally users should have the choice (as they also have with a
slice through get()). For instance, the index could come from userspace. Sure,
you can always validate the index in advance, but having a fallible variant is a
bit nicer.

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

On Tue Mar 10, 2026 at 4:56 AM CET, Eliot Courtney wrote:
> On Tue Mar 10, 2026 at 11:01 AM JST, Gary Guo wrote:
>>> +    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>>> +        let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;
>>
>> Not really related to your change, but this `&` probably require a comment, as
>> it has different behaviour compared to `%` given the `MSGQ_NUM_PAGES` is not
>> power of two. I suppose this is actually intended so there's a way to
>> distinguish between empty and full ring buffer?
>
> This is actually incorrect and I have fixed it here[1]. I think it
> should be merged in drm-rust-next now.

Probably should have been going through -fixes. Anyways, I will fix it up in
this patch too.

> [1]: https://lore.kernel.org/all/20260129-nova-core-cmdq1-v3-0-2ede85493a27@nvidia.com/

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Alexandre Courbot]

On Tue Mar 10, 2026 at 7:53 AM JST, Danilo Krummrich wrote:
> The DmaGspMem pointer accessor methods (gsp_write_ptr, gsp_read_ptr,
> cpu_read_ptr, cpu_write_ptr, advance_cpu_read_ptr,
> advance_cpu_write_ptr) dereference a raw pointer to DMA memory, creating
> an intermediate reference before calling volatile read/write methods.
>
> This is undefined behavior since DMA memory can be concurrently modified
> by the device.
>
> Fix this by moving the implementations into a gsp_mem module in fw.rs
> that uses the dma_read!() / dma_write!() macros, making the original
> methods on DmaGspMem thin forwarding wrappers.
>
> An alternative approach would have been to wrap the shared memory in
> Opaque, but that would have required even more unsafe code.
>
> Since the gsp_mem module lives in fw.rs (to access firmware-specific
> binding field names), GspMem, Msgq, DmaGspMem and their relevant fields
> are temporarily widened to pub(in crate::gsp). This will be reverted
> once IoView projections are available.
>
> Cc: Gary Guo <gary@garyguo.net>
> Closes: https://lore.kernel.org/nouveau/DGUT14ILG35P.1UMNRKU93JUM1@kernel.org/
> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Thanks, this also removes 10 unsafe statements, which is *very* nice on
top of fixing the UB.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Gary Guo]

On Tue Mar 10, 2026 at 3:56 AM GMT, Eliot Courtney wrote:
> On Tue Mar 10, 2026 at 11:01 AM JST, Gary Guo wrote:
>>> +    pub(in crate::gsp) fn advance_cpu_read_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>>> +        let rptr = cpu_read_ptr(qs).wrapping_add(count) % MSGQ_NUM_PAGES;
>>> +
>>> +        // Ensure read pointer is properly ordered.
>>> +        fence(Ordering::SeqCst);
>>> +
>>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>> +        || -> Result {
>>> +            dma_write!(qs, [0]?.cpuq.rx.0.readPtr, rptr);
>>> +            Ok(())
>>> +        }()
>>> +        .unwrap()
>>> +    }
>>> +
>>> +    pub(in crate::gsp) fn cpu_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.cpuq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>>> +    }
>>> +
>>> +    pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &CoherentAllocation<GspMem>, count: u32) {
>>> +        let wptr = cpu_write_ptr(qs).wrapping_add(count) & MSGQ_NUM_PAGES;
>>
>> Not really related to your change, but this `&` probably require a comment, as
>> it has different behaviour compared to `%` given the `MSGQ_NUM_PAGES` is not
>> power of two. I suppose this is actually intended so there's a way to
>> distinguish between empty and full ring buffer?
>>
>> Best,
>> Gary
>
> This is actually incorrect and I have fixed it here[1]. I think it
> should be merged in drm-rust-next now.

Right, I recollect reviewing your series which is why I am confused when I still
see the `&` present, thinking that this must be intentional rather than
accidental.

If this patch is intended to go through -fixes, then we should really land your
series via -fixes too, otherwise it is just causing unnecessary conflicts on
linux-next.

Best,
Gary

>
> [1]: https://lore.kernel.org/all/20260129-nova-core-cmdq1-v3-0-2ede85493a27@nvidia.com/

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Gary Guo]

On Tue Mar 10, 2026 at 10:58 AM GMT, Danilo Krummrich wrote:
> On Tue Mar 10, 2026 at 3:01 AM CET, Gary Guo wrote:
>>> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
>>> +pub(in crate::gsp) struct Msgq {
>>
>> These could all be `(in super)`?
>
> Yes, or just pub(super). However, that's not the case for the functions in the
> gsp_mem module, they could be pub(in super::super) though. But I think I prefer
> pub(in crate::gsp) for those.
>
>>> +    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>>
>> I wonder if I should add a panicking variant of index projection for this case.
>> Perhaps of syntax `[index]!`.
>>
>> We could also make the existing `[index]` becoming a panicking one instead of
>> `build_error!` one. It is more consistent with Rust index operator that way.
>
> I thought the same, as something like this `[n]?.ptes[i]` looks a bit odd.
>
> However, I think we ideally want both variants (I like your `[i]!` proposal
> above), since generally users should have the choice (as they also have with a
> slice through get()). For instance, the index could come from userspace. Sure,
> you can always validate the index in advance, but having a fallible variant is a
> bit nicer.

I'm not proposing removal of the fallible variant, just that we can make the
infallible one use panicking instead of `build_error!`.

best,
Gary

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

On Wed Mar 11, 2026 at 1:58 PM CET, Gary Guo wrote:
> If this patch is intended to go through -fixes, then we should really land your
> series via -fixes too, otherwise it is just causing unnecessary conflicts on
> linux-next.

It has already been merged in drm-rust-next, so we will have the conflict
temporarily. I say temporarily because I will backmerge once the changes in
-fixes hit Linus' tree anyways.

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

On Wed Mar 11, 2026 at 1:59 PM CET, Gary Guo wrote:
> On Tue Mar 10, 2026 at 10:58 AM GMT, Danilo Krummrich wrote:
>> On Tue Mar 10, 2026 at 3:01 AM CET, Gary Guo wrote:
>>>> +// TODO: Revert to private once `IoView` projections replace the `gsp_mem` module.
>>>> +pub(in crate::gsp) struct Msgq {
>>>
>>> These could all be `(in super)`?
>>
>> Yes, or just pub(super). However, that's not the case for the functions in the
>> gsp_mem module, they could be pub(in super::super) though. But I think I prefer
>> pub(in crate::gsp) for those.
>>
>>>> +    pub(in crate::gsp) fn gsp_write_ptr(qs: &CoherentAllocation<GspMem>) -> u32 {
>>>> +        // PANIC: A `dma::CoherentAllocation` always contains at least one element.
>>>> +        || -> Result<u32> { Ok(dma_read!(qs, [0]?.gspq.tx.0.writePtr) % MSGQ_NUM_PAGES) }().unwrap()
>>>
>>> I wonder if I should add a panicking variant of index projection for this case.
>>> Perhaps of syntax `[index]!`.
>>>
>>> We could also make the existing `[index]` becoming a panicking one instead of
>>> `build_error!` one. It is more consistent with Rust index operator that way.
>>
>> I thought the same, as something like this `[n]?.ptes[i]` looks a bit odd.
>>
>> However, I think we ideally want both variants (I like your `[i]!` proposal
>> above), since generally users should have the choice (as they also have with a
>> slice through get()). For instance, the index could come from userspace. Sure,
>> you can always validate the index in advance, but having a fallible variant is a
>> bit nicer.
>
> I'm not proposing removal of the fallible variant, just that we can make the
> infallible one use panicking instead of `build_error!`.

SGTM.

Re: [PATCH] gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

[Danilo Krummrich]

On Mon Mar 9, 2026 at 11:53 PM CET, Danilo Krummrich wrote:

Applied to drm-rust-fixes, thanks!

--- commit ---

commit 0073a17b466684413ac87cf8ff6c19560db44e7a
Author: Danilo Krummrich <dakr@kernel.org>
Date:   Mon Mar 9 23:53:24 2026 +0100

    gpu: nova-core: gsp: fix UB in DmaGspMem pointer accessors

    The DmaGspMem pointer accessor methods (gsp_write_ptr, gsp_read_ptr,
    cpu_read_ptr, cpu_write_ptr, advance_cpu_read_ptr,
    advance_cpu_write_ptr) dereference a raw pointer to DMA memory, creating
    an intermediate reference before calling volatile read/write methods.

    This is undefined behavior since DMA memory can be concurrently modified
    by the device.

    Fix this by moving the implementations into a gsp_mem module in fw.rs
    that uses the dma_read!() / dma_write!() macros, making the original
    methods on DmaGspMem thin forwarding wrappers.

    An alternative approach would have been to wrap the shared memory in
    Opaque, but that would have required even more unsafe code.

    Since the gsp_mem module lives in fw.rs (to access firmware-specific
    binding field names), GspMem, Msgq and their relevant fields are
    temporarily widened to pub(super). This will be reverted once IoView
    projections are available.

    Cc: Gary Guo <gary@garyguo.net>
    Closes: https://lore.kernel.org/nouveau/DGUT14ILG35P.1UMNRKU93JUM1@kernel.org/
    Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
    Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
    Link: https://patch.msgid.link/20260309225408.27714-1-dakr@kernel.org
    [ Use pub(super) where possible; replace bitwise-and with modulo
      operator analogous to [1]. - Danilo ]
    Link: https://lore.kernel.org/all/20260129-nova-core-cmdq1-v3-1-2ede85493a27@nvidia.com/ [1]
    Signed-off-by: Danilo Krummrich <dakr@kernel.org>