linux-kernel - Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9c3af5ce-b692-bfe2-4fef-06281d0813eb@antgroup.com>
Date:   Mon, 05 Jun 2023 16:12:30 +0800
From:   "Qingsong Chen" <changxian.cqs@...group.com>
To:     Martin Rodriguez Reboredo <yakoyoku@...il.com>,
        linux-kernel@...r.kernel.org
Cc:     "田洪亮" <tate.thl@...group.com>,
        "Miguel Ojeda" <ojeda@...nel.org>,
        "Alex Gaynor" <alex.gaynor@...il.com>,
        "Wedson Almeida Filho" <wedsonaf@...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>,
        "Alice Ryhl" <aliceryhl@...gle.com>,
        "Asahi Lina" <lina@...hilina.net>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] rust: kernel: add ScatterList abstraction

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.