linux-kernel - Re: [PATCH v3 3/3] rust: kernel: add SgTable 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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 12 Jun 2023 13:13:12 -0300
From:   Martin Rodriguez Reboredo <yakoyoku@...il.com>
To:     Qingsong Chen <changxian.cqs@...group.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>,
        rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 3/3] rust: kernel: add SgTable abstraction

On 6/10/23 07:49, 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@...group.com>
> ---
> [...]
> +impl<'a, const N: usize> SgTable<'a, N> {
> +    /// Construct a new initializer.
> +    ///
> +    /// # Errors
> +    ///
> +    /// The length of `entries` should exactly be the available size of [`SgTable<N>`],
> +    /// or else an error is returned.
> +    ///
> +    /// If the table is `chainable`, the available size is `N - 1`, because one entry
> +    /// should be reserved for chaining.
> +    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 {
> +                    // `slot` contains uninit memory, avoid creating a reference.
> +                    let opaque = addr_of!((*slot).entries[i].opaque);
> +                    let sgl = Opaque::raw_get(opaque);
> +
> +                    bindings::sg_set_buf(sgl, entries[i].as_ptr() as _, entries[i].len() as _);
> +                    if i + 1 == nr_entry {
> +                        (*sgl).page_link |= bindings::SG_END as u64;
> +                        (*sgl).page_link &= !(bindings::SG_CHAIN as u64);
> +                    }
> +                }
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Chain two [`SgTable`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to the
> +    /// first entry of `sgt` SgTable.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has been chained.
> +    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();
> +
> +        // SAFETY: `sgt.entries` are initialized by the `new` constructor,
> +        // so it's valid.
> +        let next = unsafe { ScatterList::as_mut(sgt.entries[0].opaque.get()).unwrap() };
> +        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.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has been chained.
> +    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.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has not been chained.
> +    pub fn split(&mut self) -> Result {
> +        if !self.entries[N - 1].is_chain() {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].mark_end();
> +        Ok(())
> +    }
> +
> [...]
> +}

I'd suggest being more explicit in the `# Errors` section by
mentioning that `EINVAL` is returned, but do as you prefer.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@...il.com>