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 CA82EC3DA63 for ; Fri, 26 Jul 2024 08:11:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4BBB56B0092; Fri, 26 Jul 2024 04:11:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 444C16B0099; Fri, 26 Jul 2024 04:11:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 30C826B009B; Fri, 26 Jul 2024 04:11:10 -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 12B746B0092 for ; Fri, 26 Jul 2024 04:11:10 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 80F02A1665 for ; Fri, 26 Jul 2024 08:11:09 +0000 (UTC) X-FDA: 82381183458.17.BA7378C Received: from mail-4316.protonmail.ch (mail-4316.protonmail.ch [185.70.43.16]) by imf05.hostedemail.com (Postfix) with ESMTP id 933E5100017 for ; Fri, 26 Jul 2024 08:11:06 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=TlLy6pTi; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf05.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.16 as permitted sender) smtp.mailfrom=benno.lossin@proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1721981465; a=rsa-sha256; cv=none; b=J1+zGQjXT584BCA+Rb4XhxTqvA2ck5iK+XjTHMiqQu3o9uEsSzwlsVkSRQdxrt2yM9GxGO sV0IW8NkmWSQAnkecGYSBKy+738ArDOuHn7Tw1znFIqXY+WOjmZM+vYlw+tz80sj+4DgJM cnIOgUaEtwgFPE+r4OKx2iiyCst5OJM= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=TlLy6pTi; dmarc=pass (policy=quarantine) header.from=proton.me; spf=pass (imf05.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.16 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=1721981465; 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=Tb0bny82xw3N+xnAGMVpo1hM7rDm1n5+a9liM28zCuw=; b=CV5FPb8T17kN1ZQXmdYTCG3IU7zDD0kWyck+kUY3iWi8KgBZ2A+macCw0EbUau/h+SqO7M yxYthxYavuvqh/DqDljLmYkzjYJGvFulvIkLk/lZtfT9Fhe5aM4ph6ODuAurV21ie/Vb8Q jPRgmJQULZF45L1xrzzbQyMAEk7sgkk= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1721981463; x=1722240663; bh=Tb0bny82xw3N+xnAGMVpo1hM7rDm1n5+a9liM28zCuw=; 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=TlLy6pTi5sm5nCahQ0gIP4yk5KAf7jnvliUr9AFllJT9uPlAjgDd+bcaIxUh1CuhM +zX4hcau5TXNEWrUN0XcesR/ZfxWda0uYJq03z5Tc6eBM3xrqOKtUIHlTYLZ+MnIY9 uKybJ+q/dxbKasr5C18nS4zPxx6vymFpilRfFujJuCZb45O4oUn2KRQh+rqJW/ueDi bp1/HaMOPchxX9ZTxmr9VeDNu5YG8o7hYb/eXlmyf8Df2vfw4Kgq6oyjnhAcx+NMRc EjRCdYgB2EsyCHUO/HQhVWFrW7ayoxCradonct5ezsI7VFkzztQJiYyxnjm6W83xNR 7e1jO+kkH2bAg== Date: Fri, 26 Jul 2024 08:10:58 +0000 To: Alice Ryhl , Miguel Ojeda , Andrew Morton From: Benno Lossin Cc: Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org Subject: Re: [PATCH] rust: mm: add abstractions for mm_struct and vm_area_struct Message-ID: <3bf6bfdc-84af-442a-acec-a58f023d1164@proton.me> In-Reply-To: <20240723-vma-v1-1-32ad5a0118ee@google.com> References: <20240723-vma-v1-1-32ad5a0118ee@google.com> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: a645426bb3ef230cdfb577940ac1e8add4755bc6 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 933E5100017 X-Rspamd-Server: rspam01 X-Stat-Signature: i8eqdtkh6gahojfgknw51irtdinnmkdi X-HE-Tag: 1721981466-639303 X-HE-Meta: U2FsdGVkX19oy6tlwq3vQFsXhKPYpRQSSRUIEJsSoLkEPkIiP8/tGmADX27LajSnBD+pnh6QeDGzozYRZ5g36ldvMJ7CxPqyRceNDqocIWtK0PbM76HhmHXCHyidbgOKvJScY7CohZC5YLuzq9Y9UX8U0SlYIHRQNV51+oIr7xN92tgkU8KS2+5WFSfwVtVTCvt3qQjRk3SNBJ3JqS9QQzn4/iHGuOz0R6N9n//kStnmvSakDgXVKbR/YwuzMX0uH1tmd50kdorTuVm9Va1JWmve9FuYf44qu1OiqBSDemSsQGkbIOgS2zDL9s8pfu2JOJXpwzx3UscAjEByeIMVCbfBjMK4F/quVABQoUuVbtr7M6qxu+Mbry6yWVR23QzNu4pMjN2Y47D5BHfY1oUh4OOTgr5dg4nD52EHwsQDB/K4G9+Eg8bJ4b3v0pac8esh9i1JORd/5jB2GnU8lzHKwKrXwVP9ArgW9ujP7wsyeV/x+j7y9Yi84CFnzTfmDOoBXJRv9bwmy/NVrG8eg5AH1nERrLUFLZf0CEEgANBUNF5QQ6o62GZHt4vKzHlzeMoNYTOx8hhLb0gmVY45EJ3acIE/lFwgAkob6xUULYqa0Ap2elzH1H9/O0O2BArAgyYKLIFpe2x3KYpW2i0raACDxVJIp6o3TbyDz2vLw/az6ks+oXC4DpClG14tsdN482REc6s+Kgf9mcqSA2cNoa6fDeLsFHepBl9Zd/+tbjCyKT5AYPMeT6M/6j50QFkGmFBnaPvKX4DY0YBCiHEhwVCQWJAk3gP2JBzJirVv8E92aWRhEMWCshmndXZdUV1gy4rP17n8f1lS0620tTn4Rp4w/HO8T7qXLUzg0r1FKsRlUSHA4jik8WLRSdGbPRbs57EDilUW0EgZZjHZS1HbYQ18mZLOF/RU/LKjFiKi/eB+39OrvyW5XjfHDFAY2Umy5RhoXnhK+RvxZs8auAw24bu jt/grPYV Z3Si7eoIYuRRJe7jGlN2ToTO68MWpQw/avBgM5SXJqukOCkvgJnaxpG2748vfBhz+g+vEQ6RxgOZjB75UYWNjuRy0cooJbLPX6+8vChMGrwD6ZueVucRd2rn/pUCLWUQuzVLgzyK8n6k5RNXO9mR4FLOI8bcLNgKRlM35/beJc1Ts1SPSBRZPaW94R2MOfqIEuwI983XWg1Bhzc9h4WWtJ2L8kPXoy8GobU1uZdpisTVDb7NdPNGRlc4eAHZsswMNlm+tGj2G3cIRa6gZWQQ+Suj++r3EARrBfftiFuPMgpURgIeomHXhodLGJz+LQKS42d5hmdZx1AiwW+KJu+RnU+hpGtLW/rXuip2OB3p+J7f3cINFzHEeG6IaZJN5TTvO+0KBEhjvCKhrR0+iAyzCiqOrlOm8SxMF6A/ycquKv4qpzTTTUYmTrRV7S6BYQSgETBJh3hIZHyyCoA/SzmvKVQ+uBqyfoQQ6uPqEjUpVwcWLcv/CxJoSBaVDuD8LeVnoSUXG 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 23.07.24 16:32, Alice Ryhl wrote: > This is a follow-up to the page abstractions [1] that were recently > merged in 6.11. Rust Binder will need these abstractions to manipulate > the vma in its implementation of the mmap fop on the Binder file. >=20 > The ARef wrapper is not used for mm_struct because there are several > different types of refcounts. I am confused, why can't you use the `ARef` wrapper for the different types that you create below? > This patch is based on Wedson's implementation on the old rust branch, > but has been changed significantly. All mistakes are Alice's. >=20 > Link: https://lore.kernel.org/r/20240528-alice-mm-v7-4-78222c31b8f4@googl= e.com [1] > Co-developed-by: Wedson Almeida Filho > Signed-off-by: Wedson Almeida Filho > Signed-off-by: Alice Ryhl > --- > rust/helpers.c | 49 ++++++++++ > rust/kernel/lib.rs | 1 + > rust/kernel/mm.rs | 259 +++++++++++++++++++++++++++++++++++++++++++= ++++++ > rust/kernel/mm/virt.rs | 193 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 502 insertions(+) >=20 > diff --git a/rust/helpers.c b/rust/helpers.c > index 305f0577fae9..9aa5150ebe26 100644 > --- a/rust/helpers.c > +++ b/rust/helpers.c > @@ -199,6 +199,55 @@ rust_helper_krealloc(const void *objp, size_t new_si= ze, gfp_t flags) > } > EXPORT_SYMBOL_GPL(rust_helper_krealloc); >=20 > +void rust_helper_mmgrab(struct mm_struct *mm) > +{ > +=09mmgrab(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmgrab); > + > +void rust_helper_mmdrop(struct mm_struct *mm) > +{ > +=09mmdrop(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmdrop); > + > +bool rust_helper_mmget_not_zero(struct mm_struct *mm) > +{ > +=09return mmget_not_zero(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmget_not_zero); > + > +bool rust_helper_mmap_read_trylock(struct mm_struct *mm) > +{ > +=09return mmap_read_trylock(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmap_read_trylock); > + > +void rust_helper_mmap_read_unlock(struct mm_struct *mm) > +{ > +=09mmap_read_unlock(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmap_read_unlock); > + > +void rust_helper_mmap_write_lock(struct mm_struct *mm) > +{ > +=09mmap_write_lock(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmap_write_lock); > + > +void rust_helper_mmap_write_unlock(struct mm_struct *mm) > +{ > +=09mmap_write_unlock(mm); > +} > +EXPORT_SYMBOL_GPL(rust_helper_mmap_write_unlock); > + > +struct vm_area_struct *rust_helper_vma_lookup(struct mm_struct *mm, > +=09=09=09=09=09 unsigned long addr) > +{ > +=09return vma_lookup(mm, addr); > +} > +EXPORT_SYMBOL_GPL(rust_helper_vma_lookup); > + > /* > * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we c= an > * use it in contexts where Rust expects a `usize` like slice (array) in= dices. > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 5d310e79485f..3cbc4cf847a2 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -33,6 +33,7 @@ > pub mod ioctl; > #[cfg(CONFIG_KUNIT)] > pub mod kunit; > +pub mod mm; > #[cfg(CONFIG_NET)] > pub mod net; > pub mod page; > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs > new file mode 100644 > index 000000000000..7fa1e2431944 > --- /dev/null > +++ b/rust/kernel/mm.rs > @@ -0,0 +1,259 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +// Copyright (C) 2024 Google LLC. > + > +//! Memory management. > +//! > +//! C header: [`include/linux/mm.h`](../../../../include/linux/mm.h) > + > +use crate::bindings; > + > +use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull}; > + > +pub mod virt; > + > +/// A smart pointer that references a `struct mm` and owns an `mmgrab` r= efcount. > +/// > +/// # Invariants > +/// > +/// An `MmGrab` owns an `mmgrab` refcount to the inner `struct mm_struct= `. You also need that `mm` is a valid pointer. > +pub struct MmGrab { > + mm: NonNull, > +} > + > +impl MmGrab { > + /// Call `mmgrab` on `current.mm`. > + #[inline] > + pub fn mmgrab_current() -> Option { > + // SAFETY: It's safe to get the `mm` field from current. > + let mm =3D unsafe { > + let current =3D bindings::get_current(); > + (*current).mm > + }; > + > + let mm =3D NonNull::new(mm)?; > + > + // SAFETY: We just checked that `mm` is not null. > + unsafe { bindings::mmgrab(mm.as_ptr()) }; > + > + // INVARIANT: We just created an `mmgrab` refcount. > + Some(Self { mm }) > + } > + > + /// Check whether this vma is associated with this mm. > + #[inline] > + pub fn is_same_mm(&self, area: &virt::Area) -> bool { > + // SAFETY: The `vm_mm` field of the area is immutable, so we can= read it without > + // synchronization. > + let vm_mm =3D unsafe { (*area.as_ptr()).vm_mm }; > + > + vm_mm =3D=3D self.mm.as_ptr() > + } > + > + /// Calls `mmget_not_zero` and returns a handle if it succeeds. > + #[inline] > + pub fn mmget_not_zero(&self) -> Option { > + // SAFETY: We know that `mm` is still valid since we hold an `mm= grab` refcount. > + let success =3D unsafe { bindings::mmget_not_zero(self.mm.as_ptr= ()) }; > + > + if success { > + Some(MmGet { mm: self.mm }) > + } else { > + None > + } > + } > +} > + > +// SAFETY: It is safe to call `mmdrop` on another thread than where `mmg= rab` was called. > +unsafe impl Send for MmGrab {} > +// SAFETY: All methods on this struct are safe to call in parallel from = several threads. > +unsafe impl Sync for MmGrab {} > + > +impl Drop for MmGrab { > + #[inline] > + fn drop(&mut self) { > + // SAFETY: This gives up an `mmgrab` refcount to a valid `struct= mm_struct`. > + // INVARIANT: We own an `mmgrab` refcount, so we can give it up. This INVARIANT comment seems out of place and the SAFETY comment should probably be "By the type invariant of `Self`, we own an `mmgrab` refcount and `self.mm` is valid.". > + unsafe { bindings::mmdrop(self.mm.as_ptr()) }; > + } > +} > + > +/// A smart pointer that references a `struct mm` and owns an `mmget` re= fcount. > +/// > +/// Values of this type are created using [`MmGrab::mmget_not_zero`]. > +/// > +/// # Invariants > +/// > +/// An `MmGet` owns an `mmget` refcount to the inner `struct mm_struct`. Ditto with the valid pointer here and below. > +pub struct MmGet { > + mm: NonNull, > +} > + > +impl MmGet { > + /// Lock the mmap read lock. > + #[inline] > + pub fn mmap_write_lock(&self) -> MmapWriteLock<'_> { > + // SAFETY: The pointer is valid since we hold a refcount. > + unsafe { bindings::mmap_write_lock(self.mm.as_ptr()) }; > + > + // INVARIANT: We just acquired the write lock, so we can transfe= r to this guard. > + // > + // The signature of this function ensures that the `MmapWriteLoc= k` will not outlive this > + // `mmget` refcount. > + MmapWriteLock { > + mm: self.mm, > + _lifetime: PhantomData, > + } > + } > + > + /// When dropping this refcount, use `mmput_async` instead of `mmput= `. I don't get this comment. > + #[inline] > + pub fn use_async_put(self) -> MmGetAsync { > + // Disable destructor of `self`. > + let me =3D ManuallyDrop::new(self); > + > + MmGetAsync { mm: me.mm } > + } > +} > + > +impl Drop for MmGet { > + #[inline] > + fn drop(&mut self) { > + // SAFETY: We acquired a refcount when creating this object. You can just copy-paste the SAFETY comment from above (if you don't use the `ARef` pattern). Ditto below. --- Cheers, Benno > + unsafe { bindings::mmput(self.mm.as_ptr()) }; > + } > +} > +