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 41EE9C48260 for ; Thu, 8 Feb 2024 13:14:21 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C20E86B0071; Thu, 8 Feb 2024 08:14:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BA9B66B0074; Thu, 8 Feb 2024 08:14:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A71E86B0075; Thu, 8 Feb 2024 08:14:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 96C7F6B0071 for ; Thu, 8 Feb 2024 08:14:20 -0500 (EST) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 26703160EFE for ; Thu, 8 Feb 2024 13:14:20 +0000 (UTC) X-FDA: 81768680280.19.562026E Received: from mail-ua1-f53.google.com (mail-ua1-f53.google.com [209.85.222.53]) by imf21.hostedemail.com (Postfix) with ESMTP id EDB211C001A for ; Thu, 8 Feb 2024 13:14:16 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=DtaqpmwA; spf=pass (imf21.hostedemail.com: domain of aliceryhl@google.com designates 209.85.222.53 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707398057; a=rsa-sha256; cv=none; b=0oRAffXtiKFVz5V07+agzXP4snpq+cXwB+uksbVCXaH4FSAVz6RjTlIgrB7e/bmmjjGPHm CQ86b5TueY9h+2pvH/UD1zZFdmHzFuLhlbg+FZFHYTmGI2u6wbR+XSyka8jZnDPGw9v0fO YVYLgmoaBUWYc568hyUGo8RFHs4LlV0= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=DtaqpmwA; spf=pass (imf21.hostedemail.com: domain of aliceryhl@google.com designates 209.85.222.53 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=1707398057; 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=u4Y28H6BgiSHtemuYz9Q6cvGUwcKGIFMyv2Vo+B56G0=; b=ibFDteBeUIi0rzJ4A/t69NMDq4tAXTPIx6UM64dG2IoO3rQ9DhwBTYwZJAIOim1E+VQ/RF 4cjoKoqWhZVc3qioIxA+pB0siB/Zck29q8RXItXYUGgp3IiXFj1tAALiqItu42FWWuIdb1 plDDVq+MlkGJtGEsAV3OXmbM1iEUKw8= Received: by mail-ua1-f53.google.com with SMTP id a1e0cc1a2514c-7d2e007751eso832024241.1 for ; Thu, 08 Feb 2024 05:14:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1707398056; x=1708002856; 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=u4Y28H6BgiSHtemuYz9Q6cvGUwcKGIFMyv2Vo+B56G0=; b=DtaqpmwA7fuVzGANCySLPLkUwANusieOnB7dvtq5hx9Abcw4NeRwSfqP7UsIS/aOKZ OGiXwvlqn/9echKHVL+f4oQ/s2bHYXUp5J/cM4w6/pLlj+RcE8JIeiphtCahpXF/GDsz P3Bwx1xp2OfXTHB9rMfm+Bu+kUS3DNvp/A4GRtOdk4LVbGJqeEmXrxU+ip85vZRdNdA0 B7+RuqtW9rSagfOnOMmsTG27tKbrsaUCJe0izyGY+zl7S1hbdS2b6hx5RHtfhSescx9j 49CP1Xj3yCu3Epk1lVCDzZvduI5+DLpQ31LLZd31BOVnkcYJCwJdYlJRfqvJ601h/8fk WJTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707398056; x=1708002856; 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=u4Y28H6BgiSHtemuYz9Q6cvGUwcKGIFMyv2Vo+B56G0=; b=jiyBrSnE00gNYK06mvX7V7X5xXOMRcZ6w5PEhsbYcX0uFFQ0O+W2eyIIBdg0rJl0Ax NmjzHVQEroPC0wzvyaLAHnEkAAaTke4sl+ezjxHp3nXSwolrNJ8BKDUumMvryPQqAeNv UBdeYfFDHFrDNJau34sYQzcL/4gSoDR2G6Z0PSenFpyOZOlXQKf7UiHBljr+5bnE3i7N n+XxQN0VUNNdK1VkjoeeUHg9S28fRrbfqK23RrSNMS1aGwuY9XnZekAgMo12Q78TZ12w gCPrb1iP2VGw9WXKV3Re6WMLflo++b+ukJNgjwsuZerceaCjgvY2Ow+wPZ05wHWfmz8t EsOw== X-Forwarded-Encrypted: i=1; AJvYcCWe7Jt63Qxyno84rJFuAfmMIovCB2h/Juz36dkKdFcnFhhVmmV66zrBb5Ii8HCmkQ6bTHcLgVXn3ERYgJxm+xj8Bo4= X-Gm-Message-State: AOJu0YypijszIVXRKEwhJ17vKy58+RB1NI3XLwOGBVATcmuLSVN6pYVp khvLG+L9I4Zngc4x2iulr7WMdPSvCfwEhOgE9d86Z2IHZ+IGUBnFVW/VZOAbWszcSxkCvpQjRE/ U253M9H56fUum3hppsFeJBUyIoRlsTGfZJzVk X-Google-Smtp-Source: AGHT+IFAH2AcakGpseEeHbdkDCpG2lxDefL5T1Ou+BT2/kUYqqhr3VPMB7tRMumatFtOHoQQ4hDQ0sn5XhVLJ3wPWPQ= X-Received: by 2002:a05:6102:2a7a:b0:46d:49a5:e9fd with SMTP id hp26-20020a0561022a7a00b0046d49a5e9fdmr5975810vsb.24.1707398055828; Thu, 08 Feb 2024 05:14:15 -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: From: Alice Ryhl Date: Thu, 8 Feb 2024 14:14:04 +0100 Message-ID: Subject: Re: [PATCH 2/3] rust: add typed accessors for userspace pointers To: Trevor Gross 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-Server: rspam08 X-Rspamd-Queue-Id: EDB211C001A X-Stat-Signature: u5891qu8jowhcza5c8xsoyxzqd3urhhq X-Rspam-User: X-HE-Tag: 1707398056-247181 X-HE-Meta: U2FsdGVkX1/BxT6p96nguMCPt1JB6u+2O8lMNz6OfnMB4R6QbPt4d9cwSll2N4weoPRZTXrunCnHlAqmfIJ2egJSLjLIo7aFHQe5QRvf9CY9qqMH8BdHs6LaK5LFl/7YSnw5Iiv/ukgqN51gmPOcK3iWJ3KBU0M50OE7XbAb1+QzEDV86iGXlFEWg3oMu/m1wXyEDjA4b0oo6iZFPHohzS9OrSr5J9KGPgivDHEs/0l6hni9YNRVptLcDMqPWoVPthczVDDS/UoOmWAXUJi40mLj6Qcp+dJHYIsKeYaYDPv1AQMw+Eu2v4xybilbe8sYQsd9woHGS8vRpuDrk8MSkbnemqccOHhk9cCNsRWCe6gDrhl4AuC0qY1/nKnObtu/gd1H3NaoqpGdk8DU9ZH+HwQtyYpCtJ4V9hrBcVdG+xGF64YdThBPzxv3I29kFzUO5Nu8cMYMIHDIc7dqnG3VY80agv0Z8ondesaK3pC4CdEJh0c4ubmXmsonwzZhLP4w1B17R5Vv2RnrK6imQe4cV+3F/SkPAYEi44btnhFFcWr8vbvfsIVbRvS7DvRn6exnK0x2Y7ZdTV817vXL7ooPdAEASbqY1tEx15lYmPQl9+9sQiv+EG9Iic8siY7X0IShF/23K5CbuuvGbkih2ksOP9aPoLJIRrl1ygP9cBpeXhaYlDt/JCgIU+JvBWH3DE/BavEI6TPO68mpZ+aHdELwMGYVa6iWRPH91Ui2xiMU5tPqDnQV0zl9qvG5Bggb3qgWNsLKRglmP//NV/YlukCAiCAlmbWfCRhxsN1EPyeErxbOtPIWkr/YBvUdy6HyM01DwzDVzyLU5I3SxF1wNqLuAOKIUcMRGGJJBq2rNt58/+Gu6dUFUZ4ZjKwLXzPqqNKd10YUDhf/9GWY9Tdhg0Zxk0E6dnqw2uTPEXNNOAUK7y3UCKuuorWQPuAGi0vlHHUHTrk2YdUhfx5CD5pLSn8 P1VoYexA U5RNKdavffPBwx8eleZcrR3KF35rQ1oDNaNEIdFfXBZ0iOlBLkJROEkVvHbXxggq20fUKdI2iCbWwObQ+7Coj5rrbD77Df81d526ayjRVWVFvXe6sGR5UX4m29/E641M+ZkaVYDmkJl+i0y0G3RQMWQDIQ5oQCdZNFruIGDvXDZRujdPLGr+pnbatuQONepwNw192AsG3BNEyLoU2NBSj7IO6HRtb92fIAMH07IKxv1gvBsfx5SJH7mldyztPya5f9VCwc0PxwzMlpzokNkQiQSZVAgxKsx3+THZ79ni8mel5Kh5ztAQlbNctb/ID0CurG21OKmhcPKftG9T7Iqa5uH1H5c+C1HnHWe4U3VTDkQDTi3q8fm3iyhkM6tjkhYFi3w+hXQyYnOmdaOBSOoB+dd5XBA== 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 Thu, Feb 1, 2024 at 6:04=E2=80=AFAM Trevor Gross wro= te: > > On Wed, Jan 24, 2024 at 6:21=E2=80=AFAM Alice Ryhl = wrote: > > + /// 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 . We're not moving a value of type `T`. We're creating a new value of type `T` from a byte array. The trait says that we can do that, so I think that is enough here. Besides, we'll often want to use this for wrappers around bindgen types. If we add a Copy bound, then we need to get bindgen to generate a #[derive(Copy)] for them, which I don't think it does right now. > > + // SAFETY: The local variable `out` is valid for writing `size= _of::()` 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. Done. > > + // Since this is not a pointer to a valid object in our progra= m, > > + // 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. Sure, I can use those methods. > > + /// 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. I also disagree here. We're not moving a value of type T. We're creating a byte array from a value of type T, and the trait says that we can do that. > > +/// Specifies that a type is safely readable from bytes. > > +/// > > +/// Not all types are valid for all values. For example, a `bool` must= be either > > +/// zero or one, so reading arbitrary bytes into something that contai= ns a > > +/// `bool` is not okay. > > +/// > > +/// It's okay for the type to have padding, as initializing those byte= s 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= an array, > > +// then all bit patterns are also acceptable for arrays of that type. > > +unsafe impl ReadableFromBytes for [T] {} > > +unsafe impl ReadableFromBytes fo= r [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= -for-byte > > +/// to userspace. This means that it should not have any padding, as p= adding > > +/// bytes are uninitialized. Reading uninitialized memory is not just = undefined > > +/// behavior, it may even lead to leaking sensitive information on the= stack to > > +/// userspace. > > +/// > > +/// The struct should also not hold kernel pointers, as kernel pointer= addresses > > +/// are also considered sensitive. However, leaking kernel pointers is= not > > +/// considered undefined behavior by Rust, so this is a correctness re= quirement, > > +/// 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 port= ions. > > +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 port= ions, then > > +// the the array itself does not have any uninitialized portions eithe= r. > > +unsafe impl WritableToBytes for [T] {} > > +unsafe impl WritableToBytes for [T= ; N] {} > > These traits are probably usable in a lot of other places (e.g. > packets, GPU), so could you put them in a separate module? I can move them to the types 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. Vendoring bytemuck is out of scope for this patchset. If we *are* going to vendor one of them, I would suggest zerocopy over byte= muck. > 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). I can rename the two traits, but I'm not going to introduce Pod. It's over engineered for my purposes. Also, I prefer the trait names from zerocopy. They emphasize that it's really about conversions to/from byte arrays, and not about moving values around. Note that WritableToBytes has an extra comment about pointer addresses that bytemuck/zerocopy doesn't have. > 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/