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 8E3BBD6ED0E for ; Thu, 21 Nov 2024 11:47:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id F39A26B009C; Thu, 21 Nov 2024 06:47:49 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id EE92A6B00A7; Thu, 21 Nov 2024 06:47:49 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D62176B00A6; Thu, 21 Nov 2024 06:47:49 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id B899C6B009C for ; Thu, 21 Nov 2024 06:47:49 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 69960140ED9 for ; Thu, 21 Nov 2024 11:47:49 +0000 (UTC) X-FDA: 82809926136.21.7B147C2 Received: from mail-wm1-f51.google.com (mail-wm1-f51.google.com [209.85.128.51]) by imf17.hostedemail.com (Postfix) with ESMTP id 1606F40016 for ; Thu, 21 Nov 2024 11:47:06 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=ZwLdvjpK; spf=pass (imf17.hostedemail.com: domain of aliceryhl@google.com designates 209.85.128.51 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1732189605; 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=s7Jpx3h5F/wBA3xOhCxThURBB82AusRTtrT4gd5tWfo=; b=7m1ypJ6LR7saNajKb7XwxCLNhYO2TnhthQRLuXxTZH9M/jRgjJI7y7AFeQax3GFeJQCaB3 9AnqUFlp33J8SMblvwFtHtMGjAAezFIyZ9vkCiByej+HXGNMuT8V6zlBJLW6Rq8uMDpbPc 04GvNyLAAiXH5rXopADxkjKJ5Tbd61o= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1732189605; a=rsa-sha256; cv=none; b=lfk0Qof1D6WSc2ZLr+0kFc4XUvDBSMAMU45NtSRYSCZpsOSG2g8/6+dhoGx2FqoFo3gWYp NQXTsCEGY/bz0AuMpJJ+ygHa6V9CwqiqeVj7b8iSy+uA1ikRFL0Hxv8qGp9bMEFBd+Vot8 UxJVePkcpMJe4S9xyevNUwkoXiYF5kw= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=ZwLdvjpK; spf=pass (imf17.hostedemail.com: domain of aliceryhl@google.com designates 209.85.128.51 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-wm1-f51.google.com with SMTP id 5b1f17b1804b1-43158625112so6620935e9.3 for ; Thu, 21 Nov 2024 03:47:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1732189666; x=1732794466; 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=s7Jpx3h5F/wBA3xOhCxThURBB82AusRTtrT4gd5tWfo=; b=ZwLdvjpKWBJ+5fhX8LkqQP3lCqiNo2Cyu830aUJdWcizki7Bxepb87OZ1E1v81htol XCCjOb50P2RORTlVo4tBMwrAcGuIvIBcwEfWuxG6OetITSnNxsX/Q0eD0kmsmImIonCu VdSD7fzmu3UOLS9MKJ9undAuiu/ciwECtJVPUR+ggYcZTLPRpT8tpPX2mDIX9ZmzOk1y I8uuGb14lCfrBphQ8AOwpunmaOcA1PbCRnZbr2XTreu67CIJzQCZ9D/RkoXaVwhKmUpz KAWHHraZ6/nQhfjgNn6zhmyWFuTRVJgaArIEwREyzLvkv8bG8iXqFYfiTtfwD8wRKPH1 ljDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732189666; x=1732794466; 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=s7Jpx3h5F/wBA3xOhCxThURBB82AusRTtrT4gd5tWfo=; b=JOYrm4uw0ZUzRCk2VAYOCpTYN3aooE13fa8EI7rp7PTPRvNKVR09bnmMfh16dUr2zD Q43X3c1dVQplkD4DAkzStcqPH6izuG0vD4nN0t8eQUiDrgF2wgJO69juyfwpqotoTLxi 6AbHmffVW59kY7WiuHDp9q3HcZCCqj6KS1ltl7QeHY04AK+FN04A0RgZwbYeIlcXgZIr Efq1+mFmGnGokAUK2jCwojaNDnTrwBeuA1G0C7eZ2kC5YYHjP80MLKwEmfO//55BmQ3s Wd42e+fvA+BtuC2iV4dQw1yfWr7PrHKSLquG9oCPXkLJAeM2hNOeu8IsObzVN/Y7b/kN dZAA== X-Forwarded-Encrypted: i=1; AJvYcCUR0nYKHsGPXaDET+r9lAwCGg5vgsTbdMxhGxnFeVNpjGypQJf91Hh/TPLA4ErsfrE49+vaQxsFLA==@kvack.org X-Gm-Message-State: AOJu0YwaZZ0lcHDvrwWk8mrw6eJdLu3BwIEm1NRW4LsbXoMYOUN+6z/Y WSrFBwyB/42PE2IEWeCJpAwR2TRLNZdOlTAtJCjHqvE7s32WnbFgcnMsbdCs7dYtcVc2HQhSl/w yjl3NYhNcsKPeFVF8oaKLBd/kgaywM60kPlTP X-Gm-Gg: ASbGncv8jzoicOCuOgXRJgg+S3AdNGpJO3mnYBapX+x2INdSS7gExK+iLf/eyOOzJPJ KgMlktRp7PDtUmmR7Via6/2U+hrZzxJgMX1axpLQ8TJj4Um7I9vnU5DWSyb1Zvg== X-Google-Smtp-Source: AGHT+IErqV0V+AJ2UHRmioWLhU5OsHABPh0+c+ZumlE3zmUVhcVOjKRjYxRx69HpUK6hUBWpEgTlIcHN/WLV9AwIu0M= X-Received: by 2002:a05:600c:3514:b0:431:47d4:19c7 with SMTP id 5b1f17b1804b1-433489814a4mr60730235e9.3.1732189665908; Thu, 21 Nov 2024 03:47:45 -0800 (PST) MIME-Version: 1.0 References: <20241120-vma-v8-0-eb31425da66b@google.com> <20241120-vma-v8-6-eb31425da66b@google.com> <8755851f-de55-4411-9a8a-80ff69f35905@lucifer.local> In-Reply-To: <8755851f-de55-4411-9a8a-80ff69f35905@lucifer.local> From: Alice Ryhl Date: Thu, 21 Nov 2024 12:47:33 +0100 Message-ID: Subject: Re: [PATCH v8 6/7] mm: rust: add VmAreaNew To: Lorenzo Stoakes Cc: Miguel Ojeda , Matthew Wilcox , Vlastimil Babka , John Hubbard , "Liam R. Howlett" , Andrew Morton , Greg Kroah-Hartman , Arnd Bergmann , Christian Brauner , Alex Gaynor , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , linux-kernel@vger.kernel.org, linux-mm@kvack.org, rust-for-linux@vger.kernel.org, Andreas Hindborg Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: fmxuw11og9syxh3az7dwcnwyw6iuguw4 X-Rspam-User: X-Rspamd-Queue-Id: 1606F40016 X-Rspamd-Server: rspam02 X-HE-Tag: 1732189626-71474 X-HE-Meta: U2FsdGVkX19P8gigUDt706qoguv4RuIf0v3RxjNgpUUUKK5NndnLkI0btLLCrAtn3Y0OVKHEMt6FypbEc1DEEtIxxzUtz/RwKfNaNGLfQ6prdsV6gSNav2N4jFNN0mMEal2UVIZT5P1N1Fu7Mua1V3MbMsKb5jMwPZi1+Tg6Imyzce8cgHIx3ObgyUxoDzNN4u1XSc+2T97aQWLJFCvq6RhGwrcDc0w83+8MSu7IHW9VVMyqkwIWwwshbP0QHjrna9lSxR/atRYGcYOsNXEiJVaTo3RsTfrHykH8OrvglPuKvUcU7bxVXipafQF+5QVOD8ANQOqu5WRE4BOPMXKVjVytkss86VyLpJZl6tCPIbYMvS0wZog0roSGnKeBVAY/1d/yRCf4lJY9r3P+NisxN9//AJ/svT3XOaEwAhbYz68ZnVIttSWxy3pYsQaL7Lt4ea5KFPicqrCWEhRT5t1qoFr45/R1NTVqFGsrUvV/W2fTUiKMzGR2VB/spwwY+V02Kuo7Yhe6ExPLHKlA5WS9Muo5kAxEQ0Bh1X2PXbA5NOTzUQg6ADbfBB1naqcCwA9k+IucbxKnPv93L+a456mAcI39Z7ugwZm+9sqjZk8tZYjClpF6q8mfyPJKeTRBrnN8KJYRFQXfyZwC1AbLVh6hM27fmLBhRJKD48Mxn21vijbhnpkFmqEJyd1VL9WVTcdAlG8uYcqrwYuZTI5zPeBMNxCx/cMLX9GNsbPG26E9CyJ1fLlGXh0FrJPd0O497O4DK0vZHIJDw2sSaabMmrj5hDKffuTSm3A20zGNgzPo1jDvZpwTYNJaxbwwJszXkzccLY1I0aScXfiSYnWPU1enj58PmTPpQBZKz0gwQYR8ukd+BFPHmKFGmXG1fqJMtDGOW60APPaK7hGW2Qc37lt1veevLBN2NGuYCmBcOvNUc12fax2+t5Cy7/RY5YEyEInasfN9IjqTnaUSOfQx/LW XvTyxOFl gnzoRBruB3xItBnC0sZ0NfjHKSmSREdbgI1dwY86hRSkNT6Yo9kr3Y4DhI/8JYqsimqWmOlrOd8MYfkuvFpMVaHsCqXCyl7CpbiV9Yomps5dZJpltXLbD2hA9w8NwFAUHsK/spxsAA6z7TNoSC8muXvx9KmMVP/uRvwyYdo7JQ2v3g1LWSPxwXwuEGmjiiokPpJCfl5XdJY+HlD+hreApsc4yPjity0jVfgRm7SbfukZ2Fx07lh+feqi5/uiYcK87FRY9wtB4xys7MT2PFjQLB1FnZJWXu0BvIxnf5EdE6eMAq3UayriT8aI4cbnLFVuMnOjF 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 Wed, Nov 20, 2024 at 9:02=E2=80=AFPM Lorenzo Stoakes wrote: > > On Wed, Nov 20, 2024 at 02:50:00PM +0000, Alice Ryhl wrote: > > When setting up a new vma in an mmap call, you have a bunch of extra > > permissions that you normally don't have. This introduces a new > > VmAreaNew type that is used for this case. > > Hm I'm confused by what you mean here. What permissions do you mean? I just mean that there are additional things you can do, e.g. you can set VM_MIXEDMAP. > Is this to abstract a VMA as passed by f_op->mmap()? I think it would be > better to explicitly say this if so. Yeah, the VmAreaNew type is specifically for f_op->mmap(). I'll be more explicit about that. > > To avoid setting invalid flag values, the methods for clearing > > VM_MAYWRITE and similar involve a check of VM_WRITE, and return an erro= r > > if VM_WRITE is set. Trying to use `try_clear_maywrite` without checking > > the return value results in a compilation error because the `Result` > > type is marked #[must_use]. > > This is nice. > > Though note that, it is explicitly not permitted to permit writability fo= r > a VMA that previously had it disallowed, and we explicitly WARN_ON() this > now. Concretely that means a VMA where !(vma->vm_flags & VM_MAYWRITE), yo= u > must not vma->vm_flags |=3D VM_MAYWRITE. Got it. The API here doesn't allow that, but good to know we can't add it in the future. > > For now, there's only a method for VM_MIXEDMAP and not VM_PFNMAP. When > > we add a VM_PFNMAP method, we will need some way to prevent you from > > setting both VM_MIXEDMAP and VM_PFNMAP on the same vma. > > Yes this would be unwise. > > An aside here - really you should _only_ change flags in this hook (perha= ps > of course also initialising vma->vm_private_data state), trying to change > anything _core_ here would be deeply dangerous. > > We are far too permissive with this right now, and it's something we want > to try ideally to limit in the future. The previous version just had a function called "set_flags" that could be used to set any flags you want. Given Jann's feedback, I had restricted it to only allow certain flag changes. > > Signed-off-by: Alice Ryhl > > --- > > rust/kernel/mm/virt.rs | 169 +++++++++++++++++++++++++++++++++++++++++= +++++++- > > 1 file changed, 168 insertions(+), 1 deletion(-) > > > > diff --git a/rust/kernel/mm/virt.rs b/rust/kernel/mm/virt.rs > > index de7f2338810a..22aff8e77854 100644 > > --- a/rust/kernel/mm/virt.rs > > +++ b/rust/kernel/mm/virt.rs > > @@ -6,7 +6,7 @@ > > > > use crate::{ > > bindings, > > - error::{to_result, Result}, > > + error::{code::EINVAL, to_result, Result}, > > page::Page, > > types::Opaque, > > }; > > @@ -148,6 +148,173 @@ pub fn vm_insert_page(&self, address: usize, page= : &Page) -> Result { > > } > > } > > > > +/// A builder for setting up a vma in an `mmap` call. > > Would be better to explicitly reference the struct file_operations->mmap(= ) > hook and to say that we should only be updating flags and vm_private_data > here (though perhaps not worth mentioning _that_ if not explicitly expose= d > by your interface). I guess also vm_ops? > I'm guessing fields are, unless a setter/builder is established, immutabl= e? If you have a mutable reference, you can always modify fields. When you don't want that, fields are made private. > > + /// Internal method for updating the vma flags. > > + /// > > + /// # Safety > > + /// > > + /// This must not be used to set the flags to an invalid value. > > + #[inline] > > + unsafe fn update_flags(&self, set: vm_flags_t, unset: vm_flags_t) = { > > + let mut flags =3D self.flags(); > > + flags |=3D set; > > + flags &=3D !unset; > > + > > + // SAFETY: This is not a data race: the vma is undergoing init= ial setup, so it's not yet > > + // shared. Additionally, `VmAreaNew` is `!Sync`, so it cannot = be used to write in parallel. > > + // The caller promises that this does not set the flags to an = invalid value. > > + unsafe { (*self.as_ptr()).__bindgen_anon_2.vm_flags =3D flags = }; > > Hm not sure if this is correct. We explicitly maintain a union in struct = vm_area_struct as: > > union { > const vm_flags_t vm_flags; > vm_flags_t __private __vm_flags; > }; > > Where vma->vm_flags is const, and then use helpers like vm_flags_init() t= o > set them, which also do things like assert locks (though not in the init > case, of course). > > So erally we should at least be updating __vm_flags here, though I'm not > sure how bindgen treats it? I don't think using vm_flags vs __vm_flags changes anything on the Rust side. The modifications are happening unsafely through a raw pointer to something inside Opaque, so Rust isn't going to care about stuff like your const annotation; it's unsafe either way. I'll update this to use __vm_flags instead. > > + /// Try to clear the `VM_MAYREAD` flag, failing if `VM_READ` is se= t. > > + /// > > + /// This flag indicates whether userspace is allowed to make this = vma readable with > > + /// `mprotect()`. > > + #[inline] > > + pub fn try_clear_mayread(&self) -> Result { > > + if self.get_read() { > > + return Err(EINVAL); > > + } > > This is quite nice! Strong(er) typing for the win, again :>) :) Alice