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 039C4C4345F for ; Tue, 16 Apr 2024 05:40:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5A3186B0087; Tue, 16 Apr 2024 01:40:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 552B26B0088; Tue, 16 Apr 2024 01:40:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3F36F6B0089; Tue, 16 Apr 2024 01:40:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 1EFC46B0087 for ; Tue, 16 Apr 2024 01:40:40 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 99050A01C2 for ; Tue, 16 Apr 2024 05:40:39 +0000 (UTC) X-FDA: 82014295398.06.B306119 Received: from mail-yb1-f174.google.com (mail-yb1-f174.google.com [209.85.219.174]) by imf14.hostedemail.com (Postfix) with ESMTP id 37986100002 for ; Tue, 16 Apr 2024 05:40:37 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=umich.edu header.s=google-2016-06-03 header.b=H5ITU2tB; dmarc=pass (policy=none) header.from=umich.edu; spf=pass (imf14.hostedemail.com: domain of tmgross@umich.edu designates 209.85.219.174 as permitted sender) smtp.mailfrom=tmgross@umich.edu ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713246037; 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=jZnTsFH2u/jd95rE0nScFwnMla46fGdQkSBISp00WVA=; b=aZSPafkulb/SpmlIh2yeF/SyShEoN3PD3lDJJRzp4eHjdYDbNHh1bRu0nKBi0ofvBwdvm4 p0u1gGozXs3ZpXwO2WxxNw80FXpMKxB5NbRDhDpX7JOfinnic1X2eGFLOi1iw2n0AbFycN jnz1xmhgt4H0VlR+FE+7O+IIrz9iIIw= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=umich.edu header.s=google-2016-06-03 header.b=H5ITU2tB; dmarc=pass (policy=none) header.from=umich.edu; spf=pass (imf14.hostedemail.com: domain of tmgross@umich.edu designates 209.85.219.174 as permitted sender) smtp.mailfrom=tmgross@umich.edu ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713246037; a=rsa-sha256; cv=none; b=8c06kci06rIoqzsK2HXtPGGV8RHCRs9CbHls7VM88r1TXpFeSr40jd6bzJn4zzUEBNqJcz xmx+0dSZIK1tt4lSIiaOIGVV1bHs4N5RkRIS3/9RKax2rvuLReFZCkkZ2Bs6sSZN19pWjU UQ0oOa/xua9huwuprUePLj5R/oYiysA= Received: by mail-yb1-f174.google.com with SMTP id 3f1490d57ef6-db4364ecd6aso4239388276.2 for ; Mon, 15 Apr 2024 22:40:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=umich.edu; s=google-2016-06-03; t=1713246036; x=1713850836; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=jZnTsFH2u/jd95rE0nScFwnMla46fGdQkSBISp00WVA=; b=H5ITU2tBK8GcD5bQeDHbWsfJOkmLPXKk4PH2CfFWioKpmJ3QNblPIqa5lM2/TRkKi+ jUB6/jH86W7ygswND1x6WAv6mDTp0bauwfgAdKYHtCsfgRfbf3g7BDi2hjvUpjpGU83x Dhsv55wXOqF5Rw0aWM25TNTHI2Bb9EN7hVyyBc36RvV8Cdanv1rzG00wrkQiuo2vQtte yRkxvDLcdzKOCACZEzUqGqYB2/kN/9gnePHbdaMXQbHvaiCfQVY8gWwHUroBul65AgKN D7c/5K+eQ8TlDZRhGWS3xVhfxccAg6G1f2u/v+fJ0sW/DlQ8A47vhg+lzpFICYWE/U4n 7ldg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713246036; x=1713850836; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jZnTsFH2u/jd95rE0nScFwnMla46fGdQkSBISp00WVA=; b=Pn+Fvern08KsU8iSvHogLwOI37wJrzqkz/gkTkrjE0gPbp7iqnhY+47Y2bViRF/Miz vmmx6LlIWdNFr+y6qSbmRBqcNn+mywWtZoMZ8OxMYHQ8DeafYVBKzCPf3DvFMecH3OnK iT4SZ9LOMrvUYXoyUOFo9S9fZA6ajcJbGzptNFdUHsTWmyqEqPbwFBLBUptdKajaoBPz zH6U0EwO+jcHDlVNfplZeTc/NfrojQiVoKt6eRLORVzGUOV3TvolJOCH10RkoX2UP8Fz ggZy3tzRgnqbiUvkw+gvlX47XM4llowPbFESUxOR91GHAnw1ctOo+zlQPc9iyaZSbnF+ e4mw== X-Forwarded-Encrypted: i=1; AJvYcCWp3rBEnnkmBqDx8mPB2XxKClTQm2hOdSFQgYIBBkEaqEasJpriUX91yHXdbLs4bt7WWsOcxlQHugWTvzjPOPf4lmo= X-Gm-Message-State: AOJu0Yy1AZaqMGBm1VcXDBuOA6+adF1BG0YkZistjBLahU0Zew7h+UQT hHEj7rYAbPMYYvpJVPVJ1sg5R6+7f3luOAKYbouWF9+3fUMIbLSqomG5bxkZbE7p538vriSxtoM zcFidTbEo62quS4HDUP6puNRw4HN9HlK00J1MJw== X-Google-Smtp-Source: AGHT+IGuUXE3/S314JFZlsIl8KEIWGV1s7fbLfZ+EKGCMv6cWkNb4R6jL13/UiXlkh0SwJJUedsQg35/YYR3WhhU/6g= X-Received: by 2002:a25:9ac1:0:b0:dcf:f535:dad6 with SMTP id t1-20020a259ac1000000b00dcff535dad6mr9792461ybo.56.1713246036166; Mon, 15 Apr 2024 22:40:36 -0700 (PDT) MIME-Version: 1.0 References: <20240415-alice-mm-v5-0-6f55e4d8ef51@google.com> <20240415-alice-mm-v5-4-6f55e4d8ef51@google.com> In-Reply-To: <20240415-alice-mm-v5-4-6f55e4d8ef51@google.com> From: Trevor Gross Date: Tue, 16 Apr 2024 01:40:25 -0400 Message-ID: Subject: Re: [PATCH v5 4/4] rust: add abstraction for `struct page` To: Alice Ryhl Cc: Miguel Ojeda , Matthew Wilcox , Al Viro , Andrew Morton , Kees Cook , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: z4dd4ebfkmhpbjd9fbsq7ie7t45e1rck X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 37986100002 X-HE-Tag: 1713246037-137695 X-HE-Meta: U2FsdGVkX1+ItB5wBdf02gZ48HPIr12JC23YHzCiJF6yjbaQ4KF85BtXRZx8aG7M/ja17VQx29V5+2aMhD662ZQM5MQ9LEvZWvaCHKgylH63kpCA4K1j0Dm8onkNp8vU65Ibt2U+dokFpudMcr3tJ8mQAWi1DVo9x41g8SBqku2L4oLm6mJIADa3Aim82ccDOz/VpQ53QmOdEvTlEP+BKqfaV9Lbz/irFTlF8cEh+E0Ntf8Bf/NkfNQpMjR/badK+99rU+585F94zSIY+mcsQf5CkCHKpSd0TWLzNeHtcosapyY6gw625aS1kuBxUt2ZMY7eWnANC64zNZVN54+jvTMpdWKYC+XdjLNSIE0haV3dnyexoFFCi0dGufCwKfZKiXVXcqMhbeUczmJTZHx4MCLg9zU74h1UPCQ+3xI5136FZj+uKQQhFUdCZiUMLHfSvBcAfq9LkDjgdQ4RakV4eF59Y+bKMBaW/4tAvRg0rPzwwNwI5LgNsVeKkfwIIlODJJ8PQoSakJisbS0UcpIYNaAEf+Go4YwYCLVBWQPlZZ+o7IH+xhKYlQ+0TpgdkwS9DrX8HTtj6KCP2LypTkclqI5dL/h+usucoUqkNJZET7LnI8SS8KMV8Ne7ZLhleNyBd3/+E6QEJoPr93icKaNQDbGgW6glCWhhPXo/we7qe16Zh7aUbHkbCLP+Vhfpn7urm1rVbQqU+nwx/+kWdnzty2SgPzI7F38z8SLWJiso/edo+dAuVS/BU3kct6Zr6nwlt0oGV6Uswj0blh6nH63ipeqz6BcxORxnv/IiblDqE7IerNpluoRRfFC9HBChjUXL4nJEHrpj2EX9HhePuvQUVAwTL0M8LLq2nXKTV9lhIJQ76NpR7xYqDnSY3uGYz2zUiLmdtfdcWNNQBmqGZBIh2VwV5NlXJjJAxUDUm/yDAlXEq5RZ6UeuZzzaj9Ot2RAL9Xq8huptt2/ulU6guY/ LGSJKxpp 4EqBuJD3a8GT9hTQ/kWrY/jRk1VftCKpWNNyuB9cqBzE9XNgSNbp/H+CEvbxp47wEM8mFZdqPTazZPSkUlK7yqjr4CWvzuchQpUR5TYJH9vfTrINaOx9gkt8UMD8p7nKeNd44FPDAR7pA5XcLQ+eQatUHO1pfdAUdClb0x4Poqd6zF0vbNYT6xtWIwyr8T0m3SynZ+L0u/Sca08r8toy+gQQnfkSurWfPuvzV3VCjMVGVKe/XG/vKCDL8WPe3LpOT3TpBPbLBbjtFAr8dtJjNFPxclS/QLSw1V16/bNL3svkv2HMm5Wt2KuiN+kKixj9EqyiYhlhtQIDOOd9pamSJwwzVFj6EpvS/ZRSPYQGJCcslfbiXy5m2qm4g6wJ6VHr6YnC1 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 Mon, Apr 15, 2024 at 3:15=E2=80=AFAM Alice Ryhl w= rote: > > 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. > > 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. > > 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. > > 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. > > This code is based on Wedson's page abstractions from the old rust > branch, but it has been modified by Alice by removing the incomplete > support for higher-order pages, by introducing the `with_*` helpers > to consolidate the bounds checking logic into a single place, and by > introducing gfp flags. > > Co-developed-by: Wedson Almeida Filho > Signed-off-by: Wedson Almeida Filho > Signed-off-by: Alice Ryhl I have a couple questions about naming, and think an example would be good for the functions that are trickier to use correctly. But I wouldn't block on this, implementation looks good to me. Reviewed-by: Trevor Gross > +++ b/rust/kernel/page.rs > @@ -0,0 +1,240 @@ > +// 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. > +pub const PAGE_SHIFT: usize =3D bindings::PAGE_SHIFT as usize; > + > +/// The number of bytes in a page. > +pub const PAGE_SIZE: usize =3D bindings::PAGE_SIZE; > + > +/// A bitmask that gives the page containing a given address. > +pub const PAGE_MASK: usize =3D !(PAGE_SIZE - 1); > + > +/// Flags for the "get free page" function that underlies all memory all= ocations. > +pub mod flags { > + /// gfp flags. Uppercase acronym, maybe with a description: GFP (Get Free Page) flags. > + #[allow(non_camel_case_types)] > + pub type gfp_t =3D bindings::gfp_t; Why not GfpFlags, do we do this elsewhere? > + /// `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; It feels a bit weird to have dunder constants on the rust side that aren't also `#[doc(hidden)]` or just nonpublic. Makes me think they are an implementation detail or not really meant to be used - could you update the docs if this is the case? > + > +impl Page { > + /// Allocates a new page. Could you add a small example here? > + pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result { > [...] > + } > + > + /// Returns a raw pointer to the page. Could you add a note about how the pointer needs to be used correctly, if it is for anything more than interfacing with kernel APIs? > + 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 which the closure is c= alled. The pointer might > + /// only be mapped on the current thread, and when that is the case,= dereferencing it on other > + /// threads is UB. Other than that, the usual rules for dereferencin= g a raw pointer 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 addresses are differen= t, the underlying memory is > + /// still the same for these purposes (e.g., it's still a data race = if they both write to the > + /// same underlying byte at the same time). > + fn with_page_mapped(&self, f: impl FnOnce(*mut u8) -> T) -> T { > [...] > + } Could you add an example of how to use this correctly? > + /// Runs a piece of code with a raw pointer to a slice of this page,= with bounds checking. > + /// > + /// If `f` is called, then it will be called with a pointer that poi= nts at `off` bytes into the > + /// page, and the pointer will be valid for at least `len` bytes. Th= e pointer is only valid on > + /// this task, as this method uses a local mapping. > + /// > + /// If `off` and `len` refers to a region outside of this page, then= this method returns > + /// `EINVAL` and does not call `f`. > + /// > + /// # Using the raw pointer > + /// > + /// It is up to the caller to use the provided raw pointer correctly= . The pointer is valid for > + /// `len` bytes and for the duration in which the closure is called.= The pointer might only be > + /// mapped on the current thread, and when that is the case, derefer= encing it on other threads > + /// is UB. Other than that, the usual rules for dereferencing a raw = pointer 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 addresses are differen= t, the underlying memory is > + /// still the same for these purposes (e.g., it's still a data race = if they both write to the > + /// same underlying byte at the same time). This could probably also use an example. A note about how to select between with_pointer_into_page and with_page_mapped would also be nice to guide usage, e.g. "prefer with_pointer_into_page for all cases except when..." > + fn with_pointer_into_page( > + &self, > + off: usize, > + len: usize, > + f: impl FnOnce(*mut u8) -> Result, > + ) -> Result { > [...] > + /// Maps the page and zeroes the given slice. > + /// > + /// This method will perform bounds checks on the page offset. If `o= ffset .. offset+len` goes > + /// outside ot the page, then this call returns `EINVAL`. > + /// > + /// # Safety > + /// > + /// Callers must ensure that this call does not race with a read or = write to the same page that > + /// overlaps with this write. > + pub unsafe fn fill_zero(&self, offset: usize, len: usize) -> Result = { > + self.with_pointer_into_page(offset, len, move |dst| { > + // SAFETY: If `with_pointer_into_page` calls into this closu= re, then it has performed a > + // bounds check and guarantees that `dst` is valid for `len`= bytes. > + // > + // There caller guarantees that there is no data race. > + unsafe { ptr::write_bytes(dst, 0u8, len) }; > + Ok(()) > + }) > + } Could this be named `fill_zero_raw` to leave room for a safe `fill_zero(&mut self, ...)`? > + /// Copies data from userspace into this page. > + /// > + /// This method will perform bounds checks on the page offset. If `o= ffset .. offset+len` goes > + /// outside ot the page, then this call returns `EINVAL`. > + /// > + /// Like the other `UserSliceReader` methods, data races are allowed= on the userspace address. > + /// However, they are not allowed on the page you are copying into. > + /// > + /// # Safety > + /// > + /// Callers must ensure that this call does not race with a read or = write to the same page that > + /// overlaps with this write. > + pub unsafe fn copy_from_user_slice( > + &self, > + reader: &mut UserSliceReader, > + offset: usize, > + len: usize, > + ) -> Result { > + self.with_pointer_into_page(offset, len, move |dst| { > + // SAFETY: If `with_pointer_into_page` calls into this closu= re, then it has performed a > + // bounds check and guarantees that `dst` is valid for `len`= bytes. Furthermore, we have > + // exclusive access to the slice since the caller guarantees= that there are no races. > + reader.read_raw(unsafe { core::slice::from_raw_parts_mut(dst= .cast(), len) }) > + }) > + } > +} Same as above, `copy_from_user_slice_raw` would leave room for a safe API.