All of lore.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.