public inbox for rust-for-linux@vger.kernel.org
 help / color / mirror / Atom feed
From: John Hubbard <jhubbard@nvidia.com>
To: Danilo Krummrich <dakr@kernel.org>,
	Alexandre Courbot <acourbot@nvidia.com>
Cc: "Joel Fernandes" <joelagnelf@nvidia.com>,
	"Timur Tabi" <ttabi@nvidia.com>,
	"Alistair Popple" <apopple@nvidia.com>,
	"Eliot Courtney" <ecourtney@nvidia.com>,
	"Shashank Sharma" <shashanks@nvidia.com>,
	"Zhi Wang" <zhiw@nvidia.com>, "David Airlie" <airlied@gmail.com>,
	"Simona Vetter" <simona@ffwll.ch>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	rust-for-linux@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>,
	"John Hubbard" <jhubbard@nvidia.com>
Subject: [PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code"
Date: Fri,  3 Apr 2026 17:02:54 -0700	[thread overview]
Message-ID: <20260404000254.284467-1-jhubbard@nvidia.com> (raw)

This reverts commit e3dcfad7a3c485456146587f80498348efd4f3de.

The commit uses ptr::project!() infallible range indexing on runtime
values read from DMA-coherent memory. When the compiler cannot prove
the indices are in-bounds, the build_error!() fallback survives
optimization and the rust_build_error symbol remains in the object.
For module builds this causes a modpost failure:

  ERROR: modpost: "rust_build_error" [drivers/gpu/nova-core/nova_core.ko] undefined!

The preceding commit 8815e8427a82 ("gpu: nova-core: gsp: inline methods
providing queue range invariants") adds #[inline(always)] to the pointer
accessors so the optimizer can see the range invariants and eliminate
the build_error!() path. This works on LLVM 21 (rustc 1.93.0) but not on
LLVM 18 (rustc 1.78.0), nor on LLVM 19 (rustc 1.85.0).

The kernel minimum is rustc 1.78.0.

Cc: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: John Hubbard <jhubbard@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 114 +++++++++++++-----------------
 1 file changed, 49 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 485d0c0f2a4b..72e9b79619eb 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -17,7 +17,6 @@
     },
     new_mutex,
     prelude::*,
-    ptr,
     sync::{
         aref::ARef,
         Mutex, //
@@ -256,52 +255,38 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
     /// that case the second slice will have a non-zero length.
     fn driver_write_area(&mut self) -> (&mut [[u8; GSP_PAGE_SIZE]], &mut [[u8; GSP_PAGE_SIZE]]) {
-        let tx = num::u32_as_usize(self.cpu_write_ptr());
-        let rx = num::u32_as_usize(self.gsp_read_ptr());
-
-        // Command queue data.
-        let data = ptr::project!(mut self.0.as_mut_ptr(), .cpuq.msgq.data);
-
-        let (tail_slice, wrap_slice) = if rx == 0 {
-            // The write area is non-wrapping, and stops at the second-to-last entry of the command
-            // queue (to leave the last one empty).
-            (
-                ptr::project!(mut data, [tx..num::u32_as_usize(MSGQ_NUM_PAGES) - 1]),
-                ptr::project!(mut data, [..0]),
-            )
-        } else {
-            // Leave an empty slot before `rx`.
-            let end = rx - 1;
-
-            if rx <= tx {
-                // The write area wraps and continues until `end`.
-                (
-                    ptr::project!(mut data, [tx..]),
-                    ptr::project!(mut data, [..end]),
-                )
-            } else {
-                // The write area doesn't wrap and stops at `end`.
-                (
-                    ptr::project!(mut data, [tx..end]),
-                    ptr::project!(mut data, [..0]),
-                )
-            }
-        };
+        let tx = self.cpu_write_ptr() as usize;
+        let rx = self.gsp_read_ptr() as usize;
 
         // SAFETY:
-        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
-        //   pointers to valid arrays.
-        // - The area starting at `tx` and ending at `rx - 2` modulo `MSGQ_NUM_PAGES`,
-        //   inclusive, belongs to the driver for writing and is not accessed concurrently by
-        //   the GSP.
-        // - The caller holds a reference to `self` for as long as the returned slices are live,
-        //   meaning the CPU write pointer cannot be advanced and thus that the returned area
-        //   remains exclusive to the CPU for the duration of the slices.
-        // - `tail_slice` and `wrap_slice` point to non-overlapping sub-ranges of `data` in all
-        //   branches (in the `rx <= tx` case, `wrap_slice` ends at `rx - 1` which is strictly less
-        //   than `tx` where `tail_slice` starts; in the other cases `wrap_slice` is empty), so
-        //   creating two `&mut` references from them does not violate aliasing rules.
-        (unsafe { &mut *tail_slice }, unsafe { &mut *wrap_slice })
+        // - We will only access the driver-owned part of the shared memory.
+        // - Per the safety statement of the function, no concurrent access will be performed.
+        let gsp_mem = unsafe { &mut *self.0.as_mut() };
+        // PANIC: per the invariant of `cpu_write_ptr`, `tx` is `< MSGQ_NUM_PAGES`.
+        let (before_tx, after_tx) = gsp_mem.cpuq.msgq.data.split_at_mut(tx);
+
+        // The area starting at `tx` and ending at `rx - 2` modulo MSGQ_NUM_PAGES, inclusive,
+        // belongs to the driver for writing.
+
+        if rx == 0 {
+            // Since `rx` is zero, leave an empty slot at end of the buffer.
+            let last = after_tx.len() - 1;
+            (&mut after_tx[..last], &mut [])
+        } else if rx <= tx {
+            // The area is discontiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - 1` is non-negative because `rx != 0` in this branch.
+            // - The index does not exceed `before_tx.len()` (which equals `tx`) because
+            //   `rx <= tx` in this branch.
+            (after_tx, &mut before_tx[..(rx - 1)])
+        } else {
+            // The area is contiguous and we leave an empty slot before `rx`.
+            // PANIC:
+            // - The index `rx - tx - 1` is non-negative because `rx > tx` in this branch.
+            // - The index does not exceed `after_tx.len()` (which is `MSGQ_NUM_PAGES - tx`)
+            //   because `rx < MSGQ_NUM_PAGES` by the `gsp_read_ptr` invariant.
+            (&mut after_tx[..(rx - tx - 1)], &mut [])
+        }
     }
 
     /// Returns the size of the region of the CPU message queue that the driver is currently allowed
@@ -323,28 +308,27 @@ fn driver_write_area_size(&self) -> usize {
     /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
     /// that case the second slice will have a non-zero length.
     fn driver_read_area(&self) -> (&[[u8; GSP_PAGE_SIZE]], &[[u8; GSP_PAGE_SIZE]]) {
-        let tx = num::u32_as_usize(self.gsp_write_ptr());
-        let rx = num::u32_as_usize(self.cpu_read_ptr());
-
-        // Message queue data.
-        let data = ptr::project!(self.0.as_ptr(), .gspq.msgq.data);
-
-        let (tail_slice, wrap_slice) = if rx <= tx {
-            (ptr::project!(data, [rx..tx]), ptr::project!(data, [..0]))
-        } else {
-            (ptr::project!(data, [rx..]), ptr::project!(data, [..tx]))
-        };
+        let tx = self.gsp_write_ptr() as usize;
+        let rx = self.cpu_read_ptr() as usize;
 
         // SAFETY:
-        // - Since `data` was created from a valid pointer, both `tail_slice` and `wrap_slice` are
-        //   pointers to valid arrays.
-        // - The area starting at `rx` and ending at `tx - 1` modulo `MSGQ_NUM_PAGES`,
-        //   inclusive, belongs to the driver for reading and is not accessed concurrently by
-        //   the GSP.
-        // - The caller holds a reference to `self` for as long as the returned slices are live,
-        //   meaning the CPU read pointer cannot be advanced and thus that the returned area
-        //   remains exclusive to the CPU for the duration of the slices.
-        (unsafe { &*tail_slice }, unsafe { &*wrap_slice })
+        // - We will only access the driver-owned part of the shared memory.
+        // - Per the safety statement of the function, no concurrent access will be performed.
+        let gsp_mem = unsafe { &*self.0.as_ptr() };
+        let data = &gsp_mem.gspq.msgq.data;
+
+        // The area starting at `rx` and ending at `tx - 1` modulo MSGQ_NUM_PAGES, inclusive,
+        // belongs to the driver for reading.
+        // PANIC:
+        // - per the invariant of `cpu_read_ptr`, `rx < MSGQ_NUM_PAGES`
+        // - per the invariant of `gsp_write_ptr`, `tx < MSGQ_NUM_PAGES`
+        if rx <= tx {
+            // The area is contiguous.
+            (&data[rx..tx], &[])
+        } else {
+            // The area is discontiguous.
+            (&data[rx..], &data[..tx])
+        }
     }
 
     /// Allocates a region on the command queue that is large enough to send a command of `size`

base-commit: 1db742b7ac4dd3fdb7dd62d1e1e6d0faf57f1173
-- 
2.53.0


             reply	other threads:[~2026-04-04  0:03 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-04  0:02 John Hubbard [this message]
2026-04-04  0:26 ` [PATCH] Revert "gpu: nova-core: gsp: fix undefined behavior in command queue code" Miguel Ojeda
2026-04-04  0:31   ` John Hubbard
2026-04-04  0:36     ` Miguel Ojeda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260404000254.284467-1-jhubbard@nvidia.com \
    --to=jhubbard@nvidia.com \
    --cc=a.hindborg@kernel.org \
    --cc=acourbot@nvidia.com \
    --cc=airlied@gmail.com \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=apopple@nvidia.com \
    --cc=bhelgaas@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=dakr@kernel.org \
    --cc=ecourtney@nvidia.com \
    --cc=gary@garyguo.net \
    --cc=joelagnelf@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=shashanks@nvidia.com \
    --cc=simona@ffwll.ch \
    --cc=tmgross@umich.edu \
    --cc=ttabi@nvidia.com \
    --cc=zhiw@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox