Message ID | 20230719141918.543938-10-benno.lossin@proton.me |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c923:0:b0:3e4:2afc:c1 with SMTP id j3csp2483213vqt; Wed, 19 Jul 2023 07:36:15 -0700 (PDT) X-Google-Smtp-Source: APBJJlHj5AEeLisrH92du/3qLSbHB7outZXs8U55wWVfe/mW9nXPyolmQYRQ5+pkiBGDmLeY8ptY X-Received: by 2002:a19:4f01:0:b0:4fb:78b1:1cd4 with SMTP id d1-20020a194f01000000b004fb78b11cd4mr27213lfb.49.1689777375588; Wed, 19 Jul 2023 07:36:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689777375; cv=none; d=google.com; s=arc-20160816; b=qNk/sKUeBcc5R9wmdGUYP7e4B9gPyGhXzPRqIZ88V+LN/Ux9DUD4YKRBNxEf8xvcdE QMmP8whpxnyZ9fYlinPmz8hXUtYOjNuUF7rM1zX1GHcLz/GQJllXkjBeBk/feY1jbXt5 eTBW9ihfltvFeks8mXETLEdGxPV1DbH6qlD9Ep0Af4u31YzlkUxcWCtidNpddHSQSywp i1M0pibpJ5VHaNoQb0DwaIcXFZbXiVazj7kkwIoaVHYZGRgFH7qcRUf0B6CcbCO2iD+a 4digF81p2IE1S3i+YnEFJMKFQ4Lp8smGpjbXGoN3pQrJL9uyi2p5N8sWNxMte0nGk1I1 spkA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :feedback-id:references:in-reply-to:message-id:subject:cc:from:to :dkim-signature:date; bh=ycSUa09BWmPYG+p0T3v8MXq1UGIxA3oXkswxIya2goA=; fh=qm1bOEYzVYZKL4fg86TgWb8D+QJA9mGpUhKFp2A6Xio=; b=Ill2Ed5RJfMQLkuz2eygGuZ3/KdBmEJfghjGxz/STGSsZLF6gBSS6ldarI0HJ99Woi 8Dct4wA6so250QJh3epWWyopJIspnNOGkQNbODw0Zkxl6v+9WQaJ4WmZNnsDLYPxQUI9 vso+1LmiMAvdwe+aQrHsTxo4U1Rcjq2RLVM+Hnw1rBQf0E+Iii6pDIxfRQuV4GnJS6t7 Xb0ghZmdIFI0vrPXQJ7z/9C4Er0mSM/h1Wwujp23UkpGHqWVCK3PWP4Iq5Oyrq8JfDjS 7Xckm45L13voQsrKXJX7IQ8jXEFrss7jbQTxRf19vk/zgh1oUQSd4+dyoo+huLXixmXb Uwhw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@proton.me header.s=protonmail header.b=Hp0gqBpu; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=proton.me Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id p9-20020a056402074900b005218c27b193si3018838edy.501.2023.07.19.07.35.49; Wed, 19 Jul 2023 07:36:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@proton.me header.s=protonmail header.b=Hp0gqBpu; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=proton.me Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230268AbjGSOVx (ORCPT <rfc822;assdfgzxcv4@gmail.com> + 99 others); Wed, 19 Jul 2023 10:21:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51850 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231536AbjGSOVs (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 19 Jul 2023 10:21:48 -0400 Received: from mail-4316.protonmail.ch (mail-4316.protonmail.ch [185.70.43.16]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2CEF519A3; Wed, 19 Jul 2023 07:21:22 -0700 (PDT) Date: Wed, 19 Jul 2023 14:21:06 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1689776469; x=1690035669; bh=ycSUa09BWmPYG+p0T3v8MXq1UGIxA3oXkswxIya2goA=; 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=Hp0gqBpu5Kg0lTyFWlOUxr4CFlT3YRUF3s6qV9d9wWK/qCU6Myt8GwZT33ypBSzwr ObLAaGrCb5pyVC4sZP4dDzVQ2xDgRuV8aoiFTfnNYJtdyTpUxUlK092JCDkZaprUCQ SfkSLTY8juIJkd6chETk2t54ejpvmenxydWJKtB+8DWFcUBG7MS1eZ2YV2UzcoehUJ Iwu2LD9ZycYtUouWT6KEvAhhdB8SHfjRQmVJa4rw3qmThKvHfBk4P8zPdPmF/58OJ4 eDQ+7Fv5minHxoZPSKEOohDsQ/sncyXOewAsZ1xjPXw86/64mGl/Fobzesg8qiWvSq ++0ATtS+WotfQ== To: Miguel Ojeda <ojeda@kernel.org>, Wedson Almeida Filho <wedsonaf@gmail.com>, Alex Gaynor <alex.gaynor@gmail.com> From: Benno Lossin <benno.lossin@proton.me> Cc: Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, Alice Ryhl <aliceryhl@google.com>, Andreas Hindborg <nmi@metaspace.dk>, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2 09/12] rust: init: implement Zeroable for Opaque<T> Message-ID: <20230719141918.543938-10-benno.lossin@proton.me> In-Reply-To: <20230719141918.543938-1-benno.lossin@proton.me> References: <20230719141918.543938-1-benno.lossin@proton.me> Feedback-ID: 71780778:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771860001218102531 X-GMAIL-MSGID: 1771860001218102531 |
Series |
Quality of life improvements for pin-init
|
|
Commit Message
Benno Lossin
July 19, 2023, 2:21 p.m. UTC
Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid
bit pattern for that type.
Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
rust/kernel/types.rs | 2 ++
1 file changed, 2 insertions(+)
Comments
Benno Lossin <benno.lossin@proton.me> writes: > Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid > bit pattern for that type. > > Signed-off-by: Benno Lossin <benno.lossin@proton.me> > --- > /// > /// This is meant to be used with FFI objects that are never interpreted by Rust code. > #[repr(transparent)] > +#[derive(Zeroable)] > pub struct Opaque<T> { > value: UnsafeCell<MaybeUninit<T>>, > _pin: PhantomPinned, > } Does this actually work? I don't think we implement Zeroable for UnsafeCell. I suggest you instead add Opaque to the `impl_zeroable!` macro in `rust/kernel/init.rs`. Alice
On 7/19/23 11:21, Benno Lossin wrote: > Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid > bit pattern for that type. > > Signed-off-by: Benno Lossin <benno.lossin@proton.me> > --- > [...] Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
On 20.07.23 15:34, Alice Ryhl wrote: > Benno Lossin <benno.lossin@proton.me> writes: >> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid >> bit pattern for that type. >> >> Signed-off-by: Benno Lossin <benno.lossin@proton.me> >> --- >> /// >> /// This is meant to be used with FFI objects that are never interpreted by Rust code. >> #[repr(transparent)] >> +#[derive(Zeroable)] >> pub struct Opaque<T> { >> value: UnsafeCell<MaybeUninit<T>>, >> _pin: PhantomPinned, >> } > > Does this actually work? I don't think we implement Zeroable for > UnsafeCell. Good catch, this does compile, but only because the current implementation of the derive expands to (modulo correct paths): ``` impl<T> Zeroable for Opaque<T> where UnsafeCell<MaybeUninit<T>>: Zeroable, PhantomPinned: Zeroable, {} ``` This implementation is of course useless, since `UnsafeCell` is never `Zeroable` at the moment. We could of course implement that and then this should work, but the question is if this is actually the desired output in general. I thought before that this would be a good idea, but I forgot that if the bounds are never satisfied it would silently compile. Do you think that we should have this expanded output instead? ``` impl<T: Zeroable> Zeroable for Foo<T> {} const _: () = { fn assert_zeroable<T: Zeroable>() {} fn ensure_zeroable<T: Zeroable>() { assert_zeroable::<Field1>(); assert_zeroable::<Field2>(); } }; ``` If the input was ``` #[derive(Zeroable)] struct Foo<T> { field1: Field1, field2: Field2, } ``` > I suggest you instead add Opaque to the `impl_zeroable!` macro in > `rust/kernel/init.rs`. We would have to do this when using the alternative approach from above, since we do not want the `Zeroable` bound on `T` for `Opaque`. I wanted to avoid the `SAFETY` comment, since that is needed for the `impl_zeroable` macro (as it has an unsafe block inside).
Benno Lossin <benno.lossin@proton.me> writes: > On 20.07.23 15:34, Alice Ryhl wrote: >> Benno Lossin <benno.lossin@proton.me> writes: >>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid >>> bit pattern for that type. >>> >>> Signed-off-by: Benno Lossin <benno.lossin@proton.me> >>> --- >>> /// >>> /// This is meant to be used with FFI objects that are never interpreted by Rust code. >>> #[repr(transparent)] >>> +#[derive(Zeroable)] >>> pub struct Opaque<T> { >>> value: UnsafeCell<MaybeUninit<T>>, >>> _pin: PhantomPinned, >>> } >> >> Does this actually work? I don't think we implement Zeroable for >> UnsafeCell. > > Good catch, this does compile, but only because the current > implementation of the derive expands to (modulo correct paths): > ``` > impl<T> Zeroable for Opaque<T> > where > UnsafeCell<MaybeUninit<T>>: Zeroable, > PhantomPinned: Zeroable, > {} > ``` > This implementation is of course useless, since `UnsafeCell` is never > `Zeroable` at the moment. We could of course implement that and then this > should work, but the question is if this is actually the desired output in > general. I thought before that this would be a good idea, but I forgot that > if the bounds are never satisfied it would silently compile. > > Do you think that we should have this expanded output instead? > ``` > impl<T: Zeroable> Zeroable for Foo<T> {} > const _: () = { > fn assert_zeroable<T: Zeroable>() {} > fn ensure_zeroable<T: Zeroable>() { > assert_zeroable::<Field1>(); > assert_zeroable::<Field2>(); > } > }; > ``` > If the input was > ``` > #[derive(Zeroable)] > struct Foo<T> { > field1: Field1, > field2: Field2, > } > ``` Yeah. The way that these macros usually expand is by adding `where T: Zeroable` to the impl for each generic parameter, and failing compilation if that is not enough to ensure that all of the fields are `Zeroable`. You might want to consider this expansion instead: ``` impl<T: Zeroable> Zeroable for Foo<T> {} const _: () = { fn assert_zeroable<T: Zeroable>(arg: &T) {} fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) { assert_zeroable(&arg.field1); assert_zeroable(&arg.field2); } }; ``` >> I suggest you instead add Opaque to the `impl_zeroable!` macro in >> `rust/kernel/init.rs`. > > We would have to do this when using the alternative approach from > above, since we do not want the `Zeroable` bound on `T` for `Opaque`. > I wanted to avoid the `SAFETY` comment, since that is needed for the > `impl_zeroable` macro (as it has an unsafe block inside). Indeed, if we expand the derive macro in the standard way, then it doesn't work for `Opaque` due to the extra unnecessary bound. Alice
On 25.07.23 13:57, Alice Ryhl wrote: > Benno Lossin <benno.lossin@proton.me> writes: >> On 20.07.23 15:34, Alice Ryhl wrote: >>> Benno Lossin <benno.lossin@proton.me> writes: >>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid >>>> bit pattern for that type. >>>> >>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me> >>>> --- >>>> /// >>>> /// This is meant to be used with FFI objects that are never interpreted by Rust code. >>>> #[repr(transparent)] >>>> +#[derive(Zeroable)] >>>> pub struct Opaque<T> { >>>> value: UnsafeCell<MaybeUninit<T>>, >>>> _pin: PhantomPinned, >>>> } >>> >>> Does this actually work? I don't think we implement Zeroable for >>> UnsafeCell. >> >> Good catch, this does compile, but only because the current >> implementation of the derive expands to (modulo correct paths): >> ``` >> impl<T> Zeroable for Opaque<T> >> where >> UnsafeCell<MaybeUninit<T>>: Zeroable, >> PhantomPinned: Zeroable, >> {} >> ``` >> This implementation is of course useless, since `UnsafeCell` is never >> `Zeroable` at the moment. We could of course implement that and then this >> should work, but the question is if this is actually the desired output in >> general. I thought before that this would be a good idea, but I forgot that >> if the bounds are never satisfied it would silently compile. >> >> Do you think that we should have this expanded output instead? >> ``` >> impl<T: Zeroable> Zeroable for Foo<T> {} >> const _: () = { >> fn assert_zeroable<T: Zeroable>() {} >> fn ensure_zeroable<T: Zeroable>() { >> assert_zeroable::<Field1>(); >> assert_zeroable::<Field2>(); >> } >> }; >> ``` >> If the input was >> ``` >> #[derive(Zeroable)] >> struct Foo<T> { >> field1: Field1, >> field2: Field2, >> } >> ``` > > Yeah. The way that these macros usually expand is by adding `where T: > Zeroable` to the impl for each generic parameter, and failing > compilation if that is not enough to ensure that all of the fields are > `Zeroable`. > > You might want to consider this expansion instead: > ``` > impl<T: Zeroable> Zeroable for Foo<T> {} > const _: () = { > fn assert_zeroable<T: Zeroable>(arg: &T) {} > fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) { > assert_zeroable(&arg.field1); > assert_zeroable(&arg.field2); > } > }; > ``` Is there a specific reason you think that I should us references here instead of the expansion from above (where I just use the types and not the fields themselves)?
I suggested it because it seemed less fragile. That said, after seeing what #[derive(Eq)] expands to, I'm not so sure. I'd probably investigate why the Eq macro has to expand to what it does. On 7/29/23 06:11, Benno Lossin wrote: > On 25.07.23 13:57, Alice Ryhl wrote: >> Benno Lossin <benno.lossin@proton.me> writes: >>> On 20.07.23 15:34, Alice Ryhl wrote: >>>> Benno Lossin <benno.lossin@proton.me> writes: >>>>> Since `Opaque<T>` contains a `MaybeUninit<T>`, all bytes zero is a valid >>>>> bit pattern for that type. >>>>> >>>>> Signed-off-by: Benno Lossin <benno.lossin@proton.me> >>>>> --- >>>>> /// >>>>> /// This is meant to be used with FFI objects that are never interpreted by Rust code. >>>>> #[repr(transparent)] >>>>> +#[derive(Zeroable)] >>>>> pub struct Opaque<T> { >>>>> value: UnsafeCell<MaybeUninit<T>>, >>>>> _pin: PhantomPinned, >>>>> } >>>> >>>> Does this actually work? I don't think we implement Zeroable for >>>> UnsafeCell. >>> >>> Good catch, this does compile, but only because the current >>> implementation of the derive expands to (modulo correct paths): >>> ``` >>> impl<T> Zeroable for Opaque<T> >>> where >>> UnsafeCell<MaybeUninit<T>>: Zeroable, >>> PhantomPinned: Zeroable, >>> {} >>> ``` >>> This implementation is of course useless, since `UnsafeCell` is never >>> `Zeroable` at the moment. We could of course implement that and then this >>> should work, but the question is if this is actually the desired output in >>> general. I thought before that this would be a good idea, but I forgot that >>> if the bounds are never satisfied it would silently compile. >>> >>> Do you think that we should have this expanded output instead? >>> ``` >>> impl<T: Zeroable> Zeroable for Foo<T> {} >>> const _: () = { >>> fn assert_zeroable<T: Zeroable>() {} >>> fn ensure_zeroable<T: Zeroable>() { >>> assert_zeroable::<Field1>(); >>> assert_zeroable::<Field2>(); >>> } >>> }; >>> ``` >>> If the input was >>> ``` >>> #[derive(Zeroable)] >>> struct Foo<T> { >>> field1: Field1, >>> field2: Field2, >>> } >>> ``` >> >> Yeah. The way that these macros usually expand is by adding `where T: >> Zeroable` to the impl for each generic parameter, and failing >> compilation if that is not enough to ensure that all of the fields are >> `Zeroable`. >> >> You might want to consider this expansion instead: >> ``` >> impl<T: Zeroable> Zeroable for Foo<T> {} >> const _: () = { >> fn assert_zeroable<T: Zeroable>(arg: &T) {} >> fn ensure_zeroable<T: Zeroable>(arg: &Foo<T>) { >> assert_zeroable(&arg.field1); >> assert_zeroable(&arg.field2); >> } >> }; >> ``` > > Is there a specific reason you think that I should us references here > instead of the expansion from above (where I just use the types and > not the fields themselves)? >
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs index d849e1979ac7..f0f540578d0f 100644 --- a/rust/kernel/types.rs +++ b/rust/kernel/types.rs @@ -11,6 +11,7 @@ ops::{Deref, DerefMut}, ptr::NonNull, }; +use macros::Zeroable; /// Used to transfer ownership to and from foreign (non-Rust) languages. /// @@ -251,6 +252,7 @@ fn drop(&mut self) { /// /// This is meant to be used with FFI objects that are never interpreted by Rust code. #[repr(transparent)] +#[derive(Zeroable)] pub struct Opaque<T> { value: UnsafeCell<MaybeUninit<T>>, _pin: PhantomPinned,