* [PATCH v2 0/3] Rust scatterlist abstractions
@ 2023-06-02 10:18 Qingsong Chen
2023-06-02 10:18 ` [PATCH v2 1/3] rust: kernel: add ScatterList abstraction Qingsong Chen
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Qingsong Chen @ 2023-06-02 10:18 UTC (permalink / raw)
To: linux-kernel
Cc: 田洪亮,
Qingsong Chen, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
rust-for-linux
Hi All!
This is a version of scatterlist abstractions for Rust drivers.
Scatterlist is used for efficient management of memory buffers, which is
essential for many kernel-level operations such as Direct Memory Access
(DMA) transfers and crypto APIs.
This patch should be a good start to introduce the crypto APIs for Rust
drivers and to develop cipher algorithms in Rust later.
Changelog:
----------
v1 -> v2:
- Split the old patch into smaller parts.
- Remove the selftest module, and place those use cases in the doc.
- Repair some invalid hyperlinks in the doc.
- Put some `cfgs` inside functions to avoid boilerplate.
====================
Qingsong Chen (3):
rust: kernel: add ScatterList abstraction
rust: kernel: implement iterators for ScatterList
rust: kernel: add SgTable abstraction
rust/bindings/bindings_helper.h | 1 +
rust/helpers.c | 14 +
rust/kernel/lib.rs | 1 +
rust/kernel/scatterlist.rs | 502 ++++++++++++++++++++++++++++++++
4 files changed, 518 insertions(+)
create mode 100644 rust/kernel/scatterlist.rs
--
2.40.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-02 10:18 [PATCH v2 0/3] Rust scatterlist abstractions Qingsong Chen
@ 2023-06-02 10:18 ` Qingsong Chen
2023-06-02 19:54 ` Martin Rodriguez Reboredo
2023-06-05 15:26 ` Boqun Feng
2023-06-02 10:18 ` [PATCH v2 2/3] rust: kernel: implement iterators for ScatterList Qingsong Chen
2023-06-02 10:18 ` [PATCH v2 3/3] rust: kernel: add SgTable abstraction Qingsong Chen
2 siblings, 2 replies; 14+ messages in thread
From: Qingsong Chen @ 2023-06-02 10:18 UTC (permalink / raw)
To: linux-kernel
Cc: 田洪亮,
Qingsong Chen, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Martin Rodriguez Reboredo, Alice Ryhl, Asahi Lina,
rust-for-linux
Add a wrapper for `struct scatterlist`.
Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers.c | 14 ++
rust/kernel/lib.rs | 1 +
rust/kernel/scatterlist.rs | 280 ++++++++++++++++++++++++++++++++
4 files changed, 296 insertions(+)
create mode 100644 rust/kernel/scatterlist.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 50e7a76d5455..9cfa1ef92a04 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
#include <linux/refcount.h>
#include <linux/wait.h>
#include <linux/sched.h>
+#include <linux/scatterlist.h>
/* `bindgen` gets confused at certain things. */
const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..1714ed05f561 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -26,6 +26,7 @@
#include <linux/spinlock.h>
#include <linux/sched/signal.h>
#include <linux/wait.h>
+#include <linux/scatterlist.h>
__noreturn void rust_helper_BUG(void)
{
@@ -128,6 +129,19 @@ void rust_helper_put_task_struct(struct task_struct *t)
}
EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
+void rust_helper_sg_set_buf(struct scatterlist *sg, const void *buf,
+ unsigned int buflen)
+{
+ sg_set_buf(sg, buf, buflen);
+}
+EXPORT_SYMBOL_GPL(rust_helper_sg_set_buf);
+
+void *rust_helper_sg_virt(struct scatterlist *sg)
+{
+ return sg_virt(sg);
+}
+EXPORT_SYMBOL_GPL(rust_helper_sg_virt);
+
/*
* We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
* as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 676995d4e460..d8dbcde4ad5c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@ pub mod init;
pub mod ioctl;
pub mod prelude;
pub mod print;
+pub mod scatterlist;
mod static_assert;
#[doc(hidden)]
pub mod std_vendor;
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
new file mode 100644
index 000000000000..8d6a34afb191
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Scatterlist.
+//!
+//! C header: [`include/linux/scatterlist.h`](../../../../include/linux/scatterlist.h)
+
+use crate::prelude::*;
+use crate::types::Opaque;
+use core::marker::PhantomData;
+use core::ptr::NonNull;
+
+/// Wrap the kernel's `struct scatterlist`.
+///
+/// According to the design of kernel's `struct sg_table`, the `page_link`
+/// field of one `scatterlist` may contain a pointer to another. That is,
+/// there could exist external pointers to it, making it necessary to pin
+/// this struct.
+///
+/// # Invirants
+///
+/// All instances should be valid, either created by the `new` constructor
+/// (see [`pin_init`]), or transmuted from raw pointers (which could be used
+/// to reference a valid entry of `sg_table`).
+///
+/// # Examples
+///
+/// The following is some use cases of [`ScatterList`].
+///
+/// ```rust
+/// use core::pin::pin;
+/// # use kernel::error::Result;
+/// # use kernel::scatterlist::ScatterList;
+///
+/// // Prepare memory buffers.
+/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
+/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
+///
+/// // Allocates an instance on stack.
+/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf0));
+/// let mut foo: Pin<&mut ScatterList<'_>> = foo;
+/// assert_eq!(foo.length(), 512);
+///
+/// // Alloccate an instance by Box::pin_init.
+/// let bar: Pin<Box<ScatterList<'_>>> = Box::pin_init(ScatterList::new(&buf1))?;
+/// assert_eq!(bar.length(), 512);
+///
+/// // Assert other attributes of a instance.
+/// assert_eq!(foo.is_dma_bus_address(), false);
+/// assert_eq!(foo.is_chain(), false);
+/// assert_eq!(foo.is_last(), true);
+/// assert_eq!(foo.count(), 1);
+///
+/// // Get a immutable reference to memory buffer.
+/// assert_eq!(foo.get_buf(), [0u8; 512]);
+///
+/// // Reset the memory buffer.
+/// foo.set_buf(&buf1);
+/// assert_eq!(foo.get_buf(), [1u8; 512]);
+///
+/// // Get a mutable reference to memory buffer.
+/// foo.get_mut_buf().fill(2);
+/// assert_eq!(foo.get_buf(), [2u8; 512]);
+/// ```
+#[pin_data]
+pub struct ScatterList<'a> {
+ #[pin]
+ opaque: Opaque<bindings::scatterlist>,
+ _p: PhantomData<&'a mut bindings::scatterlist>,
+}
+
+impl<'a> ScatterList<'a> {
+ /// Construct a new initializer.
+ pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
+ // SAFETY: `slot` is valid while the closure is called, the memory
+ // buffer is pinned and valid.
+ unsafe {
+ init::pin_init_from_closure(move |slot: *mut Self| {
+ (*slot).set_buf(buf);
+ (*slot).mark_end();
+ Ok(())
+ })
+ }
+ }
+
+ /// Obtain a pinned reference to an immutable scatterlist from a raw pointer.
+ pub fn as_ref(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
+ // SAFETY: `sgl` is non-null and valid.
+ NonNull::new(ptr).map(|sgl| Pin::new(unsafe { &*(sgl.as_ptr() as *const ScatterList<'_>) }))
+ }
+
+ /// Obtain a pinned reference to a mutable scatterlist from a raw pointer.
+ pub fn as_mut(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
+ // SAFETY: `sgl` is non-null and valid.
+ NonNull::new(ptr)
+ .map(|sgl| Pin::new(unsafe { &mut *(sgl.as_ptr() as *mut ScatterList<'_>) }))
+ }
+}
+
+impl ScatterList<'_> {
+ /// Return the offset of memory buffer into the page.
+ pub fn offset(&self) -> usize {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe { (*self.opaque.get()).offset as _ }
+ }
+
+ /// Return the length of memory buffer.
+ pub fn length(&self) -> usize {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe { (*self.opaque.get()).length as _ }
+ }
+
+ /// Return the mapped DMA address.
+ ///
+ /// # Safety
+ ///
+ /// It is only valid after this scatterlist has been mapped to some bus
+ /// address and then called `set_dma` method to setup it.
+ pub fn dma_address(&self) -> usize {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe { (*self.opaque.get()).dma_address as _ }
+ }
+
+ /// Return the mapped DMA length.
+ ///
+ /// # Safety
+ ///
+ /// It is only valid after this scatterlist has been mapped to some bus
+ /// address and then called `set_dma` method to setup it.
+ pub fn dma_length(&self) -> usize {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
+ unsafe {
+ (*self.opaque.get()).dma_length as _
+ }
+ #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
+ unsafe {
+ (*self.opaque.get()).length as _
+ }
+ }
+
+ /// Setup the DMA address and length.
+ pub fn set_dma(&mut self, addr: usize, len: usize) {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
+ unsafe {
+ (*self.opaque.get()).dma_address = addr as _;
+ (*self.opaque.get()).dma_length = len as _;
+ }
+ #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
+ unsafe {
+ (*self.opaque.get()).dma_address = addr as _;
+ (*self.opaque.get()).length = len as _;
+ }
+ self.dma_mark_bus_address();
+ }
+
+ /// Return `true` if it is mapped with a DMA address.
+ pub fn is_dma_bus_address(&self) -> bool {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ #[cfg(CONFIG_PCI_P2PDMA)]
+ unsafe {
+ ((*self.opaque.get()).dma_flags & bindings::SG_DMA_BUS_ADDRESS) != 0
+ }
+ #[cfg(not(CONFIG_PCI_P2PDMA))]
+ false
+ }
+
+ /// Mark it as mapped to a DMA address.
+ pub fn dma_mark_bus_address(&mut self) {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ #[cfg(CONFIG_PCI_P2PDMA)]
+ unsafe {
+ (*self.opaque.get()).dma_flags |= bindings::SG_DMA_BUS_ADDRESS;
+ }
+ }
+
+ /// Mark it as `not` mapped to a DMA address.
+ pub fn dma_unmark_bus_address(&mut self) {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ #[cfg(CONFIG_PCI_P2PDMA)]
+ unsafe {
+ (*self.opaque.get()).dma_flags &= !bindings::SG_DMA_BUS_ADDRESS;
+ }
+ }
+
+ /// Return `true` if it is a chainable entry.
+ pub fn is_chain(&self) -> bool {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe {
+ ((*self.opaque.get()).page_link
+ & (bindings::SG_PAGE_LINK_MASK as u64)
+ & (bindings::SG_CHAIN as u64))
+ != 0
+ }
+ }
+
+ /// Transfer this scatterlist as a chainable entry, linked to `sgl`.
+ pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) {
+ let addr = sgl.opaque.get() as u64;
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe {
+ (*self.opaque.get()).offset = 0;
+ (*self.opaque.get()).length = 0;
+ (*self.opaque.get()).page_link = addr | (bindings::SG_CHAIN as u64);
+ }
+ self.unmark_end();
+ }
+
+ /// Return `true` if it is the last normal entry in scatterlist.
+ pub fn is_last(&self) -> bool {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe {
+ ((*self.opaque.get()).page_link
+ & (bindings::SG_PAGE_LINK_MASK as u64)
+ & (bindings::SG_END as u64))
+ != 0
+ }
+ }
+
+ /// Mark it as the last normal entry in scatterlist.
+ pub fn mark_end(&mut self) {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe {
+ (*self.opaque.get()).page_link |= bindings::SG_END as u64;
+ (*self.opaque.get()).page_link &= !(bindings::SG_CHAIN as u64);
+ }
+ }
+
+ /// Mark it as `not` the last normal entry in scatterlist.
+ pub fn unmark_end(&mut self) {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe {
+ (*self.opaque.get()).page_link &= !(bindings::SG_END as u64);
+ }
+ }
+
+ /// Get an immutable reference to memory buffer.
+ pub fn get_buf(&self) -> &[u8] {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ let addr = unsafe { bindings::sg_virt(self.opaque.get()) };
+ let len = self.length();
+ // SAFETY: `addr` is returned by `sg_virt`, so it is valid. And the memory
+ // buffer is set by `new` constructor or `set_buf` method, so it's pinned
+ // and valid.
+ unsafe { core::slice::from_raw_parts(addr as _, len) }
+ }
+
+ /// Get a mutable reference to memory buffer.
+ pub fn get_mut_buf(&mut self) -> &mut [u8] {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ let addr = unsafe { bindings::sg_virt(self.opaque.get()) };
+ let len = self.length();
+ // SAFETY: `addr` is returned by `sg_virt`, so it is valid. And the memory
+ // buffer is set by `new` constructor or `set_buf` method, so it's pinned
+ // and valid.
+ unsafe { core::slice::from_raw_parts_mut(addr as _, len) }
+ }
+
+ /// Set the memory buffer.
+ pub fn set_buf(&mut self, buf: &Pin<&mut [u8]>) {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ // And `buf` is pinned and valid.
+ unsafe {
+ bindings::sg_set_buf(
+ self.opaque.get(),
+ buf.as_ptr() as *const core::ffi::c_void,
+ buf.len() as core::ffi::c_uint,
+ );
+ }
+ }
+
+ /// Return the total count of normal entries in scatterlist.
+ ///
+ /// Allows to know how many entries are in scatterlist, taking into account
+ /// chaining as well.
+ pub fn count(&self) -> usize {
+ // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+ unsafe { bindings::sg_nents(self.opaque.get()) as _ }
+ }
+}
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] rust: kernel: implement iterators for ScatterList
2023-06-02 10:18 [PATCH v2 0/3] Rust scatterlist abstractions Qingsong Chen
2023-06-02 10:18 ` [PATCH v2 1/3] rust: kernel: add ScatterList abstraction Qingsong Chen
@ 2023-06-02 10:18 ` Qingsong Chen
2023-06-02 19:59 ` Martin Rodriguez Reboredo
2023-06-02 10:18 ` [PATCH v2 3/3] rust: kernel: add SgTable abstraction Qingsong Chen
2 siblings, 1 reply; 14+ messages in thread
From: Qingsong Chen @ 2023-06-02 10:18 UTC (permalink / raw)
To: linux-kernel
Cc: 田洪亮,
Qingsong Chen, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
rust-for-linux
ScatterList could be transmuted from raw pointers of a valid
`sg_table`. Then we can use those iterators to access the
following normal entries.
Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
rust/kernel/scatterlist.rs | 50 ++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
index 8d6a34afb191..292fcb63a329 100644
--- a/rust/kernel/scatterlist.rs
+++ b/rust/kernel/scatterlist.rs
@@ -277,4 +277,54 @@ impl ScatterList<'_> {
// SAFETY: By the type invariant, we know that `self.opaque` is valid.
unsafe { bindings::sg_nents(self.opaque.get()) as _ }
}
+
+ /// Get an iterator for immutable references.
+ pub fn iter(&self) -> Iter<'_> {
+ Iter(ScatterList::as_ref(self.opaque.get()))
+ }
+
+ /// Get an iterator for mutable references.
+ pub fn iter_mut(&mut self) -> IterMut<'_> {
+ IterMut(ScatterList::as_mut(self.opaque.get()))
+ }
+}
+
+/// An iterator that yields [`Pin<&ScatterList>`].
+///
+/// Only iterate normal scatterlist entries, chainable entry will be skipped.
+pub struct Iter<'a>(Option<Pin<&'a ScatterList<'a>>>);
+
+impl<'a> Iterator for Iter<'a> {
+ type Item = Pin<&'a ScatterList<'a>>;
+
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.0.is_none() {
+ return None;
+ }
+ let ptr = self.0.as_ref().unwrap().opaque.get();
+ // SAFETY: `ptr` is from `self.opaque`, it is valid by the type invariant.
+ let next = unsafe { bindings::sg_next(ptr) };
+ self.0 = ScatterList::as_ref(next);
+ ScatterList::as_ref(ptr)
+ }
+}
+
+/// An iterator that yields [`Pin<&mut ScatterList>`].
+///
+/// Only iterate normal scatterlist entries, chainable entry will be skipped.
+pub struct IterMut<'a>(Option<Pin<&'a mut ScatterList<'a>>>);
+
+impl<'a> Iterator for IterMut<'a> {
+ type Item = Pin<&'a mut ScatterList<'a>>;
+
+ fn next(&mut self) -> Option<Self::Item> {
+ if self.0.is_none() {
+ return None;
+ }
+ let ptr = self.0.as_ref().unwrap().opaque.get();
+ // SAFETY: `ptr` is from `self.opaque`, it is valid by the type invariant.
+ let next = unsafe { bindings::sg_next(ptr) };
+ self.0 = ScatterList::as_mut(next);
+ ScatterList::as_mut(ptr)
+ }
}
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] rust: kernel: add SgTable abstraction
2023-06-02 10:18 [PATCH v2 0/3] Rust scatterlist abstractions Qingsong Chen
2023-06-02 10:18 ` [PATCH v2 1/3] rust: kernel: add ScatterList abstraction Qingsong Chen
2023-06-02 10:18 ` [PATCH v2 2/3] rust: kernel: implement iterators for ScatterList Qingsong Chen
@ 2023-06-02 10:18 ` Qingsong Chen
2023-06-02 20:47 ` Martin Rodriguez Reboredo
2 siblings, 1 reply; 14+ messages in thread
From: Qingsong Chen @ 2023-06-02 10:18 UTC (permalink / raw)
To: linux-kernel
Cc: 田洪亮,
Qingsong Chen, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
rust-for-linux
SgTable is similar to `struct sg_table`, consisted of
scatterlist entries, and could be chained with each other.
Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
rust/kernel/scatterlist.rs | 172 +++++++++++++++++++++++++++++++++++++
1 file changed, 172 insertions(+)
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
index 292fcb63a329..fd30f61a9743 100644
--- a/rust/kernel/scatterlist.rs
+++ b/rust/kernel/scatterlist.rs
@@ -328,3 +328,175 @@ impl<'a> Iterator for IterMut<'a> {
ScatterList::as_mut(ptr)
}
}
+
+/// A [`ScatterList`] table of fixed `N` entries.
+///
+/// According to the design of kernel's `struct sg_table`, the `page_link`
+/// field of one `scatterlist` may contain a pointer to another. That is,
+/// there could exist external pointers to it, making it necessary to pin
+/// this struct.
+///
+/// If the table is chainable, the last entry will be reserved for chainning.
+/// The recommended way to create such instances is with the [`pin_init`].
+///
+/// # Examples
+///
+/// The following is some use cases of [`SgTable<N>`].
+///
+/// ```rust
+/// use core::pin::pin;;
+/// # use kernel::error::Result;
+/// # use kernel::scatterlist::{ScatterList, SgTable};
+///
+/// // Prepare memory buffers.
+/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
+/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
+/// let mut entries: Vec<Pin<&mut [u8]>> = Vec::<Pin<&mut [u8]>>::new();
+/// entries.try_push(buf0)?;
+/// entries.try_push(buf1)?;
+///
+/// // Allocates an instance on stack.
+/// kernel::stack_try_pin_init!(let foo =? SgTable::new(entries.as_slice(), false));
+/// let mut foo: Pin<&mut SgTable<'_, 2>> = foo;
+/// assert_eq!(foo.count(), 2);
+///
+/// // Alloccate a chainable instance by Box::pin_init.
+/// let mut bar: Pin<Box<SgTable<'_, 3>>> = Box::pin_init(SgTable::new(entries.as_slice(), true))?;
+/// assert_eq!(bar.count(), 2);
+///
+/// // Chain two `SgTable` together.
+/// bar.chain_sgt(foo.as_mut())?;
+/// assert_eq!(bar.count(), 4);
+///
+/// // Get the immutable reference to the entry in the table.
+/// let sgl1: Pin<&ScatterList<'_>> = bar.get(1).ok_or(EINVAL)?;
+/// assert_eq!(sgl1.count(), 3);
+/// assert_eq!(sgl1.get_buf(), [1u8; 512]);
+///
+/// // Get the mutable reference to the entry in the table.
+/// let sgl2: Pin<&mut ScatterList<'_>> = bar.get_mut(2).ok_or(EINVAL)?;
+/// assert_eq!(sgl2.count(), 2);
+/// assert_eq!(sgl2.get_buf(), [0u8; 512]);
+///
+/// // Try to get a non-exist entry from the table.
+/// let sgl4 = bar.get(4);
+/// assert_eq!(sgl4.is_none(), true);
+///
+/// // Split the first table from chained scatterlist.
+/// bar.split()?;
+/// assert_eq!(bar.count(), 2);
+/// ```
+#[pin_data]
+pub struct SgTable<'a, const N: usize> {
+ #[pin]
+ entries: [ScatterList<'a>; N],
+}
+
+impl<'a, const N: usize> SgTable<'a, N> {
+ /// Construct a new initializer.
+ ///
+ /// The length of `entries` should exactly be the available size of [`SgTable<N>`].
+ /// If the table is `chainable`, the available size is `N - 1`, because one entry
+ /// should be reserved for chainning.
+ pub fn new(
+ entries: &'a [Pin<&mut [u8]>],
+ chainable: bool,
+ ) -> impl PinInit<SgTable<'a, N>, Error> {
+ build_assert!(N > 0);
+ // SAFETY: `slot` is valid while the closure is called, the `entries` are
+ // pinned and valid.
+ unsafe {
+ init::pin_init_from_closure(move |slot: *mut Self| {
+ let mut nr_entry = N;
+ if chainable {
+ nr_entry -= 1;
+ }
+ if nr_entry == 0 || entries.len() != nr_entry {
+ return Err(EINVAL);
+ }
+
+ for i in 0..nr_entry {
+ let sgl = &mut (*slot).entries[i];
+ sgl.set_buf(&entries[i]);
+ if i + 1 == nr_entry {
+ sgl.mark_end();
+ }
+ }
+ Ok(())
+ })
+ }
+ }
+
+ /// Chain two [`SgTable`] together.
+ ///
+ /// Transfer the last entry of this table as a chainable pointer to the
+ /// first entry of `sgt` SgTable.
+ pub fn chain_sgt<const M: usize>(&mut self, sgt: Pin<&mut SgTable<'_, M>>) -> Result {
+ if self.count() != N - 1 {
+ return Err(EINVAL);
+ }
+ self.entries[N - 2].unmark_end();
+
+ let next = ScatterList::as_mut(sgt.entries[0].opaque.get()).ok_or(EINVAL)?;
+ self.entries[N - 1].chain_sgl(next);
+ Ok(())
+ }
+
+ /// Chain [`SgTable`] and [`ScatterList`] together.
+ ///
+ /// Transfer the last entry of this table as a chainable pointer to `sgl`
+ /// scatterlist.
+ pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) -> Result {
+ if self.count() != N - 1 {
+ return Err(EINVAL);
+ }
+ self.entries[N - 2].unmark_end();
+ self.entries[N - 1].chain_sgl(sgl);
+ Ok(())
+ }
+
+ /// Split the first table from chained scatterlist.
+ pub fn split(&mut self) -> Result {
+ if !self.entries[N - 1].is_chain() {
+ return Err(EINVAL);
+ }
+ self.entries[N - 2].mark_end();
+ Ok(())
+ }
+
+ /// Return the total count of normal entries in the table.
+ ///
+ /// Allows to know how many entries are in sg, taking into account
+ /// chaining as well.
+ pub fn count(&self) -> usize {
+ // SAFETY: `self.entries` are initialized by the `new` constructor,
+ // so it's valid.
+ unsafe { bindings::sg_nents(self.entries[0].opaque.get()) as _ }
+ }
+
+ /// Get the immutable reference to `n`th entry in the table.
+ ///
+ /// Like most indexing operations, the count starts from zero. Return None
+ /// if `n` is greater than or equal to the total count of entries.
+ pub fn get(&self, n: usize) -> Option<Pin<&ScatterList<'_>>> {
+ self.iter().nth(n)
+ }
+
+ /// Get the mutable reference to `n`th entry in the table.
+ ///
+ /// Like most indexing operations, the count starts from zero. Return None
+ /// if `n` is greater than or equal to the number of total entries.
+ pub fn get_mut(&mut self, n: usize) -> Option<Pin<&mut ScatterList<'_>>> {
+ self.iter_mut().nth(n)
+ }
+
+ /// Get an iterator for immutable entries.
+ pub fn iter(&self) -> Iter<'_> {
+ Iter(ScatterList::as_ref(self.entries[0].opaque.get()))
+ }
+
+ /// Get an iterator for mutable entries.
+ pub fn iter_mut(&mut self) -> IterMut<'_> {
+ IterMut(ScatterList::as_mut(self.entries[0].opaque.get()))
+ }
+}
--
2.40.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-02 10:18 ` [PATCH v2 1/3] rust: kernel: add ScatterList abstraction Qingsong Chen
@ 2023-06-02 19:54 ` Martin Rodriguez Reboredo
2023-06-05 8:12 ` Qingsong Chen
2023-06-05 11:36 ` Miguel Ojeda
2023-06-05 15:26 ` Boqun Feng
1 sibling, 2 replies; 14+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-06-02 19:54 UTC (permalink / raw)
To: Qingsong Chen, linux-kernel
Cc: 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
Asahi Lina, rust-for-linux
On 6/2/23 07:18, Qingsong Chen wrote:
> Add a wrapper for `struct scatterlist`.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]
> +/// Wrap the kernel's `struct scatterlist`.
> +///
> +/// According to the design of kernel's `struct sg_table`, the `page_link`
> +/// field of one `scatterlist` may contain a pointer to another. That is,
> +/// there could exist external pointers to it, making it necessary to pin
> +/// this struct.
> +///
> +/// # Invirants
> +///
> +/// All instances should be valid, either created by the `new` constructor
> +/// (see [`pin_init`]), or transmuted from raw pointers (which could be used
> +/// to reference a valid entry of `sg_table`).
> +///
> +/// # Examples
> +///
> +/// The following is some use cases of [`ScatterList`].
> +///
> +/// ```rust
> +/// use core::pin::pin;
> +/// # use kernel::error::Result;
> +/// # use kernel::scatterlist::ScatterList;
> +///
> +/// // Prepare memory buffers.
> +/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
> +/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
> +///
> +/// // Allocates an instance on stack.
> +/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf0));
> +/// let mut foo: Pin<&mut ScatterList<'_>> = foo;
> +/// assert_eq!(foo.length(), 512);
> +///
> +/// // Alloccate an instance by Box::pin_init.
> +/// let bar: Pin<Box<ScatterList<'_>>> = Box::pin_init(ScatterList::new(&buf1))?;
> +/// assert_eq!(bar.length(), 512);
> +///
> +/// // Assert other attributes of a instance.
> +/// assert_eq!(foo.is_dma_bus_address(), false);
> +/// assert_eq!(foo.is_chain(), false);
> +/// assert_eq!(foo.is_last(), true);
> +/// assert_eq!(foo.count(), 1);
> +///
> +/// // Get a immutable reference to memory buffer.
> +/// assert_eq!(foo.get_buf(), [0u8; 512]);
> +///
> +/// // Reset the memory buffer.
> +/// foo.set_buf(&buf1);
> +/// assert_eq!(foo.get_buf(), [1u8; 512]);
> +///
> +/// // Get a mutable reference to memory buffer.
> +/// foo.get_mut_buf().fill(2);
> +/// assert_eq!(foo.get_buf(), [2u8; 512]);
> +/// ```
This comment has some typos, but luckily there's the `typos` tool [1]
out there to help us.
> +#[pin_data]
> +pub struct ScatterList<'a> {
> + #[pin]
> + opaque: Opaque<bindings::scatterlist>,
> + _p: PhantomData<&'a mut bindings::scatterlist>,
> +}
> +
> +impl<'a> ScatterList<'a> {
> [...]
> +
> + /// Obtain a pinned reference to an immutable scatterlist from a raw pointer.
> + pub fn as_ref(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
> + // SAFETY: `sgl` is non-null and valid.
> + NonNull::new(ptr).map(|sgl| Pin::new(unsafe { &*(sgl.as_ptr() as *const ScatterList<'_>) }))
> + }
> +
> + /// Obtain a pinned reference to a mutable scatterlist from a raw pointer.
> + pub fn as_mut(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
> + // SAFETY: `sgl` is non-null and valid.
> + NonNull::new(ptr)
> + .map(|sgl| Pin::new(unsafe { &mut *(sgl.as_ptr() as *mut ScatterList<'_>) }))
> + }
Please mark both of these as `unsafe` as you can still pass an invalid
pointer to them. Also, if there's no other user of those methods other
than this module then I'd suggest to make them private.
> +}
> +
> [...]
Link: https://github.com/crate-ci/typos [1]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] rust: kernel: implement iterators for ScatterList
2023-06-02 10:18 ` [PATCH v2 2/3] rust: kernel: implement iterators for ScatterList Qingsong Chen
@ 2023-06-02 19:59 ` Martin Rodriguez Reboredo
0 siblings, 0 replies; 14+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-06-02 19:59 UTC (permalink / raw)
To: Qingsong Chen, linux-kernel
Cc: 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, rust-for-linux
On 6/2/23 07:18, Qingsong Chen wrote:
> ScatterList could be transmuted from raw pointers of a valid
> `sg_table`. Then we can use those iterators to access the
> following normal entries.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]
> + /// Get an iterator for immutable references.
> + pub fn iter(&self) -> Iter<'_> {
> + Iter(ScatterList::as_ref(self.opaque.get()))
> + }
> +
> + /// Get an iterator for mutable references.
> + pub fn iter_mut(&mut self) -> IterMut<'_> {
> + IterMut(ScatterList::as_mut(self.opaque.get()))
> + }
Per the previous commit, because both methods are unsafe you should put
the `SAFETY` comment on them.
> +}
> +
> [...]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] rust: kernel: add SgTable abstraction
2023-06-02 10:18 ` [PATCH v2 3/3] rust: kernel: add SgTable abstraction Qingsong Chen
@ 2023-06-02 20:47 ` Martin Rodriguez Reboredo
0 siblings, 0 replies; 14+ messages in thread
From: Martin Rodriguez Reboredo @ 2023-06-02 20:47 UTC (permalink / raw)
To: Qingsong Chen, linux-kernel
Cc: 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, rust-for-linux
On 6/2/23 07:18, Qingsong Chen wrote:
> SgTable is similar to `struct sg_table`, consisted of
> scatterlist entries, and could be chained with each other.
>
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]
> +
> +impl<'a, const N: usize> SgTable<'a, N> {
> [...]
> +
> + /// Chain two [`SgTable`] together.
> + ///
> + /// Transfer the last entry of this table as a chainable pointer to the
> + /// first entry of `sgt` SgTable.
> + pub fn chain_sgt<const M: usize>(&mut self, sgt: Pin<&mut SgTable<'_, M>>) -> Result {
> + if self.count() != N - 1 {
> + return Err(EINVAL);
> + }
> + self.entries[N - 2].unmark_end();
> +
> + let next = ScatterList::as_mut(sgt.entries[0].opaque.get()).ok_or(EINVAL)?;
> + self.entries[N - 1].chain_sgl(next);
> + Ok(())
> + }
> +
> + /// Chain [`SgTable`] and [`ScatterList`] together.
> + ///
> + /// Transfer the last entry of this table as a chainable pointer to `sgl`
> + /// scatterlist.
> + pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) -> Result {
> + if self.count() != N - 1 {
> + return Err(EINVAL);
> + }
> + self.entries[N - 2].unmark_end();
> + self.entries[N - 1].chain_sgl(sgl);
> + Ok(())
> + }
> +
> + /// Split the first table from chained scatterlist.
> + pub fn split(&mut self) -> Result {
> + if !self.entries[N - 1].is_chain() {
> + return Err(EINVAL);
> + }
> + self.entries[N - 2].mark_end();
> + Ok(())
> + }
To these methods I'd suggest adding an `# Errors` section in the doc
comment to explain why would you get an `EINVAL`.
> +
> [...]
> +
> + /// Get an iterator for immutable entries.
> + pub fn iter(&self) -> Iter<'_> {
> + Iter(ScatterList::as_ref(self.entries[0].opaque.get()))
> + }
> +
> + /// Get an iterator for mutable entries.
> + pub fn iter_mut(&mut self) -> IterMut<'_> {
> + IterMut(ScatterList::as_mut(self.entries[0].opaque.get()))
> + }
Same as the previous commit, the `SAFETY` comment.
> +}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-02 19:54 ` Martin Rodriguez Reboredo
@ 2023-06-05 8:12 ` Qingsong Chen
2023-06-05 11:36 ` Miguel Ojeda
1 sibling, 0 replies; 14+ messages in thread
From: Qingsong Chen @ 2023-06-05 8:12 UTC (permalink / raw)
To: Martin Rodriguez Reboredo, linux-kernel
Cc: 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
Asahi Lina, rust-for-linux
On 6/3/23 3:54 AM, Martin Rodriguez Reboredo wrote:
> On 6/2/23 07:18, Qingsong Chen wrote:
>> +/// # Invirants
>> +///
>> +/// All instances should be valid, either created by the `new` constructor
>> +/// (see [`pin_init`]), or transmuted from raw pointers (which could be used
>> +/// to reference a valid entry of `sg_table`).
>> +///
>> +/// # Examples
>> +///
>> +/// The following is some use cases of [`ScatterList`].
>> +///
>> +/// ```rust
>> +/// use core::pin::pin;
>> +/// # use kernel::error::Result;
>> +/// # use kernel::scatterlist::ScatterList;
>> +///
>> +/// // Prepare memory buffers.
>> +/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
>> +/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
>> +///
>> +/// // Allocates an instance on stack.
>> +/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf0));
>> +/// let mut foo: Pin<&mut ScatterList<'_>> = foo;
>> +/// assert_eq!(foo.length(), 512);
>> +///
>> +/// // Alloccate an instance by Box::pin_init.
>> +/// let bar: Pin<Box<ScatterList<'_>>> = Box::pin_init(ScatterList::new(&buf1))?;
>> +/// assert_eq!(bar.length(), 512);
>
> This comment has some typos, but luckily there's the `typos` tool [1]
> out there to help us.
>
I just tried the `typos` tool, but it didn't report any errors. Though,
I would try other spell checker, with more caution. Thanks.
>> + /// Obtain a pinned reference to an immutable scatterlist from a raw pointer.
>> + pub fn as_ref(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
>> + // SAFETY: `sgl` is non-null and valid.
>> + NonNull::new(ptr).map(|sgl| Pin::new(unsafe { &*(sgl.as_ptr() as *const ScatterList<'_>) }))
>> + }
>> +
>> + /// Obtain a pinned reference to a mutable scatterlist from a raw pointer.
>> + pub fn as_mut(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
>> + // SAFETY: `sgl` is non-null and valid.
>> + NonNull::new(ptr)
>> + .map(|sgl| Pin::new(unsafe { &mut *(sgl.as_ptr() as *mut ScatterList<'_>) }))
>> + }
>
> Please mark both of these as `unsafe` as you can still pass an invalid
> pointer to them. Also, if there's no other user of those methods other
> than this module then I'd suggest to make them private.
>
Those methods are only used by `SgTable` and the iterators by design.
And it would be better if they were private and marked as unsafe. Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-02 19:54 ` Martin Rodriguez Reboredo
2023-06-05 8:12 ` Qingsong Chen
@ 2023-06-05 11:36 ` Miguel Ojeda
1 sibling, 0 replies; 14+ messages in thread
From: Miguel Ojeda @ 2023-06-05 11:36 UTC (permalink / raw)
To: Martin Rodriguez Reboredo
Cc: Qingsong Chen, linux-kernel, 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Alice Ryhl,
Asahi Lina, rust-for-linux
On Fri, Jun 2, 2023 at 10:47 PM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
>
> This comment has some typos, but luckily there's the `typos` tool [1]
> out there to help us.
`scripts/checkpatch.pl` has `--codespell`, but the main dictionary
does not have it, so I sent:
https://github.com/codespell-project/codespell/pull/2869
https://github.com/codespell-project/codespell/pull/2870
Since `typos` appears to import `codespell` main dictionary, I guess
that will eventually help both tools.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-02 10:18 ` [PATCH v2 1/3] rust: kernel: add ScatterList abstraction Qingsong Chen
2023-06-02 19:54 ` Martin Rodriguez Reboredo
@ 2023-06-05 15:26 ` Boqun Feng
2023-06-06 7:01 ` Qingsong Chen
2023-06-07 18:42 ` Gary Guo
1 sibling, 2 replies; 14+ messages in thread
From: Boqun Feng @ 2023-06-05 15:26 UTC (permalink / raw)
To: Qingsong Chen
Cc: linux-kernel, 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Asahi Lina, rust-for-linux
On Fri, Jun 02, 2023 at 06:18:17PM +0800, Qingsong Chen wrote:
[...]
> +impl<'a> ScatterList<'a> {
> + /// Construct a new initializer.
> + pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
> + // SAFETY: `slot` is valid while the closure is called, the memory
> + // buffer is pinned and valid.
> + unsafe {
> + init::pin_init_from_closure(move |slot: *mut Self| {
> + (*slot).set_buf(buf);
> + (*slot).mark_end();
Benno can provide more information, but you cannot dereference or create
a reference to `*slot`, since `slot` points to an uninitialized object
(see `try_pin_init` implementations), and referencing uninitialized
objects is UB (or may cause UB).
Note that you could do the following for `set_buf`:
// `addr_of!`[1] is special since it won't create references
// (even temporary onces).
let opaque = addr_of!((*slot).opaque); // <- *const Opaque<bindings::scatterlist>
let ptr = Opaque::raw_get(opaque); // <- *mut bindings::scatterlist
// Maybe this can be wrapped as a Rust function with a
// parameter: *mut bindings::scatterlist.
unsafe {
bindings::sg_set_buf(ptr, buf.as_ptr(), buf.len());
}
[1]: https://doc.rust-lang.org/core/ptr/macro.addr_of.html
Regards,
Boqun
> + Ok(())
> + })
> + }
> + }
> +
[...]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-05 15:26 ` Boqun Feng
@ 2023-06-06 7:01 ` Qingsong Chen
2023-06-07 17:18 ` Boqun Feng
2023-06-07 18:42 ` Gary Guo
1 sibling, 1 reply; 14+ messages in thread
From: Qingsong Chen @ 2023-06-06 7:01 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-kernel, 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Asahi Lina, rust-for-linux
On 6/5/23 11:26 PM, Boqun Feng wrote:
> On Fri, Jun 02, 2023 at 06:18:17PM +0800, Qingsong Chen wrote:
> [...]
>> +impl<'a> ScatterList<'a> {
>> + /// Construct a new initializer.
>> + pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
>> + // SAFETY: `slot` is valid while the closure is called, the memory
>> + // buffer is pinned and valid.
>> + unsafe {
>> + init::pin_init_from_closure(move |slot: *mut Self| {
>> + (*slot).set_buf(buf);
>> + (*slot).mark_end();
>
> Benno can provide more information, but you cannot dereference or create
> a reference to `*slot`, since `slot` points to an uninitialized object
> (see `try_pin_init` implementations), and referencing uninitialized
> objects is UB (or may cause UB).
I understand that `reading` from uninitialized objects is UB, either
via references or raw pointers. But in this unsafe closure, we just do
the `writing` job to `slot` for initializing. This makes me a little
confused, why is there a difference between reference and raw pointer?
Is there any compiler magic on reference which may cause UB? Still, I
would fix this by `addr_of`. Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-06 7:01 ` Qingsong Chen
@ 2023-06-07 17:18 ` Boqun Feng
0 siblings, 0 replies; 14+ messages in thread
From: Boqun Feng @ 2023-06-07 17:18 UTC (permalink / raw)
To: Qingsong Chen
Cc: linux-kernel, 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Asahi Lina, rust-for-linux
On Tue, Jun 06, 2023 at 03:01:30PM +0800, Qingsong Chen wrote:
> On 6/5/23 11:26 PM, Boqun Feng wrote:
> > On Fri, Jun 02, 2023 at 06:18:17PM +0800, Qingsong Chen wrote:
> > [...]
> > > +impl<'a> ScatterList<'a> {
> > > + /// Construct a new initializer.
> > > + pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
> > > + // SAFETY: `slot` is valid while the closure is called, the memory
> > > + // buffer is pinned and valid.
> > > + unsafe {
> > > + init::pin_init_from_closure(move |slot: *mut Self| {
> > > + (*slot).set_buf(buf);
> > > + (*slot).mark_end();
> >
> > Benno can provide more information, but you cannot dereference or create
> > a reference to `*slot`, since `slot` points to an uninitialized object
> > (see `try_pin_init` implementations), and referencing uninitialized
> > objects is UB (or may cause UB).
>
> I understand that `reading` from uninitialized objects is UB, either
> via references or raw pointers. But in this unsafe closure, we just do
> the `writing` job to `slot` for initializing. This makes me a little
> confused, why is there a difference between reference and raw pointer?
References (or the existence of the references) mean the underlying
objects are valid:
https://doc.rust-lang.org/std/primitive.reference.html
, so creating a reference (even a temporary one) to an uninitialized is
an UB, that being said..
> Is there any compiler magic on reference which may cause UB? Still, I
I'm not creative enough to come up with a compiler optimization that
will (ab)use this UB to do evil if you can be sure that set_buf() and
mark_end() are purely writes. So your confusion looks reasonable to me
;-) But still it's an UB. And I guess you don't want to be suprised by
any future compiler optimization, right? ;-)
Regards,
Boqun
> would fix this by `addr_of`. Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-05 15:26 ` Boqun Feng
2023-06-06 7:01 ` Qingsong Chen
@ 2023-06-07 18:42 ` Gary Guo
2023-06-07 19:40 ` Boqun Feng
1 sibling, 1 reply; 14+ messages in thread
From: Gary Guo @ 2023-06-07 18:42 UTC (permalink / raw)
To: Boqun Feng
Cc: Qingsong Chen, linux-kernel, 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Asahi Lina, rust-for-linux
On Mon, 5 Jun 2023 08:26:04 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:
> On Fri, Jun 02, 2023 at 06:18:17PM +0800, Qingsong Chen wrote:
> [...]
> > +impl<'a> ScatterList<'a> {
> > + /// Construct a new initializer.
> > + pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
> > + // SAFETY: `slot` is valid while the closure is called, the memory
> > + // buffer is pinned and valid.
> > + unsafe {
> > + init::pin_init_from_closure(move |slot: *mut Self| {
> > + (*slot).set_buf(buf);
> > + (*slot).mark_end();
>
> Benno can provide more information, but you cannot dereference or create
> a reference to `*slot`, since `slot` points to an uninitialized object
> (see `try_pin_init` implementations), and referencing uninitialized
> objects is UB (or may cause UB).
This is fine for `Self`, because the only non-ZST field in there is
`Opaque`, which can be uninitialised.
>
> Note that you could do the following for `set_buf`:
>
> // `addr_of!`[1] is special since it won't create references
> // (even temporary onces).
> let opaque = addr_of!((*slot).opaque); // <- *const Opaque<bindings::scatterlist>
>
> let ptr = Opaque::raw_get(opaque); // <- *mut bindings::scatterlist
>
> // Maybe this can be wrapped as a Rust function with a
> // parameter: *mut bindings::scatterlist.
> unsafe {
> bindings::sg_set_buf(ptr, buf.as_ptr(), buf.len());
> }
>
> [1]: https://doc.rust-lang.org/core/ptr/macro.addr_of.html
>
> Regards,
> Boqun
>
> > + Ok(())
> > + })
> > + }
> > + }
> > +
> [...]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction
2023-06-07 18:42 ` Gary Guo
@ 2023-06-07 19:40 ` Boqun Feng
0 siblings, 0 replies; 14+ messages in thread
From: Boqun Feng @ 2023-06-07 19:40 UTC (permalink / raw)
To: Gary Guo
Cc: Qingsong Chen, linux-kernel, 田洪亮,
Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
Björn Roy Baron, Benno Lossin, Martin Rodriguez Reboredo,
Alice Ryhl, Asahi Lina, rust-for-linux
On Wed, Jun 07, 2023 at 07:42:10PM +0100, Gary Guo wrote:
> On Mon, 5 Jun 2023 08:26:04 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
>
> > On Fri, Jun 02, 2023 at 06:18:17PM +0800, Qingsong Chen wrote:
> > [...]
> > > +impl<'a> ScatterList<'a> {
> > > + /// Construct a new initializer.
> > > + pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
> > > + // SAFETY: `slot` is valid while the closure is called, the memory
> > > + // buffer is pinned and valid.
> > > + unsafe {
> > > + init::pin_init_from_closure(move |slot: *mut Self| {
> > > + (*slot).set_buf(buf);
> > > + (*slot).mark_end();
> >
> > Benno can provide more information, but you cannot dereference or create
> > a reference to `*slot`, since `slot` points to an uninitialized object
> > (see `try_pin_init` implementations), and referencing uninitialized
> > objects is UB (or may cause UB).
>
> This is fine for `Self`, because the only non-ZST field in there is
> `Opaque`, which can be uninitialised.
>
OK, but if you do a
// this is OK
let slot_ref = unsafe { &(*slot) };
// reads uninitialised data.
let o = slot_ref.offset();
here (before any `set_buf()` or `mark_end()`), isn't it an UB?
Regards,
Boqun
> >
> > Note that you could do the following for `set_buf`:
> >
> > // `addr_of!`[1] is special since it won't create references
> > // (even temporary onces).
> > let opaque = addr_of!((*slot).opaque); // <- *const Opaque<bindings::scatterlist>
> >
> > let ptr = Opaque::raw_get(opaque); // <- *mut bindings::scatterlist
> >
> > // Maybe this can be wrapped as a Rust function with a
> > // parameter: *mut bindings::scatterlist.
> > unsafe {
> > bindings::sg_set_buf(ptr, buf.as_ptr(), buf.len());
> > }
> >
> > [1]: https://doc.rust-lang.org/core/ptr/macro.addr_of.html
> >
> > Regards,
> > Boqun
> >
> > > + Ok(())
> > > + })
> > > + }
> > > + }
> > > +
> > [...]
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-07 19:42 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-02 10:18 [PATCH v2 0/3] Rust scatterlist abstractions Qingsong Chen
2023-06-02 10:18 ` [PATCH v2 1/3] rust: kernel: add ScatterList abstraction Qingsong Chen
2023-06-02 19:54 ` Martin Rodriguez Reboredo
2023-06-05 8:12 ` Qingsong Chen
2023-06-05 11:36 ` Miguel Ojeda
2023-06-05 15:26 ` Boqun Feng
2023-06-06 7:01 ` Qingsong Chen
2023-06-07 17:18 ` Boqun Feng
2023-06-07 18:42 ` Gary Guo
2023-06-07 19:40 ` Boqun Feng
2023-06-02 10:18 ` [PATCH v2 2/3] rust: kernel: implement iterators for ScatterList Qingsong Chen
2023-06-02 19:59 ` Martin Rodriguez Reboredo
2023-06-02 10:18 ` [PATCH v2 3/3] rust: kernel: add SgTable abstraction Qingsong Chen
2023-06-02 20:47 ` Martin Rodriguez Reboredo
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.