Re: [RFC PATCH 02/11] rust: add `pages` module for handling page allocation

From: Benno Lossin
Date: Wed May 03 2023 - 08:38:44 EST

Next message: Wolfram Sang: "Re: [PATCH 1/2] clk: renesas: r8a779a0: Add PWM clock"
Previous message: James Bottomley: "Re: [PATCH 00/40] Memory allocation profiling"
In reply to: Benno Lossin: "Re: [RFC PATCH 02/11] rust: add `pages` module for handling page allocation"
Next in thread: Matthew Wilcox: "Re: [RFC PATCH 02/11] rust: add `pages` module for handling page allocation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Sorry, forgot to replace `> #@` with nothing. Fixed here:

On 03.05.23 11:06, Andreas Hindborg wrote:
> From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>
> This patch adds support for working with pages of order 0. Support for pages
> with higher order is deferred. Page allocation flags are fixed in this patch.
> Future work might allow the user to specify allocation flags.
>
> This patch is a heavily modified version of code available in the rust tree [1],
> primarily adding support for multiple page mapping strategies.
>
> [1] https://github.com/rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f/rust/kernel/pages.rs
>
> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
> ---
> rust/helpers.c | 31 +++++
> rust/kernel/lib.rs | 6 +
> rust/kernel/pages.rs | 284 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 321 insertions(+)
> create mode 100644 rust/kernel/pages.rs
>
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 5dd5e325b7cc..9bd9d95da951 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -27,6 +27,7 @@
> #include <linux/sched/signal.h>
> #include <linux/wait.h>
> #include <linux/radix-tree.h>
> +#include <linux/highmem.h>
>
> __noreturn void rust_helper_BUG(void)
> {
> @@ -150,6 +151,36 @@ void **rust_helper_radix_tree_next_slot(void **slot,
> }
> EXPORT_SYMBOL_GPL(rust_helper_radix_tree_next_slot);
>
> +void *rust_helper_kmap(struct page *page)
> +{
> + return kmap(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kmap);
> +
> +void rust_helper_kunmap(struct page *page)
> +{
> + return kunmap(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kunmap);
> +
> +void *rust_helper_kmap_atomic(struct page *page)
> +{
> + return kmap_atomic(page);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kmap_atomic);
> +
> +void rust_helper_kunmap_atomic(void *address)
> +{
> + kunmap_atomic(address);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_kunmap_atomic);
> +
> +struct page *rust_helper_alloc_pages(gfp_t gfp_mask, unsigned int order)
> +{
> + return alloc_pages(gfp_mask, order);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_alloc_pages);
> +
> /*
> * 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 a85cb6aae8d6..8bef6686504b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -38,6 +38,7 @@ mod build_assert;
> pub mod error;
> pub mod init;
> pub mod ioctl;
> +pub mod pages;
> pub mod prelude;
> pub mod print;
> pub mod radix_tree;
> @@ -57,6 +58,11 @@ pub use uapi;
> #[doc(hidden)]
> pub use build_error::build_error;
>
> +/// Page size defined in terms of the `PAGE_SHIFT` macro from C.

`PAGE_SHIFT` is not using a doc-link.

> +///
> +/// [`PAGE_SHIFT`]: ../../../include/asm-generic/page.h
> +pub const PAGE_SIZE: u32 = 1 << bindings::PAGE_SHIFT;

This should be of type `usize`.

> +
> /// Prefix to appear before log messages printed from within the `kernel` crate.
> const __LOG_PREFIX: &[u8] = b"rust_kernel\0";
>
> diff --git a/rust/kernel/pages.rs b/rust/kernel/pages.rs
> new file mode 100644
> index 000000000000..ed51b053dd5d
> --- /dev/null
> +++ b/rust/kernel/pages.rs
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Kernel page allocation and management.
> +//!
> +//! This module currently provides limited support. It supports pages of order 0
> +//! for most operations. Page allocation flags are fixed.
> +
> +use crate::{bindings, error::code::*, error::Result, PAGE_SIZE};
> +use core::{marker::PhantomData, ptr};
> +
> +/// A set of physical pages.
> +///
> +/// `Pages` holds a reference to a set of pages of order `ORDER`. Having the order as a generic
> +/// const allows the struct to have the same size as a pointer.

I would remove the 'Having the order as a...' sentence. Since that is
just implementation detail.

> +///
> +/// # Invariants
> +///
> +/// The pointer `Pages::pages` is valid and points to 2^ORDER pages.

`Pages::pages` -> `pages`.

> +pub struct Pages<const ORDER: u32> {
> + pub(crate) pages: *mut bindings::page,
> +}
> +
> +impl<const ORDER: u32> Pages<ORDER> {
> + /// Allocates a new set of contiguous pages.
> + pub fn new() -> Result<Self> {
> + let pages = unsafe {
> + bindings::alloc_pages(
> + bindings::GFP_KERNEL | bindings::__GFP_ZERO | bindings::___GFP_HIGHMEM,
> + ORDER,
> + )
> + };

Missing `SAFETY` comment.

> + if pages.is_null() {
> + return Err(ENOMEM);
> + }
> + // INVARIANTS: We checked that the allocation above succeeded.
> + // SAFETY: We allocated pages above
> + Ok(unsafe { Self::from_raw(pages) })
> + }
> +
> + /// Create a `Pages` from a raw `struct page` pointer
> + ///
> + /// # Safety
> + ///
> + /// Caller must own the pages pointed to by `ptr` as these will be freed
> + /// when the returned `Pages` is dropped.
> + pub unsafe fn from_raw(ptr: *mut bindings::page) -> Self {
> + Self { pages: ptr }
> + }
> +}
> +
> +impl Pages<0> {
> + #[inline(always)]

Is this really needed? I think this function should be inlined
automatically.

> + fn check_offset_and_map<I: MappingInfo>(
> + &self,
> + offset: usize,
> + len: usize,
> + ) -> Result<PageMapping<'_, I>>
> + where
> + Pages<0>: MappingActions,

Why not use `Self: MappingActions`?

> + {
> + let end = offset.checked_add(len).ok_or(EINVAL)?;
> + if end as u32 > PAGE_SIZE {

Remove the `as u32`, since `PAGE_SIZE` should be of type `usize`.

> + return Err(EINVAL);

I think it would make sense to create a more descriptive Rust error with
a `From` impl to turn it into an `Error`. It always is better to know from
the signature what exactly can go wrong when calling a function.

> + }
> +
> + let mapping = <Self as MappingActions>::map(self);
> +
> + Ok(mapping)

I would merge these lines.

> + }
> +
> + #[inline(always)]
> + unsafe fn read_internal<I: MappingInfo>(

Missing `# Safety` section.

> + &self,
> + dest: *mut u8,
> + offset: usize,
> + len: usize,
> + ) -> Result
> + where
> + Pages<0>: MappingActions,
> + {
> + let mapping = self.check_offset_and_map::(offset, len)?;
> +
> + unsafe { ptr::copy_nonoverlapping((mapping.ptr as *mut u8).add(offset), dest, len) };

Missing `SAFETY` comment. Replace `as *mut u8` with `.cast::<u8>()`.

> + Ok(())
> + }
> +
> + /// Maps the pages and reads from them into the given buffer.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that the destination buffer is valid for the given
> + /// length. Additionally, if the raw buffer is intended to be recast, they
> + /// must ensure that the data can be safely cast;
> + /// [`crate::io_buffer::ReadableFromBytes`] has more details about it.
> + /// `dest` may not point to the source page.

- `dest` is valid for writes for `len`.
- What is meant by 'the raw buffer is intended to be recast'?
- `io_buffer` does not yet exist in `rust-next`.

> + #[inline(always)]
> + pub unsafe fn read(&self, dest: *mut u8, offset: usize, len: usize) -> Result {
> + unsafe { self.read_internal::<NormalMappingInfo>(dest, offset, len) }

Missing `SAFETY` comment.

> + }
> +
> + /// Maps the pages and reads from them into the given buffer. The page is
> + /// mapped atomically.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that the destination buffer is valid for the given
> + /// length. Additionally, if the raw buffer is intended to be recast, they
> + /// must ensure that the data can be safely cast;
> + /// [`crate::io_buffer::ReadableFromBytes`] has more details about it.
> + /// `dest` may not point to the source page.
> + #[inline(always)]
> + pub unsafe fn read_atomic(&self, dest: *mut u8, offset: usize, len: usize) -> Result {
> + unsafe { self.read_internal::<AtomicMappingInfo>(dest, offset, len) }

Missing `SAFETY` comment.

> + }
> +
> + #[inline(always)]
> + unsafe fn write_internal<I: MappingInfo>(

Missing `# Safety` section.

> + &self,
> + src: *const u8,
> + offset: usize,
> + len: usize,
> + ) -> Result
> + where
> + Pages<0>: MappingActions,
> + {
> + let mapping = self.check_offset_and_map::(offset, len)?;
> +
> + unsafe { ptr::copy_nonoverlapping(src, (mapping.ptr as *mut u8).add(offset), len) };

Missing `SAFETY` comment.

> + Ok(())
> + }
> +
> + /// Maps the pages and writes into them from the given buffer.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that the buffer is valid for the given length.
> + /// Additionally, if the page is (or will be) mapped by userspace, they must
> + /// ensure that no kernel data is leaked through padding if it was cast from
> + /// another type; [`crate::io_buffer::WritableToBytes`] has more details
> + /// about it. `src` must not point to the destination page.

`src` is valid for reads for `len`.

> + #[inline(always)]
> + pub unsafe fn write(&self, src: *const u8, offset: usize, len: usize) -> Result {
> + unsafe { self.write_internal::<NormalMappingInfo>(src, offset, len) }
> + }
> +
> + /// Maps the pages and writes into them from the given buffer. The page is
> + /// mapped atomically.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that the buffer is valid for the given length.
> + /// Additionally, if the page is (or will be) mapped by userspace, they must
> + /// ensure that no kernel data is leaked through padding if it was cast from
> + /// another type; [`crate::io_buffer::WritableToBytes`] has more details
> + /// about it. `src` must not point to the destination page.
> + #[inline(always)]
> + pub unsafe fn write_atomic(&self, src: *const u8, offset: usize, len: usize) -> Result {
> + unsafe { self.write_internal::<AtomicMappingInfo>(src, offset, len) }
> + }
> +
> + /// Maps the page at index 0.
> + #[inline(always)]
> + pub fn kmap(&self) -> PageMapping<'_, NormalMappingInfo> {
> + let ptr = unsafe { bindings::kmap(self.pages) };

Missing `SAFETY` comment.

> +
> + PageMapping {
> + page: self.pages,
> + ptr,
> + _phantom: PhantomData,
> + _phantom2: PhantomData,
> + }
> + }
> +
> + /// Atomically Maps the page at index 0.
> + #[inline(always)]
> + pub fn kmap_atomic(&self) -> PageMapping<'_, AtomicMappingInfo> {
> + let ptr = unsafe { bindings::kmap_atomic(self.pages) };

Missing `SAFETY` comment.

> +
> + PageMapping {
> + page: self.pages,
> + ptr,
> + _phantom: PhantomData,
> + _phantom2: PhantomData,
> + }
> + }
> +}
> +
> +impl<const ORDER: u32> Drop for Pages<ORDER> {
> + fn drop(&mut self) {
> + // SAFETY: By the type invariants, we know the pages are allocated with the given order.
> + unsafe { bindings::__free_pages(self.pages, ORDER) };
> + }
> +}
> +
> +/// Specifies the type of page mapping
> +pub trait MappingInfo {}
> +
> +/// Encapsulates methods to map and unmap pages
> +pub trait MappingActions<I: MappingInfo>
> +where
> + Pages<0>: MappingActions,
> +{
> + /// Map a page into the kernel address scpace

Typo.

> + fn map(pages: &Pages<0>) -> PageMapping<'_, I>;
> +
> + /// Unmap a page specified by `mapping`
> + ///
> + /// # Safety
> + ///
> + /// Must only be called by `PageMapping::drop()`.
> + unsafe fn unmap(mapping: &PageMapping<'_, I>);
> +}
> +
> +/// A type state indicating that pages were mapped atomically
> +pub struct AtomicMappingInfo;
> +impl MappingInfo for AtomicMappingInfo {}
> +
> +/// A type state indicating that pages were not mapped atomically
> +pub struct NormalMappingInfo;
> +impl MappingInfo for NormalMappingInfo {}
> +
> +impl MappingActions<AtomicMappingInfo> for Pages<0> {
> + #[inline(always)]
> + fn map(pages: &Pages<0>) -> PageMapping<'_, AtomicMappingInfo> {
> + pages.kmap_atomic()
> + }
> +
> + #[inline(always)]
> + unsafe fn unmap(mapping: &PageMapping<'_, AtomicMappingInfo>) {
> + // SAFETY: An instance of `PageMapping` is created only when `kmap` succeeded for the given
> + // page, so it is safe to unmap it here.
> + unsafe { bindings::kunmap_atomic(mapping.ptr) };
> + }
> +}
> +
> +impl MappingActions<NormalMappingInfo> for Pages<0> {
> + #[inline(always)]
> + fn map(pages: &Pages<0>) -> PageMapping<'_, NormalMappingInfo> {
> + pages.kmap()
> + }
> +
> + #[inline(always)]
> + unsafe fn unmap(mapping: &PageMapping<'_, NormalMappingInfo>) {
> + // SAFETY: An instance of `PageMapping` is created only when `kmap` succeeded for the given
> + // page, so it is safe to unmap it here.
> + unsafe { bindings::kunmap(mapping.page) };
> + }
> +}

I am not sure if this is the best implementation, why do the `kmap` and
`kmap_atomic` functions exist? Would it not make sense to implement
them entirely in `MappingActions::map`?

> +
> +/// An owned page mapping. When this struct is dropped, the page is unmapped.
> +pub struct PageMapping<'a, I: MappingInfo>
> +where
> + Pages<0>: MappingActions,
> +{
> + page: *mut bindings::page,
> + ptr: *mut core::ffi::c_void,
> + _phantom: PhantomData<&'a i32>,
> + _phantom2: PhantomData,
> +}
> +
> +impl<'a, I: MappingInfo> PageMapping<'a, I>
> +where
> + Pages<0>: MappingActions,
> +{
> + /// Return a pointer to the wrapped `struct page`
> + #[inline(always)]
> + pub fn get_ptr(&self) -> *mut core::ffi::c_void {
> + self.ptr
> + }
> +}
> +
> +// Because we do not have Drop specialization, we have to do this dance. Life
> +// would be much more simple if we could have `impl Drop for PageMapping<'_,
> +// Atomic>` and `impl Drop for PageMapping<'_, NotAtomic>`
> +impl<I: MappingInfo> Drop for PageMapping<'_, I>
> +where
> + Pages<0>: MappingActions,
> +{
> + #[inline(always)]
> + fn drop(&mut self) {
> + // SAFETY: We are OK to call this because we are `PageMapping::drop()`
> + unsafe { <Pages<0> as MappingActions>::unmap(self) }
> + }
> +}
> --
> 2.40.0

Here are some more general things:
- I think we could use this as an opportunity to add more docs about how
paging works, or at least add some links to the C documentation.
- Can we improve the paging API? I have not given it any thought yet, but
the current API looks very primitive.
- Documentation comments should form complete sentences (so end with `.`).

--
Cheers,
Benno

Next message: Wolfram Sang: "Re: [PATCH 1/2] clk: renesas: r8a779a0: Add PWM clock"
Previous message: James Bottomley: "Re: [PATCH 00/40] Memory allocation profiling"
In reply to: Benno Lossin: "Re: [RFC PATCH 02/11] rust: add `pages` module for handling page allocation"
Next in thread: Matthew Wilcox: "Re: [RFC PATCH 02/11] rust: add `pages` module for handling page allocation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]