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 482CAC4345F for ; Tue, 16 Apr 2024 09:53:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B6A306B0092; Tue, 16 Apr 2024 05:53:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AF1036B0093; Tue, 16 Apr 2024 05:53:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 967E66B0095; Tue, 16 Apr 2024 05:53:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 73B196B0092 for ; Tue, 16 Apr 2024 05:53:39 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 0D195A0AA5 for ; Tue, 16 Apr 2024 09:53:39 +0000 (UTC) X-FDA: 82014932958.13.6DF03C3 Received: from mail-lf1-f73.google.com (mail-lf1-f73.google.com [209.85.167.73]) by imf15.hostedemail.com (Postfix) with ESMTP id 17433A0007 for ; Tue, 16 Apr 2024 09:53:36 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=OlRkqPyX; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of 3n0oeZgkKCOQGROIKXeNRMUUMRK.IUSROTad-SSQbGIQ.UXM@flex--aliceryhl.bounces.google.com designates 209.85.167.73 as permitted sender) smtp.mailfrom=3n0oeZgkKCOQGROIKXeNRMUUMRK.IUSROTad-SSQbGIQ.UXM@flex--aliceryhl.bounces.google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713261217; 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=8dWBKU5D27aRchDLY2aDBlpODU4i4Munl9LNc7I7ZXQ=; b=CHvePRSxdvNq0hPYDTSh59G7nQHqQyuI2kTwObjcvBvQVeiK1oZQl03ykO7sr2EX1LW4hn JZ76w6+xNAbHyDc1QmTPYyWO95wUgz/G+AKdanI25+RH8y5KrmOQ6Z4x92AFujl+TORSzi ttdp2xV5bNfJsU55jXoSjukhOSVtW9c= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=OlRkqPyX; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf15.hostedemail.com: domain of 3n0oeZgkKCOQGROIKXeNRMUUMRK.IUSROTad-SSQbGIQ.UXM@flex--aliceryhl.bounces.google.com designates 209.85.167.73 as permitted sender) smtp.mailfrom=3n0oeZgkKCOQGROIKXeNRMUUMRK.IUSROTad-SSQbGIQ.UXM@flex--aliceryhl.bounces.google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713261217; a=rsa-sha256; cv=none; b=IMNSINat+2XkTw3x8xxrVx+6RIFKTx4spQrC1kT1B8knPThh7twaomYz48/ISvLrjPMv7H i5YQ8s0FwK3vDPfg3cVBOENXmoadUIQJ8ALHvRLkTpyppAkdyR1N9+CpHBWVsA5TaLwvyB xVOKRDfqwLc9NsMu0Q3JICziHWuLmJk= Received: by mail-lf1-f73.google.com with SMTP id 2adb3069b0e04-516de13dd48so2648259e87.0 for ; Tue, 16 Apr 2024 02:53:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1713261215; x=1713866015; darn=kvack.org; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:from:to:cc:subject:date:message-id :reply-to; bh=8dWBKU5D27aRchDLY2aDBlpODU4i4Munl9LNc7I7ZXQ=; b=OlRkqPyXfmTEhnRNJSx/SwU2EaA8BA/y2I/KxVvVL7k296pvbEqhMUmwAb9mdrwOVu Q3hQHL9Pf14Nt3kIE/KnE/5qZVYSQfS3GBZpE+dVGT+9pyB/P2eFgSOIBDk+cBpg3kOS 6EpIx4+TuXL5xlPLHff8C/vgq6rnMbceznquOQqCRgNQTgr3GKXlpJii7bbScyVRyAW7 X07qBLpMTy6cuZYo23q+2nH1pxR9S80VcVbRF1blIQKlTQbHrtuAEkRd5B/1wuYl58Px Dc1OgZvO5Seaduo/8Z6HeEih+Ko/jDG5gH/mLD2K7NpDGu2NWMPq9yS9bVyotEGSyrsp oZbw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713261215; x=1713866015; h=content-transfer-encoding:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to; bh=8dWBKU5D27aRchDLY2aDBlpODU4i4Munl9LNc7I7ZXQ=; b=qi5kvSyCC++Yvusiu61lVA0rv+gA1DwelGALTshMvB18XBxuBZXPCKrGo1XD612ZNm e7CF8/8aIYtUl04gmOggUx4BsyUjQepuUf0Gqo6BgX8n8inQdteQQMD7XKcNHy3w+agf TNvHDZ3zt4ea6oXexap9/Cfk9QJ7+3eSDXwJGZwIakmv3Kf0HLa9mfuN++jC85njeS2O AIAIRLAazi/Pg+GXW94vRQnCZ7EVyh/KlbOVWwZEDkKqIVqBfqO29DkJkq7EzhUtCDnA yEzfV2/Qf3kbkgWOOColMLad/bq5KYdG2YoVTQ7NuMUjjOE8BXUKO7qzqPlIdBfmXGPg axHQ== X-Forwarded-Encrypted: i=1; AJvYcCUAW2FpeYgBbEg4NUXSi3uBKJ0yoJ8WedFQO6mBtlTdhjXUTMICYtHmp3xQ+q016LQEALzmaNj2taPVV1vb+s1fbuI= X-Gm-Message-State: AOJu0YwumVlIS5uwdxKWVEsnPxdUt8TYCk/Qft8eud1y2TZ8KVsjWnvY xY6zRzdjau07L0+geRIKY9UmU2K4+5zCUhibd3udK+YqRVcLx+8aBDh1DMtYMprNDchhK8JqvF6 R1R7I8B+lkxUkTw== X-Google-Smtp-Source: AGHT+IHcsJ5hw/vAV99RGLFobjm/zsOzmQtTDncHcg7emEQfzy1uf60bL8m87fTCzGgApod6m1hAcwICRv+RptI= X-Received: from aliceryhl2.c.googlers.com ([fda3:e722:ac3:cc00:68:949d:c0a8:572]) (user=aliceryhl job=sendgmr) by 2002:a05:6512:3411:b0:518:a967:e2b3 with SMTP id i17-20020a056512341100b00518a967e2b3mr9083lfr.0.1713261215426; Tue, 16 Apr 2024 02:53:35 -0700 (PDT) Date: Tue, 16 Apr 2024 09:53:32 +0000 In-Reply-To: Mime-Version: 1.0 References: X-Mailer: git-send-email 2.44.0.683.g7961c838ac-goog Message-ID: <20240416095333.1108884-1-aliceryhl@google.com> Subject: Re: [PATCH v5 4/4] rust: add abstraction for `struct page` From: Alice Ryhl To: tmgross@umich.edu Cc: a.hindborg@samsung.com, akpm@linux-foundation.org, alex.gaynor@gmail.com, aliceryhl@google.com, arnd@arndb.de, arve@android.com, benno.lossin@proton.me, bjorn3_gh@protonmail.com, boqun.feng@gmail.com, brauner@kernel.org, cmllamas@google.com, gary@garyguo.net, gregkh@linuxfoundation.org, joel@joelfernandes.org, keescook@chromium.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, maco@android.com, ojeda@kernel.org, rust-for-linux@vger.kernel.org, surenb@google.com, tkjos@android.com, viro@zeniv.linux.org.uk, wedsonaf@gmail.com, willy@infradead.org Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 17433A0007 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: df6hnj79iuchzpjxus4tdunogd9pysfa X-HE-Tag: 1713261216-297057 X-HE-Meta: U2FsdGVkX18wB14UTRCkaORPtMPVvNRbWqWutxxunsPNc0883+3Uoc114XIqpWGyxQ0Tc9xzwJWx1d+SA9i90Jf49LGJCVyg8yYXtOAbrjuJsYp2cyHBK3sKU4EOEDMt7kTAlvr6PHLpVYoxrDaqbx+2d84stSHsR/2cdQQ3c9LhyQzcLgmh+4nYitYgOde8qtXncKfl4Qim6Osd3C6t9hTEDuhB4bcjismFVMEcPqOZ9egbNql/pIdkVWjiOYXvd6D2JhIZGsKkM+ynqXhc2VuGBCkL4wvSMmxU9XhnUNk+DmBn8X430RY1LkTiMh2AkNYtMVhsap9Oz4R00V/ebmF9UFpf+iLcoDUYbKFJ5oBxH4OAXFGy5HPSVpl3sQ+q1jSMznUaCeC804Q6XPkesaDkuNV8TduPINnZ544CuvYqMSDkIsb+S+GtJaulc6tdq/1/Z8x8EQYkcG802/ZLgWVxHuhtt3cNoAsZ8PY1/pZG24rztiec2k7+6JscN97I+jyeejJpLTce6ZmQQ9nCS8tGNecrRmzX/XfGPXrYoj1FGOxqlsIxMj9UAtg77wDjOT2G3TvySqphEE2WbyKoW6NLppI8lJeDGa5Hc00SXVmorIbciIftKOKoVeFbfXpVrSNMi9Perr8PHkO0NpugvaIMMvp1kcYt/f6GZYaXOw3GD+YbpNbBUTh7bGYzJ8iHNKtMDwdcxT/ZGYDY3hZjgTj5EPJ4DdXVX1E19xuIvT1OLsbfngd6NHB4Op/Cj3/VnZSlvHiEuuSM/mdMfWVSs4ELbbcXH7JFLMIW2nMQ2PEV23ve6/fvOG8zD+gSFcURXHbWrUDw2PuPEpXRDRcsz1vzzve6/JUr3oKZNHUu5QmJkZbwwPlheu4YPm7Q8mxNFC9OUMg+Fck4E8MxxFudc1bFIiR/YQkF+FAFkL7kIcvuvMgKMuv7SG74xIfn/2mYYqLvH+QBrbWhyyfBKp7 Tkw/YLs7 Vo9tK5NG52EfVyViadKBHCB4wVeotwGoyRnwpJU5NyZHYYkzayIeLkClamCqxJmSZPss8okGoIgg9rPY/PRYtxW4peTJZBZgaUxnJJKykBJrPa1lVjnLR0KcoQjCCGVL4qFAb1tHb/KZnE5YTGBSQfF3ztkEB/lRWRptTVIhiDKIWCiDRgl8uKx+bg3gqglCf4t5sAby22+GNQ+9NKuosW1+3WogmNzCdKkkoc6o47wxGUj8btkz5JTyDMGxvdRQU4y3n0xLWv0dFmd7diVUJtj0hyc3QHc1FjDZPZKVLEBLlEbyfKf2R7pcAiVVcGa2eE4S7drYlfYtMmHHbmRhq6jQdlRut9bEi21DCwneopbKhc+uSwQCZGtql4BE9PXstzwc6NxknH+LspqibSFls58IQFvlxDgEnl3WLVegfHwhndvXduSHIxMZ7OeJtCUwyBDmQ4TfurWASL7XS9m9RfeuNCBE16Hq2QUa1p098JS+uTL9YiFeCU6A6fSIjlFrHW/8ZorSZndxpNMEjZoIta+ASRExhLjSSpuBNWE/L1nVoUhldWGWpNo07rzx81mpF60NiW8oyc514vwEcauPfq03S7yIp2M1Xymk6ktkru5dpwk+90ufveuqKU12lNGyQOKBEMAaRKFMfPPKURB2LmpKldgwtnlVMppmUTh1VFTatT3kXf6xQnc2YKWNGE8/qqeLNozyRpunAZUup0phC/XnnzQj6fMReOLF1t95uIJNG/mkNxbLIURg2vXsHLlXX1ba4Hh0IWOupNq9zAej1xctkfFjfeQMw6Clha68edUWYTcuy7nXWDjhYNw== 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: Trevor Gross writes: > On Mon, Apr 15, 2024 at 3:15=E2=80=AFAM 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. >> >> 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 >=20 > 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. >=20 > Reviewed-by: Trevor Gross Thanks for taking a look! >> +/// Flags for the "get free page" function that underlies all memory al= locations. >> +pub mod flags { >> + /// gfp flags. >=20 > Uppercase acronym, maybe with a description: >=20 > GFP (Get Free Page) flags. >=20 >> + #[allow(non_camel_case_types)] >> + pub type gfp_t =3D bindings::gfp_t; >=20 > Why not GfpFlags, do we do this elsewhere? >=20 >> + /// `GFP_KERNEL` is typical for kernel-internal allocations. The ca= ller 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 locate= d in high memory. >> + pub const __GFP_HIGHMEM: gfp_t =3D bindings::__GFP_HIGHMEM; >=20 > 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? All of this is going away in the next version because it will be based on [1], which defines the gfp flags type for us. [1]: https://lore.kernel.org/rust-for-linux/20240328013603.206764-1-wedsona= f@gmail.com/ >> + >> +impl Page { >> + /// Allocates a new page. >=20 > Could you add a small example here? I can add an example that shows how to pass gfp flags. >> + pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result { >> [...] >> + } >> + >> + /// Returns a raw pointer to the page. >=20 > 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? I can clarify that it's a pointer to the `struct page` and not the actual PAGE_SIZE bytes stored in the page, and that it's for use with the raw C apis. I won't go into more details than that. >> + 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 correctl= y. The pointer is valid for >> + /// `PAGE_SIZE` 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= , dereferencing it on other >> + /// threads is UB. Other than that, the usual rules for dereferenci= ng 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 th= ey may reference with >> + /// different addresses. However, even if the addresses are differe= nt, 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 { >> [...] >> + } >=20 > Could you add an example of how to use this correctly? This is a private function, you're not supposed to use it directly. Anyone who is modifying this file directly can look at the existing users for examples. >> + /// 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 po= ints at `off` bytes into the >> + /// page, and the pointer will be valid for at least `len` bytes. T= he 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, the= n 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 correctl= y. 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, derefe= rencing 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 th= ey may reference with >> + /// different addresses. However, even if the addresses are differe= nt, 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). >=20 > 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..." Same as above. This is a private function. >> + 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 `= offset .. 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 clos= ure, 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(()) >> + }) >> + } >=20 > Could this be named `fill_zero_raw` to leave room for a safe > `fill_zero(&mut self, ...)`? I suppose I can rename these _raw as well. >> + /// Copies data from userspace into this page. >> + /// >> + /// This method will perform bounds checks on the page offset. If `= offset .. offset+len` goes >> + /// outside ot the page, then this call returns `EINVAL`. >> + /// >> + /// Like the other `UserSliceReader` methods, data races are allowe= d 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 clos= ure, 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 guarantee= s that there are no races. >> + reader.read_raw(unsafe { core::slice::from_raw_parts_mut(ds= t.cast(), len) }) >> + }) >> + } >> +} >=20 > Same as above, `copy_from_user_slice_raw` would leave room for a safe API= . Okay Alice