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 E1321C47DDB for ; Thu, 1 Feb 2024 05:04:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5DC9D6B0074; Thu, 1 Feb 2024 00:04:08 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 58C4E6B007B; Thu, 1 Feb 2024 00:04:08 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 47B116B007D; Thu, 1 Feb 2024 00:04:08 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 376996B0074 for ; Thu, 1 Feb 2024 00:04:08 -0500 (EST) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C3EDFC0A33 for ; Thu, 1 Feb 2024 05:04:07 +0000 (UTC) X-FDA: 81742043334.20.7E28E38 Received: from mail-yw1-f169.google.com (mail-yw1-f169.google.com [209.85.128.169]) by imf18.hostedemail.com (Postfix) with ESMTP id BF73D1C0017 for ; Thu, 1 Feb 2024 05:04:05 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=umich.edu header.s=google-2016-06-03 header.b=BHM1duYv; spf=pass (imf18.hostedemail.com: domain of tmgross@umich.edu designates 209.85.128.169 as permitted sender) smtp.mailfrom=tmgross@umich.edu; dmarc=pass (policy=none) header.from=umich.edu ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706763846; 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=fIVlKOR+0dK11siwWLke6mXGwWm4TuJ6szHx+WxrPIU=; b=1XwoVmKRyTU/8tg61IlkfyKkyXiGHz2IZp1Ssw8GG1KENQF5ldGGLxSVgc2gAgY+jK+nnK 0J4r6mOU9Ipzc+cisq4L7e/wGPXmnZreA7tF7vfoIuadjuEXm4QlvEXrEzGY0dsjfezGMX HrXaYAWCGzRIU5Lx96ezFLcJLTxT4sw= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706763846; a=rsa-sha256; cv=none; b=6Fz+qqxWV1GRUdcsWodrAa9//0KEgCgPSY9auNB0POfJJMUrsXkiCU/O51s/Z42UjVhqri Wf1cEWkb/SLyDvl227aXZYEZgm/zi40SRbidLwRHST0St67Qx9V3mV+ilrskl8Nw6D80fY W2AB99j8ohW1CzebJWIGC3UdWbwAiK0= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=umich.edu header.s=google-2016-06-03 header.b=BHM1duYv; spf=pass (imf18.hostedemail.com: domain of tmgross@umich.edu designates 209.85.128.169 as permitted sender) smtp.mailfrom=tmgross@umich.edu; dmarc=pass (policy=none) header.from=umich.edu Received: by mail-yw1-f169.google.com with SMTP id 00721157ae682-60417488f07so5891917b3.1 for ; Wed, 31 Jan 2024 21:04:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=umich.edu; s=google-2016-06-03; t=1706763845; x=1707368645; 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=fIVlKOR+0dK11siwWLke6mXGwWm4TuJ6szHx+WxrPIU=; b=BHM1duYvt8zlTEBCkym4VHC31aHeagkdncbHRlJQsqUuoLQWxAA6CDDmKYnabtrWvM LeEZ2BZbyNN/nk0ZMoqgq/7benfevfjI2nZfFgc+xZ9my5XEi/3Chp+iHE8Iz7USxgzt tjOJVzGBTqLcEUIwyaQrtrXYb6e2CxK7mlb9r5AMJVKiH8i2C8UVJylrsu4OHEQQFEmI nCta5R2w/rHMNNzmgUSYdiF6PgxjJ4q0k2m38/3Ae4F8fy3TiIJEzrVNbtf+rIbS5nGC 3cG6SDZEH/olFlpnOIDWYp8X2dhPUDYjA6fpy8rslT8xfWkk5eDvFU5dx3MmOkXwUKwg ymHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706763845; x=1707368645; 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=fIVlKOR+0dK11siwWLke6mXGwWm4TuJ6szHx+WxrPIU=; b=FTMRyKRzwY1WAnEXU+KJqTjMxUbTHcS1X4LeXMcqQxbyq5HlqpTXVhAccaEAPSDAuG eIDvCeXjSS+f1fG0cyTHuq5e3PAcLiTYqKmhLyKqEY6e3pkar1yDjDlrCByp6cjGu3kB UUC+8CSZn1VjcQMnozVCHEYa9d3XEtHUbwVs3D1GxQo2JYE/lK5HtY6JZ1LA01KwI8mU BmHVrpXkVW+zecsWA4pVIjfZBVCpCKjHxSYb09yxSnbOtyDLIfd5qg7BVlcx/XHAjtlc LoyOlZBS1QH0zL1ND7/nDRJuU857Vt1f0yDqEYbjQyEcwo3OQqFiKbpvj70Egl9/D97v 12xA== X-Gm-Message-State: AOJu0Yx1dclcrnT8kXh7cu0h1h9bjPjoEZ3bE1QHI4K+ZtZz/A9QlVJ7 6JtgLm0H8To1ovipieGYnQ6am93gRn/3NWw7+xzgYZ+qa9S4JHbIQQ0+yqz6LKrzRautj27ffxX TI7nlTW+54VFK1nz57CcICoYb9ORfRr1wlog5yg== X-Google-Smtp-Source: AGHT+IFBz1YTkrf/qhzCgZIqKfossW9Ek9i6uJcsPLXTqKzNstEFU4dmZO6iv8Nqu4nRoYfb+44TKDCWKyYKEI3eeIo= X-Received: by 2002:a05:690c:82d:b0:5f0:e95f:5ad8 with SMTP id by13-20020a05690c082d00b005f0e95f5ad8mr1327057ywb.19.1706763844706; Wed, 31 Jan 2024 21:04:04 -0800 (PST) MIME-Version: 1.0 References: <20240124-alice-mm-v1-0-d1abcec83c44@google.com> <20240124-alice-mm-v1-2-d1abcec83c44@google.com> In-Reply-To: <20240124-alice-mm-v1-2-d1abcec83c44@google.com> From: Trevor Gross Date: Thu, 1 Feb 2024 00:03:53 -0500 Message-ID: Subject: Re: [PATCH 2/3] rust: add typed accessors for userspace pointers To: Alice Ryhl Cc: Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Kees Cook , Al Viro , Andrew Morton , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: BF73D1C0017 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: puqwp8jerumoeomytzt48b5m5tg1npez X-HE-Tag: 1706763845-879124 X-HE-Meta: U2FsdGVkX1+7Nz3gXhfaylV+Y7p3K5XALBHu0/YYbEhQ9jTp+zFVqeoXW57kiT//ijGYotZWf7As4mwUxwcXfXEuycQ9Yi/szbTZ6tKsyopiYXgCbYBQ51D9XWjc/DX7f/yG6dmvLTsu1PsA8i4KzFrXYlUG60Uqv6ta1UxpZYAG8/5A0VoOv9bYBd6m2bC/GZ5vXxAD/L8O6mHnxf3GGFoKyh6pEz/aHlbwDR5LXEKYkpP8oklCXYxwLMMlgwK/Js8pNr7zfOWDYjURY7kjYXdjKykO4hSAcJGgApY/jPHXWHrqRjKkbFiMm11LXx9MC/8tToG4edLFJw7JVJZY3VJwPajDZagRObbKD22Fm/7FVczy68j9ZhfvVj8zTOTgreVbcmZCbag6gqf5FBRDIOVbuPIVLb4DT1JiJAMxHzIRWhePWBl9p7aCquvPvetIQIZzwRh7xYes57Dtmd9DJp2pRSHgunGXBoCfQg2Sk4ZzNvSHmRXDHt8enxVdsCGwcRFzdfY76UDkeqyDtdo/wh7Q5pbHTC/8SjvwibdpyOfr3ddipxogYRUmOuOc0b3yN03SYD/HCn8Sfg+EwWF0qXAQnDbbxVVXLaKc5IeM8mzg2WAJF15z9EZvJFnBng2UDLh2l/ow78fpbpaqWDSJucYiMaioYyW8WiCRsPbRIZEUuvGUXNjXWvuR8bWg6GX1ZnOxIKfce2DBStRSdCXzj0j6XBcIxECXJBRGgx/sBnJuAyxGbpznSuWV2q4pNwYfnViTJTVzs8s21BvmjUqycNcjqfrbt5djW5vSp9MgQwTrLaEOk8s3nfRADCkH4QIG/vpVIJcb/oCOob80M9CtQafsF7eCCihGY3W8kaYXhLtx+Xo8lXTt7TOKq+FRMpqtpfMOD+WOBuq/yu6obVJNtUpwJ7s8brppHUOhewsCQqPE3bxUQAkAi/XFG8CEfxYK8CJos89jsW4hAUIebb/ bZmfaYvH DjQSeYPweRnwPNpGFIAA8JqtPg5q80Wcutzc79yCgNZW6zCdII6YMfpTjrPuBywToG8c/y63+nKxRVSFYxPqGZSY4aTfLfLkqtKp/CzjCVvUqKsFxxZuNuaV4SC4B7ImUtOvu4cei3jQIzgYgNznmXmm2MPaIMSkLVjzX2Axszg0bl24mhRvuwbxKEQbAbjliKmZwDyNZax+ADOuySkAyEmw+hVau4MxNaTKwQf09j6DyOlPcQGzbaxB3S5Mc8MLxWUyx/ixL+FxRZlM4NB7B2AT0u4mLCnTqhpXavEFbW8iOlNa6jQC2zpgBFVwBmgDjWte3wAUF0OVXQw3dopRhM2akq7tFSyKkQJsh7bIZxHx3k3Ny4bR9gtQwnRgUpKfAfWD7Sw/UUMcLgogh9B3OxHUEFHu+yzs2waaTrPywjvwykbPImsbkNKA46w== 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, Jan 24, 2024 at 6:21=E2=80=AFAM Alice Ryhl w= rote: I see this patch answers some of my naming questions from 1/3, sorry for not reading all the way through. > diff --git a/rust/kernel/user_ptr.rs b/rust/kernel/user_ptr.rs > index 00aa26aa6a83..daa46abe5525 100644 > --- a/rust/kernel/user_ptr.rs > +++ b/rust/kernel/user_ptr.rs > @@ -11,6 +11,7 @@ > use crate::{bindings, error::code::*, error::Result}; > use alloc::vec::Vec; > use core::ffi::{c_ulong, c_void}; > +use core::mem::{size_of, MaybeUninit}; > > /// The maximum length of a operation using `copy_[from|to]_user`. > /// > @@ -151,6 +152,36 @@ pub unsafe fn read_raw(&mut self, out: *mut u8, len:= usize) -> Result { > Ok(()) > } > > + /// Reads a value of the specified type. > + /// > + /// Fails with `EFAULT` if the read encounters a page fault. > + pub fn read(&mut self) -> Result { I think that `T: Copy` is required here, or for Copy to be a supertrait of ReadableBytes, since the data in the buffer is being duplicated from a reference. Send is probably also a reasonable bound to have . > + if size_of::() > self.1 || size_of::() > MAX_USER_OP_LEN { > + return Err(EFAULT); > + } > + let mut out: MaybeUninit =3D MaybeUninit::uninit(); > + // SAFETY: The local variable `out` is valid for writing `size_o= f::()` bytes. > + let res =3D unsafe { > + bindings::copy_from_user_unsafe_skip_check_object_size( > + out.as_mut_ptr().cast::(), > + self.0, > + size_of::() as c_ulong, As with the other patch, I think it would be more clear to use `c_ulong::try_from(...)` rather than comparing against `MAX_USER_OP_LEN ` and later casting. Possibly just in a helper function. > + ) > + }; > + if res !=3D 0 { > + return Err(EFAULT); > + } > + // Since this is not a pointer to a valid object in our program, > + // we cannot use `add`, which has C-style rules for defined > + // behavior. > + self.0 =3D self.0.wrapping_add(size_of::()); There are now methods `wrapping_byte_add` (since 1.75). Doesn't make much of a difference since the pointer is c_void anyway, but it does make the unit more clear. > + self.1 -=3D size_of::(); > + > + // SAFETY: The read above has initialized all bytes in `out`, an= d since > + // `T` implements `ReadableFromBytes`, any bit-pattern is a vali= d value > + // for this type. > + Ok(unsafe { out.assume_init() }) > + } > + > /// Reads all remaining data in the buffer into a vector. > /// > /// Fails with `EFAULT` if the read encounters a page fault. > @@ -219,4 +250,98 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result= { > // `len`, so the pointer is valid for reading `len` bytes. > unsafe { self.write_raw(ptr, len) } > } > + > + /// Writes the provided Rust value to this userspace pointer. > + /// > + /// Fails with `EFAULT` if the write encounters a page fault. > + pub fn write(&mut self, value: &T) -> Result { Send + Copy are also needed here, or supertraits of WritableToBytes. > + if size_of::() > self.1 || size_of::() > MAX_USER_OP_LEN { > + return Err(EFAULT); > + } > + // SAFETY: The reference points to a value of type `T`, so it is= valid > + // for reading `size_of::()` bytes. > + let res =3D unsafe { > + bindings::copy_to_user_unsafe_skip_check_object_size( > + self.0, > + (value as *const T).cast::(), > + size_of::() as c_ulong, > + ) > + }; > + if res !=3D 0 { > + return Err(EFAULT); > + } > + // Since this is not a pointer to a valid object in our program, > + // we cannot use `add`, which has C-style rules for defined > + // behavior. > + self.0 =3D self.0.wrapping_add(size_of::()); > + self.1 -=3D size_of::(); > + Ok(()) > + } > } > + > +/// Specifies that a type is safely readable from bytes. > +/// > +/// Not all types are valid for all values. For example, a `bool` must b= e either > +/// zero or one, so reading arbitrary bytes into something that contains= a > +/// `bool` is not okay. > +/// > +/// It's okay for the type to have padding, as initializing those bytes = has no > +/// effect. > +/// > +/// # Safety > +/// > +/// All bit-patterns must be valid for this type. > +pub unsafe trait ReadableFromBytes {} > + > +// SAFETY: All bit patterns are acceptable values of the types below. > +unsafe impl ReadableFromBytes for u8 {} > +unsafe impl ReadableFromBytes for u16 {} > +unsafe impl ReadableFromBytes for u32 {} > +unsafe impl ReadableFromBytes for u64 {} > +unsafe impl ReadableFromBytes for usize {} > +unsafe impl ReadableFromBytes for i8 {} > +unsafe impl ReadableFromBytes for i16 {} > +unsafe impl ReadableFromBytes for i32 {} > +unsafe impl ReadableFromBytes for i64 {} > +unsafe impl ReadableFromBytes for isize {} > +// SAFETY: If all bit patterns are acceptable for individual values in a= n array, > +// then all bit patterns are also acceptable for arrays of that type. > +unsafe impl ReadableFromBytes for [T] {} > +unsafe impl ReadableFromBytes for = [T; N] {} > + > +/// Specifies that a type is safely writable to bytes. > +/// > +/// If a struct implements this trait, then it is okay to copy it byte-f= or-byte > +/// to userspace. This means that it should not have any padding, as pad= ding > +/// bytes are uninitialized. Reading uninitialized memory is not just un= defined > +/// behavior, it may even lead to leaking sensitive information on the s= tack to > +/// userspace. > +/// > +/// The struct should also not hold kernel pointers, as kernel pointer a= ddresses > +/// are also considered sensitive. However, leaking kernel pointers is n= ot > +/// considered undefined behavior by Rust, so this is a correctness requ= irement, > +/// but not a safety requirement. > +/// > +/// # Safety > +/// > +/// Values of this type may not contain any uninitialized bytes. > +pub unsafe trait WritableToBytes {} > + > +// SAFETY: Instances of the following types have no uninitialized portio= ns. > +unsafe impl WritableToBytes for u8 {} > +unsafe impl WritableToBytes for u16 {} > +unsafe impl WritableToBytes for u32 {} > +unsafe impl WritableToBytes for u64 {} > +unsafe impl WritableToBytes for usize {} > +unsafe impl WritableToBytes for i8 {} > +unsafe impl WritableToBytes for i16 {} > +unsafe impl WritableToBytes for i32 {} > +unsafe impl WritableToBytes for i64 {} > +unsafe impl WritableToBytes for isize {} > +unsafe impl WritableToBytes for bool {} > +unsafe impl WritableToBytes for char {} > +unsafe impl WritableToBytes for str {} > +// SAFETY: If individual values in an array have no uninitialized portio= ns, then > +// the the array itself does not have any uninitialized portions either. > +unsafe impl WritableToBytes for [T] {} > +unsafe impl WritableToBytes for [T; = N] {} > > -- > 2.43.0.429.g432eaa2c6b-goog > > These traits are probably usable in a lot of other places (e.g. packets, GPU), so could you put them in a separate module? The patterns here are pretty similar to what the bytemuck crate does [1]. Since that crate is well established and open licensed, I think it makes sense to keep their naming or possibly even vendor a portion in. In particular, this would likely include the traits: - AnyBitPattern, which is roughly ReadableFromBytes here - NoUninit, which is roughly WritableToBytes here - Optionally Pod (plain old data), a supertrait of both AnyBitPattern and NoUninit just used to simplify trait implementation (impl Pod and you get the other two). And the functions: - from_bytes to turn &[u8] into &T for use in `read`. Needs `T: Copy` to return an owned value, as noted above. - bytes_of to turn &T into &[u8], for use in `write` The derive macros would also be nice to have down the line, though bytemuck's unfortunately relies on syn. - Trevor [1]: https://docs.rs/bytemuck/latest/bytemuck/