rust: add pointer projection infrastructure and convert DMA

From: Gary Guo <[email protected]>

This adds a general pointer projection infrastructure which is intended to serve as the basis for generic I/O projections. The idea is that we will have a unified way of reading/writing MMIO or DMA allocation, creating typed subviews into them, and able to read/write subfields safely.

This is the first step towards that direction, where an infra for projecting arbitrary pointers are added. This takes care to not assume pointers are part of a valid allocation, so it can be used to project I/O pointers, too. dma_read! and dma_write! is converted to use this infra, as they currently have soundness issues [1] and users have already encountered limitation of it [2].

---
Changes since v1:
- Squashed patch 2,3,4 so that all users are converted in single commit
(Danilo)
- I discovered an additional soundness bug in dma_read/write macros
where it can create misaligned access. This is addressed in this new
version.
- Updated sample driver to have a function that does check instead use a budget-try block (Benno)
- Improved diagnostics when index projection is used on types that are
not slices.
- Link to v1: https://lore.kernel.org/rust-for-linux/[email protected]/

---
Gary Guo (2):
rust: add projection infrastructure
rust: dma: use pointer projection infra for dma_{read,write} macro

 drivers/gpu/nova-core/gsp.rs      |  14 +-
 drivers/gpu/nova-core/gsp/boot.rs |   2 +-
 drivers/gpu/nova-core/gsp/cmdq.rs |  10 +-
 rust/kernel/dma.rs                | 114 ++++++------
 rust/kernel/lib.rs                |   5 +
 rust/kernel/projection.rs         | 284 ++++++++++++++++++++++++++++++
 samples/rust/rust_dma.rs          |  30 ++--
 scripts/Makefile.build            |   4 +-
 8 files changed, 373 insertions(+), 90 deletions(-)
 create mode 100644 rust/kernel/projection.rs

base-commit: 877552aa875839314afad7154b5a561889e87ea9

2.51.2

From: Gary Guo <[email protected]>

Add a generic infrastructure for performing field and index projections on raw pointers. This will form the basis of performing I/O projections.

Pointers manipulations are intentionally using the safe wrapping variants instead of the unsafe variants, as the latter requires pointers to be inside an allocation which is not necessarily true for I/O pointers.

This projection macro protects against rogue Deref implementation, which can causes the projected pointer to be outside the bounds of starting pointer. This is extremely unlikely and Rust has a lint to catch this, but is unsoundness regardless. The protection works by inducing type inference ambiguity when Deref is implemented.

This projection macro also stops projecting into unaligned fields (i.e. fields of #[repr(packed)] structs), as misaligned pointers require special handling. This is implemented by attempting to create reference to projected field inside a if false block. Despite being unreachable, Rust still checks that they're not unaligned fields.

The projection macro supports both fallible and infallible index projections. These are described in detail inside the documentation.

---
 rust/kernel/lib.rs        |   5 +
 rust/kernel/projection.rs | 284 ++++++++++++++++++++++++++++++++++++++
 scripts/Makefile.build    |   4 +-
 3 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/projection.rs

index 3da92f18f4ee..50866b481bdb 100644
@@ -20,6 +20,7 @@
 #![feature(generic_nonzero)]
 #![feature(inline_const)]
 #![feature(pointer_is_aligned)]
+#![feature(slice_ptr_len)]
 //
 // Stable since Rust 1.80.0.
 #![feature(slice_flatten)]

@@ -37,6 +38,9 @@
 #![feature(const_ptr_write)]
 #![feature(const_refs_to_cell)]
 //
+// Stable since Rust 1.84.0.
+#![feature(strict_provenance)]
+//
 // Expected to become stable.
 #![feature(arbitrary_self_types)]
 //

@@ -130,6 +134,7 @@
 pub mod prelude;
 pub mod print;
 pub mod processor;
+pub mod projection;
 pub mod ptr;
 #[cfg(CONFIG_RUST_PWM_ABSTRACTIONS)]
 pub mod pwm;

index 32e209bc7985..3652b85be545 100644
@@ -310,16 +310,18 @@ $(obj)/%.lst: $(obj)/%.c FORCE
 
 # The features in this list are the ones allowed for non-`rust/` code.
 #
+#   - Stable since Rust 1.79.0: `feature(slice_ptr_len)`.
 #   - Stable since Rust 1.81.0: `feature(lint_reasons)`.
 #   - Stable since Rust 1.82.0: `feature(asm_const)`,
 #     `feature(offset_of_nested)`, `feature(raw_ref_op)`.
+#   - Stable since Rust 1.84.0: `feature(strict_provenance)`.
 #   - Stable since Rust 1.87.0: `feature(asm_goto)`.
 #   - Expected to become stable: `feature(arbitrary_self_types)`.
 #   - To be determined: `feature(used_with_arg)`.
 #
 # Please see https://github.com/Rust-for-Linux/linux/issues/2 for details on
 # the unstable features in use.
-rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,offset_of_nested,raw_ref_op,used_with_arg
+rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,offset_of_nested,raw_ref_op,slice_ptr_len,strict_provenance,used_with_arg
 
 # `--out-dir` is required to avoid temporaries being created by `rustc` in the
 # current working directory, which may be not accessible in the out-of-tree

2.51.2

Good catch!

The code below

    if self.end > slice.len() {
        return None;
    }

is rightfully > as it's okay to have end being equal to the length. I suppose this is what you mean by "somewhere else in the code"?

This one though should indeed be >=, and I've obviously messed this one up.

Best,
Gary

Yes the second check which i was referring belongs to core::ops::Range<usize>, I agree with you it makes sense for Range.

Patch looks good to me once this is fixed for usize :)

Thanks
Aditya

From: Gary Guo <[email protected]>

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.

---
 drivers/gpu/nova-core/gsp.rs      |  14 ++--
 drivers/gpu/nova-core/gsp/boot.rs |   2 +-
 drivers/gpu/nova-core/gsp/cmdq.rs |  10 ++-
 rust/kernel/dma.rs                | 114 +++++++++++++-----------------
 samples/rust/rust_dma.rs          |  30 ++++----
 5 files changed, 81 insertions(+), 89 deletions(-)

index 174feaca0a6b..25cd48514c77 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));
                 },
             }))
         })

index c56029f444cb..7f46fa5e9b50 100644
@@ -157,7 +157,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))?;

index 87dbbd6d1be9..0056bfbf0a44 100644
@@ -202,9 +202,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))
     }

index 9c45851c876e..ce39b5545097 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,24 +85,26 @@ 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>) {
         dev_info!(self.pdev, "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!(

2.51.2