| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87h63qhz4q.fsf@kernel.org>
Date: Tue, 18 Mar 2025 21:17:41 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Abdiel Janulgue" <abdiel.janulgue@...il.com>
Cc: <rust-for-linux@...r.kernel.org>, <daniel.almeida@...labora.com>,
<dakr@...nel.org>, <robin.murphy@....com>, <aliceryhl@...gle.com>,
"Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
<alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
<gary@...yguo.net>, Björn Roy Baron
<bjorn3_gh@...tonmail.com>, "Benno
Lossin" <benno.lossin@...ton.me>, "Trevor Gross" <tmgross@...ch.edu>,
"Valentin Obst" <kernel@...entinobst.de>, "open list"
<linux-kernel@...r.kernel.org>, "Christoph Hellwig" <hch@....de>, "Marek
Szyprowski" <m.szyprowski@...sung.com>, <airlied@...hat.com>, "open
list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver
"Abdiel Janulgue" <abdiel.janulgue@...il.com> writes:
> Hi,
>
> On 18/03/2025 15:26, Andreas Hindborg wrote:
>> Abdiel Janulgue <abdiel.janulgue@...il.com> writes:
>>
>>> Add a simple driver to excercise the basics of the Rust DMA
>>> coherent allocator bindings.
>>>
>>> Suggested-by: Danilo Krummrich <dakr@...nel.org>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
>>> ---
>>> samples/rust/Kconfig | 11 +++++
>>> samples/rust/Makefile | 1 +
>>> samples/rust/rust_dma.rs | 97 ++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 109 insertions(+)
>>> create mode 100644 samples/rust/rust_dma.rs
>>>
>>> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>>> index 3b6eae84b297..e2d14aa6beec 100644
>>> --- a/samples/rust/Kconfig
>>> +++ b/samples/rust/Kconfig
>>> @@ -78,4 +78,15 @@ config SAMPLE_RUST_HOSTPROGS
>>>
>>> If unsure, say N.
>>>
>>> +config SAMPLE_RUST_DRIVER_DMA
>>> + tristate "DMA Test Driver"
>>> + depends on PCI
>>> + help
>>> + This option builds the Rust dma test driver sample.
>>> +
>>> + To compile this as a module, choose M here:
>>> + the module will be called dma.
>>> +
>>> + If unsure, say N.
>>> +
>>> endif # SAMPLES_RUST
>>> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
>>> index 0dbc6d90f1ef..1a9aff6e8d6a 100644
>>> --- a/samples/rust/Makefile
>>> +++ b/samples/rust/Makefile
>>> @@ -7,6 +7,7 @@ obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o
>>> obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o
>>> obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o
>>> obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o
>>> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_DMA) += rust_dma.o
>>>
>>> rust_print-y := rust_print_main.o rust_print_events.o
>>>
>>> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
>>> new file mode 100644
>>> index 000000000000..1740140faba6
>>> --- /dev/null
>>> +++ b/samples/rust/rust_dma.rs
>>> @@ -0,0 +1,97 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Rust DMA api test (based on QEMU's `pci-testdev`).
>>> +//!
>>> +//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
>>> +
>>> +use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
>>> +
>>> +struct DmaSampleDriver {
>>> + pdev: pci::Device,
>>> + ca: CoherentAllocation<MyStruct>,
>>> +}
>>> +
>>> +const TEST_VALUES: [(u32, u32); 5] = [
>>> + (0xa, 0xb),
>>> + (0xc, 0xd),
>>> + (0xe, 0xf),
>>> + (0xab, 0xba),
>>> + (0xcd, 0xef),
>>> +];
>>> +
>>> +struct MyStruct {
>>> + h: u32,
>>> + b: u32,
>>> +}
>>> +
>>> +impl MyStruct {
>>> + fn new(h: u32, b: u32) -> Self {
>>> + Self { h, b }
>>> + }
>>> +}
>>> +// SAFETY: All bit patterns are acceptable values for `MyStruct`.
>>> +unsafe impl kernel::transmute::AsBytes for MyStruct {}
>>> +// SAFETY: Instances of `MyStruct` have no uninitialized portions.
>>> +unsafe impl kernel::transmute::FromBytes for MyStruct {}
>>> +
>>> +kernel::pci_device_table!(
>>> + PCI_TABLE,
>>> + MODULE_PCI_TABLE,
>>> + <DmaSampleDriver as pci::Driver>::IdInfo,
>>> + [(
>>> + pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
>>> + ()
>>> + )]
>>> +);
>>> +
>>> +impl pci::Driver for DmaSampleDriver {
>>> + type IdInfo = ();
>>> + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
>>> +
>>> + fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
>>> + dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
>>> +
>>> + let ca: CoherentAllocation<MyStruct> =
>>> + CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
>>> +
>>> + || -> Result {
>>> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
>>> + kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
>>> + }
>>> +
>>> + Ok(())
>>> + }()?;
>>
>> Why is this placed in a closure? Left over from deferred error for pin-init?
>>
>
> The macro expands into a block which needs to validate the function
> item_from_index()?
It is not required here, since we are already in a function that returns
result.
>
>>> +
>>> + let drvdata = KBox::new(
>>> + Self {
>>> + pdev: pdev.clone(),
>>> + ca,
>>> + },
>>> + GFP_KERNEL,
>>> + )?;
>>> +
>>> + Ok(drvdata.into())
>>> + }
>>> +}
>>> +
>>> +impl Drop for DmaSampleDriver {
>>> + fn drop(&mut self) {
>>> + dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
>>> +
>>> + let _ = || -> Result {
>>> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
>>> + assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
>>> + assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
>>
>> We should probably change `dma_read!`/`dma_write!` to return `Result`,
>> so that we don't have to wrap these calls in a closure for obscure reasons.
>>
>
> Changing `dma_read!`/`dma_write!` to return `Result` is probably not a
> trivial change, would it be okay to have this fixed in a subsequent patch?
It is actually trivial:
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 027ef75a461a..52d235f61886 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -446,20 +446,22 @@ fn drop(&mut self) {
#[macro_export]
macro_rules! dma_read {
($dma:expr, $idx: expr, $($field:tt)*) => {{
- let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
- // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
- // dereferenced. The compiler also further validates the expression on whether `field`
- // is a member of `item` when expanded by the macro.
- unsafe {
- let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
- $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
- }
+ (|| -> Result<_> {
+ let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+ // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+ // dereferenced. The compiler also further validates the expression on whether `field`
+ // is a member of `item` when expanded by the macro.
+ unsafe {
+ let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
+ ::core::result::Result::Ok($crate::dma::CoherentAllocation::field_read(&$dma, ptr_field))
+ }
+ })()
}};
($dma:ident [ $idx:expr ] $($field:tt)* ) => {
- $crate::dma_read!($dma, $idx, $($field)*);
+ $crate::dma_read!($dma, $idx, $($field)*)
};
($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
- $crate::dma_read!($($dma).*, $idx, $($field)*);
+ $crate::dma_read!($($dma).*, $idx, $($field)*)
};
}
@@ -486,25 +488,31 @@ macro_rules! dma_read {
#[macro_export]
macro_rules! dma_write {
($dma:ident [ $idx:expr ] $($field:tt)*) => {{
- $crate::dma_write!($dma, $idx, $($field)*);
+ $crate::dma_write!($dma, $idx, $($field)*)
}};
($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
- $crate::dma_write!($($dma).*, $idx, $($field)*);
+ $crate::dma_write!($($dma).*, $idx, $($field)*)
}};
($dma:expr, $idx: expr, = $val:expr) => {
- 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) }
+ (|| -> Result {
+ 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) }
+ ::core::result::Result::Ok(())
+ })()
};
($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
- let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
- // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
- // dereferenced. The compiler also further validates the expression on whether `field`
- // 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)
- }
+ (|| -> Result {
+ let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+ // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
+ // dereferenced. The compiler also further validates the expression on whether `field`
+ // 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)
+ }
+ ::core::result::Result::Ok(())
+ })()
};
}
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 39b6050aa3b6..962e65322893 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -61,13 +61,10 @@ fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>
let ca: CoherentAllocation<MyStruct> =
CoherentAllocation::alloc_coherent(pdev, TEST_VALUES.len(), GFP_KERNEL)?;
- || -> Result {
- for (i, value) in TEST_VALUES.into_iter().enumerate() {
- kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
- }
+ for (i, value) in TEST_VALUES.into_iter().enumerate() {
+ kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
+ }
- Ok(())
- }()?;
let drvdata = KBox::new(
Self {
@@ -85,13 +82,10 @@ impl Drop for DmaSampleDriver {
fn drop(&mut self) {
dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
- let _ = || -> Result {
- for (i, value) in TEST_VALUES.into_iter().enumerate() {
- assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
- assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
- }
- Ok(())
- }();
+ for (i, value) in TEST_VALUES.into_iter().enumerate() {
+ assert_eq!(kernel::dma_read!(self.ca[i].h).unwrap(), value.0);
+ assert_eq!(kernel::dma_read!(self.ca[i].b).unwrap(), value.1);
+ }
}
}
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists