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 B60C8C02194 for ; Tue, 4 Feb 2025 12:45:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DB0F06B007B; Tue, 4 Feb 2025 07:45:47 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D88726B0083; Tue, 4 Feb 2025 07:45:47 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C287E6B0085; Tue, 4 Feb 2025 07:45:47 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id A59016B007B for ; Tue, 4 Feb 2025 07:45:47 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D6353120AF5 for ; Tue, 4 Feb 2025 12:45:16 +0000 (UTC) X-FDA: 83082232674.24.D3BBFE0 Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by imf21.hostedemail.com (Postfix) with ESMTP id CD9491C0010 for ; Tue, 4 Feb 2025 12:45:14 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=W6OdVL2u; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of aliceryhl@google.com designates 209.85.128.46 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1738673114; a=rsa-sha256; cv=none; b=SDHHErK+UFIGER/AySaRsyFE0wG14jbCE5n8yuGgRxm2YAPUjirbuHy0TAnePSqYlS1V5c o0nuA4bhW0YwzC2XrE4ge+G0o/naIukhEqjFRYQngxihOUIq0TmcH3VqrL+VBy4O+sN8NI 4GtL01+go73ai82wQSyTHIMJ1hXDqho= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=W6OdVL2u; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf21.hostedemail.com: domain of aliceryhl@google.com designates 209.85.128.46 as permitted sender) smtp.mailfrom=aliceryhl@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1738673114; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=LoXJE3ykK2IyJltJSvnFg+xcHFmtht2j+zENZGxvBgs=; b=MVOAF1sCzaJXLE8E6s6rIyOhcAq9OteWxOZbYySASZElJR4ecIZu+co4BH7vfSn6fwZ4V7 ZYQ8fYcGcijkAn2Dya46IvjcZ2WBDGBIyWXpIX9gIIjr44x2SU9qXc7KjyfnI6qByVb0pc jupbaH+cXUedTHJtPO4QL/qRKVp0wIg= Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-436326dcb1cso38288305e9.0 for ; Tue, 04 Feb 2025 04:45:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1738673113; x=1739277913; darn=kvack.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=LoXJE3ykK2IyJltJSvnFg+xcHFmtht2j+zENZGxvBgs=; b=W6OdVL2uOjPpCtQpHT4svhoQmvvfsUHJZ6zFCaiJ4fZ2LkHKXfk625bIbZEAnzFb6d RfVpmLG6YLKFrmgV5zAUx5QBzkWE6Uvs3R8rQ34bHNvHJpZQj5iwSk9xo+B1V34rz1Fp YmO6rYWssS8RAzDE+mLSxYepwrAcIQrXXw6AnJrIGb0DUs+BPkW8UzCIS3DiCPT3Mrxn 8JhMgrw3dJXJBUw57QcWs7xSFe29/hw+L+ZI4Vpj7BIep5xpcbX9yci87irCfNtf42M1 IWGB39Q/bfGVkgN4Zm64QDPlrR2QWuOWC5EIVzJSxQ88jHspfwiBUouLdO4eb3MDeCfj Evpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738673113; x=1739277913; h=content-transfer-encoding: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=LoXJE3ykK2IyJltJSvnFg+xcHFmtht2j+zENZGxvBgs=; b=GdIi69ObV1y9l3FENsrurhvJgsvdCzNL3Tx4+hdthLknIMSrXcNH7GilgLQ7dCF7to ELPOvTdiDoUBiLPAeuMVdo5iYgHNyIJ+QVrlojdA1omVE2U4DCyotI98shdWHDGBiYd+ 1uNnn82RpZX5m27dWyJK5bTMsD0YHrm/lTBg0UwJnsNVr/NUWzviHBSCZrGPKoTc3F7/ 1d+7wdrNzmV8cXXiHLgCErpzd+sk5Lzak0f/JxNajad6kU1tI796R7uKllDRpYuQAuZm bYAuy6alYN8r2Kpgseb7hUEsDuqoWix6OjcLhQCDtxvchhQ3KJEH0O/k/B9LEC0w9lDo Urxw== X-Forwarded-Encrypted: i=1; AJvYcCVWi6f8PFtr6OYooFdaybNuhG11EXQWu0xbSt9xV7fA1tAt3vcIj/p33utwAAD0geJ36sdY6VDzXw==@kvack.org X-Gm-Message-State: AOJu0YwuWojWJfrsvO+wFrmQAYhJxroE89hMghXxss2n57ojeaPDvvU2 Jz9nKjG5JqeOWxesiDASYyM1dbgkX47hrccuxo8yBRdENlebrf/TkafFJqbLM89jrS/1psugHtH rzOxByVrEo+6IoCkFdabxGFmAR2o28DxaWkii X-Gm-Gg: ASbGncsLD1ntCZ5A8yL0oifX+mBDxZb730oYPOrey5xKyf57mHk2OaVrir6fZhCO8lg 7oHdKcXney2VlDX247AN7eIvuB47iaD8j0nsodEFbtTT3oVtVBmHqTRdWlEY+FKched+NHTaOlb X86eiCqRRcS7bZH8S8cftHxivTyQ== X-Google-Smtp-Source: AGHT+IGLublWl9F89PbsgxVMnM6oF5TiMelRRa2OcDUdpOzd1+u/nfcuJ+oiHenJG/FUn+Brh6IzKP1iNPH1goM7TGo= X-Received: by 2002:a05:600c:c17:b0:436:51bb:7a52 with SMTP id 5b1f17b1804b1-438dc3bd7e3mr239782735e9.7.1738673113074; Tue, 04 Feb 2025 04:45:13 -0800 (PST) MIME-Version: 1.0 References: <20250203-vma-v13-0-2b998268a396@google.com> <20250203-vma-v13-2-2b998268a396@google.com> In-Reply-To: From: Alice Ryhl Date: Tue, 4 Feb 2025 13:45:01 +0100 X-Gm-Features: AWEUYZmZtlTW4vkVtV9EtFizlhhuOo5vqgV13DO87MCPZV7TprFw86-lpsIaGAY Message-ID: Subject: Re: [PATCH v13 2/8] mm: rust: add vm_area_struct methods that require read access To: "Liam R. Howlett" , Alice Ryhl , Miguel Ojeda , Matthew Wilcox , Lorenzo Stoakes , Vlastimil Babka , John Hubbard , Andrew Morton , Greg Kroah-Hartman , Arnd Bergmann , Jann Horn , Suren Baghdasaryan , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Trevor Gross , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: CD9491C0010 X-Stat-Signature: qqmb46uszcq83qh5u76yigpiuwaqrb6m X-Rspam-User: X-HE-Tag: 1738673114-341688 X-HE-Meta: U2FsdGVkX1/ZcvH6OEx2FaO8p3N7cVUQJv73V/BWrZVbpy5uGllbuINDPtAIf2MZP92ZQr25ddVG95/nrHBwVNd+0qUik/k3sX/QpekAKXIA0VQhu6+h6xnCeYy/uuKb557o6NI1ROwV1ZEoreIjI0dRkbmwmDnWD+5VMyvOjuK8ovhODaXnxMaAj7kTuWAcWtN7cTz/K2FKImXe/zH8Z4IGcLZ3gOxBVfjw8CLcoA87z7dpzMQ2zRy6vAbjxD5T/VtYG6oQUixQOZJo50E33uJEbl/e4aeik18lsNu0NfFmdyxrqQTZxeS8FjzXuIBz1JJB8TkB9Y0O19/dEmCB/0lwpr62KGH/SJVuf/9xt+8ZNyz17i5oLHm/gXBEPIvv3jk3P45trcNL5I1z60Sk02Xor3tuK+uppX+RIMsFJ9Jb13rMbwtfNuy7WZNcEPYd0nVAEq5Rb1aCXxVlKkKICkJyZkUOu8lfCfBCTax0w7KohU0ckWKBAeTStM1KHUjy11E7k2uvVLSc5nKWuGsVibeGnfjpg6B5MMhJT4eeq3wQ33EUuSFJDd67Nu7uH0aLOvtqinaAQwET9ssEiu0rFgVUZS+SosKAyhjsa8D5wl0bV0DP38OOqIhanjzqKl+g/VItBUDisOP8thgxMixzBX4VjYZEZ1hosf8lFT+3KstEB3cihkXl6rIbco4jHTRn9yxc1XK8J7At15keAUD5J0DLhCrNgrXRBVBBYIzekjMbCp+YUNiHQ1L9U5eOIXpwC2F+PlArbumn+peBUJzPPVrA1Vi3garOJmwwe9y3IkvbV5aJDdefHXs4WtEqpZW1sousG6Lc2YBtrgSyGF/hzb/XqTZATWSOCj34VOXuNqzk3Vf+qyIhWmOM1CzbQPNUx6BPdJcZf13KgYNWJWL9TxmZJiV0PidV4hQior7ldbN30duyo0YCJw80WmUv4eKnEdubtC+sSgYNK1eiDiz Tt7L/7ja ul+aI5ydJS8LkWWqCBnOQO+4CjeD+NRAxyPYjJKpk2F9++FnPnmFW6jaMisOpEUcBsTPeNrMSnWl37n4kykcTL6l+wQIUA4UXkuSE1uH/x8QJBYlXs0n4Af42CekgTIbK5ZiHf7iwo0n+NcOds7QeGvxs9CaVrqBvHBctUHrrNeO+kb6f0gaR+c5tp7CyThL3lD4BK2HJWJUTIpusMMkO59uSLhEBTFD5DknMpmYwBq+hAjDbWAMdxmAF5wx6yMc5jPAjeayWxsc8Hw6QYk9x5/j8eKZ2ZiUuMzP6/Be9vKvNp426hx6TceWd1NBrC45+RiWvwPoM8Bum2qYMn/88O/Sz0zP3Y6jjx/bPmR1ONHc9woEVIxhAsMav2MUU6EbD4lyg0y4V6XlRhB3C0C3jpTCkOJrB/1NDEjzz6Y60UZSpRIrpn/xRfhHLzOB+Z2lMcYPS/ha2ym5SE9OjXscSA7m8jw== X-Bogosity: Unsure, tests=bogofilter, spamicity=0.494534, 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, Feb 3, 2025 at 4:44=E2=80=AFPM Liam R. Howlett wrote: > > * Alice Ryhl [250203 07:15]: > > Hi Alice, > > These comments are probably for my education. I'm holding back my > training on naming and trying to reconcile the snake-case and upper case > camel of the rust language. > > > This adds a type called VmAreaRef which is used when referencing a vma > > How did you land on VmAreaRef? Why not VmaRef? We mostly use vma for > vmas today, not vm_area. Is it because the header declares it > vm_area_struct? I'd be happier with VmaRef, but I'm guessing this is a > direct translation from vm_area_struct? > > > that you have read access to. Here, read access means that you hold > > either the mmap read lock or the vma read lock (or stronger). > > I have questions.. they are below. > > > > > Additionally, a vma_lookup method is added to the mmap read guard, whic= h > > enables you to obtain a &VmAreaRef in safe Rust code. > > > > This patch only provides a way to lock the mmap read lock, but a > > follow-up patch also provides a way to just lock the vma read lock. > > > > Acked-by: Lorenzo Stoakes > > Reviewed-by: Jann Horn > > Reviewed-by: Andreas Hindborg > > Signed-off-by: Alice Ryhl > > --- > > rust/helpers/mm.c | 6 ++ > > rust/kernel/mm.rs | 21 +++++ > > rust/kernel/mm/virt.rs | 215 +++++++++++++++++++++++++++++++++++++++++= ++++++++ > > 3 files changed, 242 insertions(+) > > > > diff --git a/rust/helpers/mm.c b/rust/helpers/mm.c > > index 7201747a5d31..7b72eb065a3e 100644 > > --- a/rust/helpers/mm.c > > +++ b/rust/helpers/mm.c > > @@ -37,3 +37,9 @@ void rust_helper_mmap_read_unlock(struct mm_struct *m= m) > > { > > mmap_read_unlock(mm); > > } > > + > > +struct vm_area_struct *rust_helper_vma_lookup(struct mm_struct *mm, > > + unsigned long addr) > > +{ > > + return vma_lookup(mm, addr); > > +} > > diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs > > index 2fb5f440af60..bd6ff40f106f 100644 > > --- a/rust/kernel/mm.rs > > +++ b/rust/kernel/mm.rs > > @@ -17,6 +17,8 @@ > > }; > > use core::{ops::Deref, ptr::NonNull}; > > > > +pub mod virt; > > + > > /// A wrapper for the kernel's `struct mm_struct`. > > /// > > /// This represents the address space of a userspace process, so each = process has one `Mm` > > @@ -200,6 +202,25 @@ pub struct MmapReadGuard<'a> { > > _nts: NotThreadSafe, > > } > > > > +impl<'a> MmapReadGuard<'a> { > > + /// Look up a vma at the given address. > > + #[inline] > > + pub fn vma_lookup(&self, vma_addr: usize) -> Option<&virt::VmAreaR= ef> { > > + // SAFETY: We hold a reference to the mm, so the pointer must = be valid. Any value is okay > > + // for `vma_addr`. > > Is this true? In C we hold a reference to the mm and the vma can still > go away. We get safety from the locking on the C side. Notice that this function is in the `impl MmapReadGuard` block. This implies that you *must* hold the mmap read guard to call this function. The safety comment should probably be updated to mention that we hold the g= uard. > > + let vma =3D unsafe { bindings::vma_lookup(self.mm.as_raw(), vm= a_addr) }; > > + > > + if vma.is_null() { > > + None > > + } else { > > + // SAFETY: We just checked that a vma was found, so the po= inter is valid. Furthermore, > > + // the returned area will borrow from this read lock guard= , so it can only be used > > + // while the mmap read lock is still held. > > So We have complicated the locking of the vmas with rcu and per-vma > locking recently. We are now able to look up and use a vma under the > rcu read lock. Does this translate to rust model? > > I believe this is true in recent version of binder as well? Yes. The safety requirements of VmAreaRef is that you must hold the mmap read lock *or* the vma read lock while you have a VmAreaRef reference. This particular method achieves that requirement by holding the mmap read lock. But there is also a Rust lock_vma_under_rcu(), see patch 4 for that. > > +// Methods you can call when holding the mmap or vma read lock (or str= onger). They must be usable > > +// no matter what the vma flags are. > > +impl VmAreaRef { > > + /// Access a virtual memory area given a raw pointer. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure that `vma` is valid for the duration of 'a= , and that the mmap or vma > > + /// read lock (or stronger) is held for at least the duration of '= a. > > What is 'a? > > > + #[inline] > > + pub unsafe fn from_raw<'a>(vma: *const bindings::vm_area_struct) -= > &'a Self { > > This 'a? What can I look up in rust to know what this is about? This is called a lifetime. It represents some region of code, and the idea is that the returned reference is guaranteed valid inside the lifetime 'a. For this particular function, the caller chooses what the lifetime is completely unsafely, so there is no check, but in other cases it's enforced by the compiler. For example, if we take vma_lookup from above, then its signature is short-hand [1] for this: pub fn vma_lookup<'a>(&'a self, vma_addr: usize) -> Option<&'a virt::VmAreaRef> { This signature tells the compiler that the returned pointer to VmAreaRef can only be used in the region of code where `self` is also valid. In this case `self` is an `MmapReadLock` (because that's the impl block the function is defined inside), so the returned VmAreaRef is valid in the same region as the MmapReadLock object. This enforces that the returned pointer can only be used while you still hold the read lock. If your code does not follow that requirement, then that's a compilation error. [1]: https://doc.rust-lang.org/book/ch10-03-lifetime-syntax.html#lifetime-e= lision > > + // SAFETY: The caller ensures that the invariants are satisfie= d for the duration of 'a. > > + unsafe { &*vma.cast() } > > + } > > + > > + /// Returns a raw pointer to this area. > > + #[inline] > > + pub fn as_ptr(&self) -> *mut bindings::vm_area_struct { > > + self.vma.get() > > + } > > + > > + /// Access the underlying `mm_struct`. > > + #[inline] > > + pub fn mm(&self) -> &MmWithUser { > > + // SAFETY: By the type invariants, this `vm_area_struct` is va= lid and we hold the mmap/vma > > + // read lock or stronger. This implies that the underlying mm = has a non-zero value of > > + // `mm_users`. > > Again, I'm not sure this statement still holds as it once did? If it's possible to hold the mmap/vma read lock for a vma that is part of an mm whose mm_users count has dropped to zero, then it is incorrect for us to have this method. Alice