From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7794FCD1292 for ; Thu, 4 Apr 2024 22:33:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C6DCE6B009A; Thu, 4 Apr 2024 18:33:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C1DC36B009C; Thu, 4 Apr 2024 18:33:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AE5586B009F; Thu, 4 Apr 2024 18:33:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 8D3C16B009A for ; Thu, 4 Apr 2024 18:33:57 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 5C2D9A15C3 for ; Thu, 4 Apr 2024 22:33:57 +0000 (UTC) X-FDA: 81973303314.21.AFA5B53 Received: from mail-40131.protonmail.ch (mail-40131.protonmail.ch [185.70.40.131]) by imf28.hostedemail.com (Postfix) with ESMTP id 51F1FC0012 for ; Thu, 4 Apr 2024 22:33:55 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=proton.me header.s=i72cbelw3nfq7acf5fidjehlti.protonmail header.b=KwgD1BTs; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf28.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.131 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712270035; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ZVL2Z0cS5q+LN/fYP+ykEsgKaNFbc8NkPR7q7d8Y7Bo=; b=op/FdvMeVA3OJW/KX/7PMF1vjeQfhyH6dBFbP8f0ZZc+EtyGsFidaMqbK/A9zfQNXWQo2O uSC1Lok8yLRkZ7wCl5rkDsv0n7awrvQgGafm+tgeO0jOzHdgLzWJau24w4Yz279b5byRor L0D5gEr4sodcFu85LJMFpzOuitH+EaI= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=proton.me header.s=i72cbelw3nfq7acf5fidjehlti.protonmail header.b=KwgD1BTs; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf28.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.40.131 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712270035; a=rsa-sha256; cv=none; b=U1g4810yn5W//Gs8/UKCTUTgFxYq2qRr44WjQql7cw+tJCLzTdDZd818e8LDLjD864lj7A ZHe3X/DojFtdovy6cJx5phZ1OqDw2HlRSZ9Cu+zRweUgotvlFvyyeX0N7WaQLruDSmRXDv rC0/onmNdS+ko40xL8L4XjXkMKU1MyI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=i72cbelw3nfq7acf5fidjehlti.protonmail; t=1712270032; x=1712529232; bh=ZVL2Z0cS5q+LN/fYP+ykEsgKaNFbc8NkPR7q7d8Y7Bo=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=KwgD1BTs62V/7Rke9wK4bxM66yt20AtPTwOGpJq0vS+Fh7Xo1SYkE58+jw7TBknSa M4rt9UsmzBMP7ZKbLymXNE2/S/8DJ9/yADXe7xQlwyOnBkwJw/Hw7JT5Qzzpl9a8Ms +WnwMUUBfiurWz5wW032w6aA+E8VTyWo+qL3rRCIXtqup0Isx7OBV06ngQZ8YvTaYC rnNaciU1wzvdJiQuIRb3gnQZt1EwGU3Civ5HOcJIGWO6zEbQh/tlDF5+1egGblZaEw g6tPHb6txJ1nBScmvpB+g6gso0CqJQafBzvu+4TnqZ/es6DSX3ySX+w/gdquTX/WWC WT7fe/TPrmJ8w== Date: Thu, 04 Apr 2024 22:33:42 +0000 To: Alice Ryhl , Miguel Ojeda , Matthew Wilcox , Al Viro , Andrew Morton , Kees Cook From: Benno Lossin Cc: Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Greg Kroah-Hartman , =?utf-8?Q?Arve_Hj=C3=B8nnev=C3=A5g?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Carlos Llamas , Suren Baghdasaryan , Arnd Bergmann , linux-mm@kvack.org, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, Christian Brauner Subject: Re: [PATCH v4 4/4] rust: add abstraction for `struct page` Message-ID: In-Reply-To: <20240404-alice-mm-v4-4-49a84242cf02@google.com> References: <20240404-alice-mm-v4-0-49a84242cf02@google.com> <20240404-alice-mm-v4-4-49a84242cf02@google.com> Feedback-ID: 71780778:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 51F1FC0012 X-Stat-Signature: xhfi9btgqdojjppuo3toqbrbykgrpbuz X-HE-Tag: 1712270035-93580 X-HE-Meta: U2FsdGVkX190XH6GcXbgOLCPOKeB96WepRiF5780miIz8VYWdokT4km9AIj0kZmQkMtDRDjaCOa1qhW0UU63akSuITWHj+MmipTsZ6r816f8ZwBj9+0LIQBwW0zWzby/98wLe+J0J2SqCiXxEFiXGFoPHtfWRnfZVNoPF2S4DaXRlfRranlh9o/6HuBrsorrSjS/kCMEqCRvp46x9epUEw9k4GWkYOJKuH0lDJSZp2fHWDfGmm0pyF33TBaLjazUXrBP+JdcZUyEqAjcJZ3QI7VDau4CQRroK3qV+PRKU8dMyJmtULX4guxV4216tUTqg9rsDLJHxN/dW+d1QTIJkT0m29CnSNO4BihUuoo8hZMOU0whfPxvjaB30F6qjF8VrMw9BFpfPxVhXrWhAQhIrI1BPEx9EgpRxN/wjPyZF34FZPfyQjg6PFKP8ry1xwyc9n+/iJkMv5Q+4+wHbpwUWlxU0pVbBc/HzrJqnHWd2GUKzpRtfGSzWJU19ZNk/eeiShNwJ+oloj99IkXZryXzpXCNg9DQ48k/Uu0JRwUnyUGhy/+TePnK4GNkdOP10xN4v/cpegbLe1aP6gw6AKpaTeQLZznuwPGlXetlrgZIA+k18kwwOO53k1DZQsk/P9G+RgT2jLdk1cGDApqzYFuubQ70Hs4rTqYbCG1eNCIZ/uBjTpsvng2wPmOuVtgHhr36iofq9bze73gGEvcb6en/uEGuG7ELcuwSkJohplMk9sOtLB+kWHqudBgQKz/e6D20JSoQHGFr7EqLAX6s4ifz4Abb2A5He/tH+nyWnyHI+4c6fFNIDLUfHEOxOAhPd9g2Hc2QZa9ZR7R1Yw/1ZEwGbgTrwyLMbROgNt0gr5yKbcURw5oKY23bgQUh9zYw4JBDUU2JgaD5KjNWH2j85w74te0RPpbd8Q/bgixJ9rwzC4x+XzAzy8LLrhoCHuBNFbQt0fiR4Q/NnRfz+pFIrYo qKA5nVF3 6jkSH91iiVvz2y0Vv069Suxlhkda+csSlxrj903CKCxABz1t5YO5cBxbe0NBX+/Oko1hLjr3j71fHlhkJppGT2ah9KNyWosoEYGIdMDEGDZu5iyYkM7QFwIhRZiCYrHS5+ISEpzGK/368Fo13tJ5/xdpNIAySFAv4Jr0gi6t58pjClfVbyPI2U9Y3Y6SQO93aVGBpXFQ4VO5CmaVACjFKVMQWHXewMSYVUidw+zCIn9+yT8zV+Tivgdig6YMuiCkTbmOisdmCDQZ2L/0Lwy7menyZ+zc6XVtxG4BgDitU5k+0FIColgPOF2i+wRi2uTSzIganMFzbHlA1yAx6sGpy0Cs54osv18UQbg6Kxi2R0MzvlNQ= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 04.04.24 14:31, Alice Ryhl wrote: > Adds a new struct called `Page` that wraps a pointer to `struct page`. > This struct is assumed to hold ownership over the page, so that Rust > code can allocate and manage pages directly. >=20 > The page type has various methods for reading and writing into the page. > These methods will temporarily map the page to allow the operation. All > of these methods use a helper that takes an offset and length, performs > bounds checks, and returns a pointer to the given offset in the page. >=20 > This patch only adds support for pages of order zero, as that is all > Rust Binder needs. However, it is written to make it easy to add support > for higher-order pages in the future. To do that, you would add a const > generic parameter to `Page` that specifies the order. Most of the > methods do not need to be adjusted, as the logic for dealing with > mapping multiple pages at once can be isolated to just the > `with_pointer_into_page` method. Finally, the struct can be renamed to > `Pages`, and the type alias `Page =3D Pages<0>` can be introduced. This part seems outdated, I think we probably make `ORDER` default to 0. >=20 > Rust Binder needs to manage pages directly as that is how transactions > are delivered: Each process has an mmap'd region for incoming > transactions. When an incoming transaction arrives, the Binder driver > will choose a region in the mmap, allocate and map the relevant pages > manually, and copy the incoming transaction directly into the page. This > architecture allows the driver to copy transactions directly from the > address space of one process to another, without an intermediate copy > to a kernel buffer. [...] > diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs > new file mode 100644 > index 000000000000..5aba0261242d > --- /dev/null > +++ b/rust/kernel/page.rs > @@ -0,0 +1,259 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Kernel page allocation and management. > + > +use crate::{bindings, error::code::*, error::Result, uaccess::UserSliceR= eader}; > +use core::{ > + alloc::AllocError, > + ptr::{self, NonNull}, > +}; > + > +/// A bitwise shift for the page size. > +#[allow(clippy::unnecessary_cast)] Why can't you remove the cast? > +pub const PAGE_SHIFT: usize =3D bindings::PAGE_SHIFT as usize; > + > +/// The number of bytes in a page. > +#[allow(clippy::unnecessary_cast)] > +pub const PAGE_SIZE: usize =3D bindings::PAGE_SIZE as usize; > + > +/// A bitmask that gives the page containing a given address. > +pub const PAGE_MASK: usize =3D !(PAGE_SIZE-1); This line doesn't seem to be correctly formatted. > + > +/// Flags for the "get free page" function that underlies all memory all= ocations. > +pub mod flags { > + /// gfp flags. > + #[allow(non_camel_case_types)] > + pub type gfp_t =3D bindings::gfp_t; > + > + /// `GFP_KERNEL` is typical for kernel-internal allocations. The cal= ler requires `ZONE_NORMAL` > + /// or a lower zone for direct access but can direct reclaim. > + pub const GFP_KERNEL: gfp_t =3D bindings::GFP_KERNEL; > + /// `GFP_ZERO` returns a zeroed page on success. > + pub const __GFP_ZERO: gfp_t =3D bindings::__GFP_ZERO; > + /// `GFP_HIGHMEM` indicates that the allocated memory may be located= in high memory. > + pub const __GFP_HIGHMEM: gfp_t =3D bindings::__GFP_HIGHMEM; > +} > + > +/// A pointer to a page that owns the page allocation. > +/// > +/// # Invariants > +/// > +/// The pointer is valid, and has ownership over the page. > +pub struct Page { > + page: NonNull, > +} > + > +// SAFETY: Pages have no logic that relies on them staying on a given th= read, so > +// moving them across threads is safe. > +unsafe impl Send for Page {} > + > +// SAFETY: Pages have no logic that relies on them not being accessed > +// concurrently, so accessing them concurrently is safe. > +unsafe impl Sync for Page {} > + > +impl Page { > + /// Allocates a new page. > + pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result { > + // SAFETY: Depending on the value of `gfp_flags`, this call may = sleep. > + // Other than that, it is always safe to call this method. > + let page =3D unsafe { bindings::alloc_pages(gfp_flags, 0) }; > + let page =3D NonNull::new(page).ok_or(AllocError)?; > + // INVARIANT: We just successfully allocated a page, so we now h= ave > + // ownership of the newly allocated page. We transfer that owner= ship to > + // the new `Page` object. > + Ok(Self { page }) > + } > + > + /// Returns a raw pointer to the page. > + pub fn as_ptr(&self) -> *mut bindings::page { > + self.page.as_ptr() > + } > + > + /// Runs a piece of code with this page mapped to an address. > + /// > + /// The page is unmapped when this call returns. > + /// > + /// # Using the raw pointer > + /// > + /// It is up to the caller to use the provided raw pointer correctly= . The > + /// pointer is valid for `PAGE_SIZE` bytes and for the duration in w= hich the > + /// closure is called. The pointer might only be mapped on the curre= nt > + /// thread, and when that is the case, dereferencing it on other thr= eads is > + /// UB. Other than that, the usual rules for dereferencing a raw poi= nter > + /// apply: don't cause data races, the memory may be uninitialized, = and so > + /// on. > + /// > + /// If multiple threads map the same page at the same time, then the= y may > + /// reference with different addresses. However, even if the address= es are > + /// different, the underlying memory is still the same for these pur= poses > + /// (e.g., it's still a data race if they both write to the same und= erlying > + /// byte at the same time). This is nice. --=20 Cheers, Benno > + fn with_page_mapped(&self, f: impl FnOnce(*mut u8) -> T) -> T { > + // SAFETY: `page` is valid due to the type invariants on `Page`. > + let mapped_addr =3D unsafe { bindings::kmap_local_page(self.as_p= tr()) }; > + > + let res =3D f(mapped_addr.cast()); > + > + // This unmaps the page mapped above. > + // > + // SAFETY: Since this API takes the user code as a closure, it c= an only > + // be used in a manner where the pages are unmapped in reverse o= rder. > + // This is as required by `kunmap_local`. > + // > + // In other words, if this call to `kunmap_local` happens when a > + // different page should be unmapped first, then there must nece= ssarily > + // be a call to `kmap_local_page` other than the call just above= in > + // `with_page_mapped` that made that possible. In this case, it = is the > + // unsafe block that wraps that other call that is incorrect. > + unsafe { bindings::kunmap_local(mapped_addr) }; > + > + res > + }