[PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()

[PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()

[Lyude Paul]

At the moment - CoherentAllocation::field_write() only takes an immutable
reference to self. This means it's possible for a user to mistakenly call
field_write() while Rust still has a slice taken out for the coherent
allocation:

  let alloc: CoherentAllocation<CoolStruct> = /* … */;

  let evil_slice = unsafe { alloc.as_slice(/* … */)? };
  dma_write!(alloc[1].cool_field = 42); /* UB! */

Keep in mind: the above example is technically a violation of the safety
contract of as_slice(), so luckily this detail shouldn't currently be
causing any UB in the kernel. But, there's no reason we should be solely
relying on the safety contract for enforcing this when we can just use a
mutable reference and already do so in other parts of the API.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Cc: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/dma.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 4e0af3e1a3b9a..e6ac0be80da96 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -611,7 +611,7 @@ pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
     ///
     /// Public but hidden since it should only be used from [`dma_write`] macro.
     #[doc(hidden)]
-    pub unsafe fn field_write<F: AsBytes>(&self, field: *mut F, val: F) {
+    pub unsafe fn field_write<F: AsBytes>(&mut self, field: *mut F, val: F) {
         // SAFETY:
         // - By the safety requirements field is valid.
         // - Using write_volatile() here is not sound as per the usual rules, the usage here is
@@ -708,7 +708,7 @@ macro_rules! dma_read {
 /// // SAFETY: Instances of `MyStruct` have no uninitialized portions.
 /// unsafe impl kernel::transmute::AsBytes for MyStruct{};
 ///
-/// # fn test(alloc: &kernel::dma::CoherentAllocation<MyStruct>) -> Result {
+/// # fn test(mut alloc: &mut kernel::dma::CoherentAllocation<MyStruct>) -> Result {
 /// kernel::dma_write!(alloc[2].member = 0xf);
 /// kernel::dma_write!(alloc[1] = MyStruct { member: 0xf });
 /// # Ok::<(), Error>(()) }
@@ -725,7 +725,7 @@ macro_rules! dma_write {
         (|| -> ::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) }
+            unsafe { $crate::dma::CoherentAllocation::field_write(&mut $dma, item, $val) }
             ::core::result::Result::Ok(())
         })()
     };
@@ -737,7 +737,7 @@ macro_rules! dma_write {
             // 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)
+                $crate::dma::CoherentAllocation::field_write(&mut $dma, ptr_field, $val)
             }
             ::core::result::Result::Ok(())
         })()

base-commit: 3b83f5d5e78ac5cddd811a5e431af73959864390
-- 
2.51.0

Re: [PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()

[Danilo Krummrich]

On Tue Oct 28, 2025 at 10:18 PM CET, Lyude Paul wrote:
> At the moment - CoherentAllocation::field_write() only takes an immutable
> reference to self. This means it's possible for a user to mistakenly call
> field_write() while Rust still has a slice taken out for the coherent
> allocation:
>
>   let alloc: CoherentAllocation<CoolStruct> = /* … */;
>
>   let evil_slice = unsafe { alloc.as_slice(/* … */)? };
>   dma_write!(alloc[1].cool_field = 42); /* UB! */
>
> Keep in mind: the above example is technically a violation of the safety
> contract of as_slice(), so luckily this detail shouldn't currently be
> causing any UB in the kernel. But, there's no reason we should be solely
> relying on the safety contract for enforcing this when we can just use a
> mutable reference and already do so in other parts of the API.

While I generally agree with this, the catch is that it would also enforce that
you would need a lock for calling dma_write!() in a concurrent context.

I.e. if your CoherentAllocation is shared between tasks we can currently have
the tasks calling dma_write!() and dma_read!() concurrently without requiring a
lock for the CoherentAllocation.

Requiring a spinlock for such a case wouldn't be the end of the world of course,
but it would still be unnecessary.

Re: [PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()

[Alice Ryhl]

On Tue, Oct 28, 2025 at 05:18:01PM -0400, Lyude Paul wrote:
> At the moment - CoherentAllocation::field_write() only takes an immutable
> reference to self. This means it's possible for a user to mistakenly call
> field_write() while Rust still has a slice taken out for the coherent
> allocation:
> 
>   let alloc: CoherentAllocation<CoolStruct> = /* … */;
> 
>   let evil_slice = unsafe { alloc.as_slice(/* … */)? };
>   dma_write!(alloc[1].cool_field = 42); /* UB! */
> 
> Keep in mind: the above example is technically a violation of the safety
> contract of as_slice(), so luckily this detail shouldn't currently be
> causing any UB in the kernel. But, there's no reason we should be solely
> relying on the safety contract for enforcing this when we can just use a
> mutable reference and already do so in other parts of the API.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Cc: Danilo Krummrich <dakr@kernel.org>

Didn't we do this intentionally so that it's possible to write to
different parts of the allocation without protecting the entire region
with a lock?

Alice

Re: [PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()

[John Hubbard]

On 10/28/25 2:52 PM, Alice Ryhl wrote:
> On Tue, Oct 28, 2025 at 05:18:01PM -0400, Lyude Paul wrote:
>> At the moment - CoherentAllocation::field_write() only takes an immutable
>> reference to self. This means it's possible for a user to mistakenly call
>> field_write() while Rust still has a slice taken out for the coherent
>> allocation:
>>
>>   let alloc: CoherentAllocation<CoolStruct> = /* … */;
>>
>>   let evil_slice = unsafe { alloc.as_slice(/* … */)? };
>>   dma_write!(alloc[1].cool_field = 42); /* UB! */
>>
>> Keep in mind: the above example is technically a violation of the safety
>> contract of as_slice(), so luckily this detail shouldn't currently be
>> causing any UB in the kernel. But, there's no reason we should be solely
>> relying on the safety contract for enforcing this when we can just use a
>> mutable reference and already do so in other parts of the API.
>>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
> 
> Didn't we do this intentionally so that it's possible to write to
> different parts of the allocation without protecting the entire region
> with a lock?
> 

If so, that seems like it was a good decision, IMHO. Because with
DMA, you can't use CPU-side Rust code to provide full safety (the
device is blithely unaware of any of this, and can scribble all over
the area at any time). And while you could prevent the CPU-side code
from interfering with itself in a dma region, the downside is that
we'll take a performance hit and even a deadlock risk, and run slower
than the non-Rust DMA code in the kernel.


thanks,
-- 
John Hubbard

Re: [PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()

[kernel test robot]

Hi Lyude,

kernel test robot noticed the following build errors:

[auto build test ERROR on 3b83f5d5e78ac5cddd811a5e431af73959864390]

url:    https://github.com/intel-lab-lkp/linux/commits/Lyude-Paul/rust-dma-Take-mut-self-in-CoherentAllocation-field_write/20251029-052034
base:   3b83f5d5e78ac5cddd811a5e431af73959864390
patch link:    https://lore.kernel.org/r/20251028211801.85215-1-lyude%40redhat.com
patch subject: [PATCH] rust/dma: Take &mut self in CoherentAllocation::field_write()
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20251031/202510310102.NdHj0ur8-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251031/202510310102.NdHj0ur8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510310102.NdHj0ur8-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0596]: cannot borrow value as mutable, as it is not declared as mutable
   --> samples/rust/rust_dma.rs:70:13
   |
   70 |             kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
   |
   = note: this error originates in the macro `$crate::dma_write` which comes from the expansion of the macro `kernel::dma_write` (in Nightly builds, run with -Z macro-backtrace for more info)
   help: consider changing this to be mutable
   |
   66 |         let mut ca: CoherentAllocation<MyStruct> =
   |             +++

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki