Re: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg_table

[Daniel Almeida <daniel.almeida@collabora.com>]

> On 26 Aug 2025, at 11:32, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Tue Aug 26, 2025 at 4:16 PM CEST, Alice Ryhl wrote:
>> On Mon, Aug 25, 2025 at 03:24:42PM +0200, Danilo Krummrich wrote:
>> Overall LGTM. With comments addressed:
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> 
>>> +impl RawSGTable {
>>> +    fn new(
>>> +        pages: &mut [*mut bindings::page],
>> 
>> This should probably be unsafe due to the raw pointer. Or could we pass
>> any pointer here?
> 
> Good catch, we should indeed make this unsafe and add the corresponding safety
> requirements:
> 
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> index e76e5c2cbdc7..a562c0360842 100644
> --- a/rust/kernel/scatterlist.rs
> +++ b/rust/kernel/scatterlist.rs
> @@ -251,7 +251,12 @@ unsafe impl Send for RawSGTable {}
> unsafe impl Sync for RawSGTable {}
> 
> impl RawSGTable {
> -    fn new(
> +    /// # Safety
> +    ///
> +    /// - `pages` must be a slice of valid `struct page *`.
> +    /// - The pages pointed to by `pages` must remain valid for the entire lifetime of the returned
> +    ///   [`RawSGTable`].
> +    unsafe fn new(
>         pages: &mut [*mut bindings::page],
>         size: usize,
>         max_segment: u32,
> @@ -355,7 +360,11 @@ fn new(
>         };
> 
>         Ok(try_pin_init!(&this in Self {
> -            sgt <- RawSGTable::new(&mut page_vec, size, max_segment, flags),
> +            // SAFETY:
> +            // - `page_vec` is a `KVec` of valid `struct page *` obtained from `pages`.
> +            // - The pages contained in `pages` remain valid for the entire lifetime of the
> +            //   `RawSGTable`.
> +            sgt <- unsafe { RawSGTable::new(&mut page_vec, size, max_segment, flags) },
>             dma <- {
>                 // SAFETY: `this` is a valid pointer to uninitialized memory.
>                 let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
> 
> 

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>