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 4FE97C48BF6 for ; Sat, 16 Mar 2024 20:40:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4BA5A6B007B; Sat, 16 Mar 2024 16:40:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4428B6B0083; Sat, 16 Mar 2024 16:40:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2BB976B0085; Sat, 16 Mar 2024 16:40:06 -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 140CB6B007B for ; Sat, 16 Mar 2024 16:40:06 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 8A93E40558 for ; Sat, 16 Mar 2024 20:40:05 +0000 (UTC) X-FDA: 81904069170.11.EDAD39F Received: from mail-4322.protonmail.ch (mail-4322.protonmail.ch [185.70.43.22]) by imf04.hostedemail.com (Postfix) with ESMTP id 3AC3B40019 for ; Sat, 16 Mar 2024 20:40:02 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=JgGHx0uu; spf=pass (imf04.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.22 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710621604; a=rsa-sha256; cv=none; b=Ok88begf5S6FdgqgdbOpe8TTfctp6Y3B2R4V2+/kbh7fnfL1bW7HM2K84oZ9wOP4MuzBXn ZBekHqk9EK1h21i9GziJOmsvJVI57+IeS6XNICybBAEBKVo5kpoxiU1CIsI4EEDuYqGwgm 5oC8kdhRAjwrPiCuaRNxsD+/1d/ZN0Y= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=JgGHx0uu; spf=pass (imf04.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.22 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710621604; 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=A+6SILfQFnqsXEoZHJxtgDtsdF1CEuXy/QosmJOt4ws=; b=iA7VeOzdLJjCo+5HTCIpsxggBAHTlfRBceGnTwuBclwdl6LZeWQpH8t/BM632rZMPjhplo rVHbZ7jvc+jWuGTZCzEZzyedAi4A8DqJ4+88ljCcMzqz1oF+UDqrrQ5iCSral29HkWynTU LSdyNAZ0hLQE93di9Mdxj/ClhnAcQQ0= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1710621600; x=1710880800; bh=A+6SILfQFnqsXEoZHJxtgDtsdF1CEuXy/QosmJOt4ws=; 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=JgGHx0uuMKBY3EgucoOqQigFj4ZnBofZ9IxIHHWScg/kbyb+yXdlWDAmFJFDQ3OuR fpumksGePiOKvE4YlfElV5zQSq3SbTtpFQYFj7UuWR93EyVxgHXpOhFfvhN2zcIpG4 Kmhj+3GyqOp9Ga/sVM5GgYasYfzvMFBHFD2zkK0KbSNvy38WRXlYd3CiJlntNnO5nB JnFk46P9Lc7TJ9TlljquzGRU5Lg7Y9K1N/aMvDAtCWpY8lAWnXDTcmeZXPQeF9jF79 hIL8H81YQH0qu7QHhjNjj2BwC3jt78ZydYOdynUzsBHJqKMv4Uf4e1MvT/PK5IqGNx h6hMuqChE8Rhw== Date: Sat, 16 Mar 2024 20:39:34 +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 v3 4/4] rust: add abstraction for `struct page` Message-ID: In-Reply-To: <20240311-alice-mm-v3-4-cdf7b3a2049c@google.com> References: <20240311-alice-mm-v3-0-cdf7b3a2049c@google.com> <20240311-alice-mm-v3-4-cdf7b3a2049c@google.com> Feedback-ID: 71780778:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 3AC3B40019 X-Stat-Signature: c7tj9p6kmb59bzhse63sansz9miw8trg X-Rspam-User: X-HE-Tag: 1710621602-887010 X-HE-Meta: U2FsdGVkX19DQN5Km2B2PzzPdU7BqKXaeBpdYyPJ6ehHarh2xeUGynEKT+GyYn9ebxnnJ7sMQeIESC8e5RxTLbU76Fa9Tl0r4gAhiouErkM48yKrAa0iKM/nrd3/sEayRD9gdCphTHN4tf+NUEIrSZjiD9CmmoaU6UfrJ9cB32AMGziM3+2L3ymidPmC0aMCT8KTvaXIA2VrE0wuorO7JG6KC9U/BBHzoxInibKLuWfoMgHFh9XpGWia5xd0zX8I9jn/KqgDXUWM8Tqrc7SIDUQLhAwdwanAUlfwqjcDBd30ju20+yaVxFx+1jzmGpWO0qxoKh+UcxagSF0jw3KOFLWPd2SvyFaSns5UfXyHAv0YGbZyUe7C+0NMH798BFJUsrTkaRY+g5e7igbzycdocAr5ykgd8CnXGyyRXuN5pV/iMxyEwjgMvIH+r46CASmHCXAiUKTjsV4ZHsUvrtNiJw2BXLronKBFg+XYDgcfLwUFwqzIWrYhFeaU8TxeqbBc/1qe8aLP739ZD+6o28NTd1raVIRWrgs6ZcKAxwj/hMeV1x8+E5hCAwZo9+e+lKjwq9wrc3ID7npiSjEeptgOJtkRYBYhMgjBKsgVDzUnMGv3qKDnhgHE+/ZIxVVZWD6aX0y93vUmKcWfyxAuLPdX1Ld3eUKbt0WY4ztJg19FtX1rM7bGJ1C+1aY7wpKzKGBFK+3oYtLKw3zg8n8TWSXbqEwbrAIBmCsAfYw3ZzjfMkGyZhyJxWqpMmnChjr/tOk3cHozYoG27Ykn7w4gy2DPlNQILD2YdpFtn3tBOQ3sMZWPciUIML6EDyAvkPc/NgNM9Mm3d4tgsut/R6EWpCeUiIOQR4YPyzcW152ScBLHKnbbRSNu64+SGrrGP6AQ2lL9VSGlQsomodCw+eaF8mBnx+QEHejdLmB1kd0J0OH1KuJDE4s59dpIzD6VIkiyNZQuLI2A9ednmUTkmCOTzPm BjOle5q6 B1HdCGw7cvTHWJJtVf0hBbMLk+PyQi0HTcQByKXbjbMXv+X92+VvuYE2Vzsw+BIbXab2FJHbLhvNRRyocK9a2GtqIQHGFmKmDonJHtgxCDa5mhCJF2PHO6yz5ON3x2jFXZHnP3ED9Z+bwMKEv+EJArKhRB9zx6F1xpY/tZSKyGPZWaryEaOlE4sadOjinXZKi+31wOvfkrOqjb4SMcWIMtimc6yS8BaQRkDvoL0+okEsW5bIPXYXsg/LxVli6gTcnzLZi77XpnhQ8s9Az+LLrmXBNEaDLc7+BLB4+n3FotnH3x523LRwnhYqjJ9oLQ2eco1zh 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 3/11/24 11:47, Alice Ryhl wrote: > +/// A pointer to a page that owns the page allocation. > +/// > +/// # Invariants > +/// > +/// The pointer points at a page, and has ownership over the page. Why not "`page` is valid"? Do you mean by ownership of the page that `page` has ownership of the allocation, or does that entail any other property/privilege? > +pub struct Page { > + page: NonNull, > +} > + > +// SAFETY: It is safe to transfer page allocations between threads. Why? > +unsafe impl Send for Page {} > + > +// SAFETY: As long as the safety requirements for `&self` methods on thi= s type > +// are followed, there is no problem with calling them in parallel. Why? > +unsafe impl Sync for Page {} > + > +impl Page { > + /// Allocates a new page. > + pub fn alloc_page(gfp_flags: flags::gfp_t) -> Result { > + // SAFETY: The specified order is zero and we want one page. This doesn't explain why it is sound to call the function. I expect that it is always sound to call this function with valid arguments. > + let page =3D unsafe { bindings::alloc_pages(gfp_flags, 0) }; > + let page =3D NonNull::new(page).ok_or(AllocError)?; > + // INVARIANT: We checked that the allocation succeeded. Doesn't mention ownership. > + 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. > + /// > + /// It is up to the caller to use the provided raw pointer correctly= . This says nothing about what 'correctly' means. What I gathered from the implementation is that the supplied pointer is valid for the execution of `f` for `PAGE_SIZE` bytes. What other things are you allowed to rely upon? Is it really OK for this function to be called from multiple threads? Could that not result in the same page being mapped multiple times? If that is fine, what about potential data races when two threads write to the pointer given to `f`? > + pub 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()); > + > + // SAFETY: This unmaps the page mapped above. This doesn't explain why it is sound. > + // > + // Since this API takes the user code as a closure, it can only = be used > + // in a manner where the pages are unmapped in reverse order. Th= is 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 > + } > + > + /// 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. The pointer is only valid on this task, as this met= hod uses > + /// a local mapping. This information about the pointer only being valid on this task should also apply to `with_page_mapped`, right? > + /// > + /// If `off` and `len` refers to a region outside of this page, then= this > + /// method returns `EINVAL` and does not call `f`. > + /// > + /// It is up to the caller to use the provided raw pointer correctly= . Again, please specify what 'correctly' means. --=20 Cheers, Benno > + pub fn with_pointer_into_page( > + &self, > + off: usize, > + len: usize, > + f: impl FnOnce(*mut u8) -> Result, > + ) -> Result { > + let bounds_ok =3D off <=3D PAGE_SIZE && len <=3D PAGE_SIZE && (o= ff + len) <=3D PAGE_SIZE; > + > + if bounds_ok { > + self.with_page_mapped(move |page_addr| { > + // SAFETY: The `off` integer is at most `PAGE_SIZE`, so = this pointer offset will > + // result in a pointer that is in bounds or one off the = end of the page. > + f(unsafe { page_addr.add(off) }) > + }) > + } else { > + Err(EINVAL) > + } > + }