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;
References: <20230517203119.3160435-1-aliceryhl@google.com>
 <20230517203119.3160435-3-aliceryhl@google.com>
 <20230523164818.0a44fec8.gary@garyguo.net>
User-agent: mu4e 1.10.3; emacs 28.2.50
From:   Andreas Hindborg <nmi@metaspace.dk>
To:     Gary Guo <gary@garyguo.net>
Cc:     Alice Ryhl <aliceryhl@google.com>, rust-for-linux@vger.kernel.org,
        Miguel Ojeda <ojeda@kernel.org>,
        Wedson Almeida Filho <wedsonaf@gmail.com>,
        Tejun Heo <tj@kernel.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        =?utf-8?Q?Bj=C3=B6rn?= Roy Baron <bjorn3_gh@protonmail.com>,
        Benno Lossin <benno.lossin@proton.me>,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev
Subject: Re: [PATCH v1 2/7] rust: add offset_of! macro
Date:   Tue, 30 May 2023 10:40:03 +0200
In-reply-to: <20230523164818.0a44fec8.gary@garyguo.net>
Message-ID: <87pm6i9pqh.fsf@metaspace.dk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Gary Guo <gary@garyguo.net> writes:

> On Wed, 17 May 2023 20:31:14 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> From: Wedson Almeida Filho <walmeida@microsoft.com>
>>=20
>> This macro is used to compute the offset of a field in a struct.
>>=20
>> This commit enables two unstable features that are necessary for using
>> the macro in a constant. However, this is not a problem as the macro
>> will become available from the Rust standard library soon [1]. The
>> unstable features can be disabled again once that happens.
>>=20
>> The macro in this patch does not support sub-fields. That is, you cannot
>> write `offset_of!(MyStruct, field.sub_field)` to get the offset of
>> `sub_field` with `field`'s type being a struct with a field called
>> `sub_field`. This is because `field` might be a `Box<SubStruct>`, which
>> means that you would be trying to compute the offset to something in an
>> entirely different allocation. There's no easy way to fix the current
>> macro to support subfields, but the version being added to the standard
>> library should support it, so the limitation is temporary and not a big
>> deal.
>>=20
>> Link: https://github.com/rust-lang/rust/issues/106655 [1]
>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>> ---
>>  rust/kernel/lib.rs     | 35 +++++++++++++++++++++++++++++++++++
>>  scripts/Makefile.build |  2 +-
>>  2 files changed, 36 insertions(+), 1 deletion(-)
>>=20
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index c718524056a6..cdf9fe999328 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -14,6 +14,8 @@
>>  #![no_std]
>>  #![feature(allocator_api)]
>>  #![feature(coerce_unsized)]
>> +#![feature(const_ptr_offset_from)]
>> +#![feature(const_refs_to_cell)]
>>  #![feature(core_ffi_c)]
>>  #![feature(dispatch_from_dyn)]
>>  #![feature(explicit_generic_args_with_impl_trait)]
>> @@ -102,3 +104,36 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
>>      // instead of `!`. See <https://github.com/rust-lang/rust-bindgen/i=
ssues/2094>.
>>      loop {}
>>  }
>> +
>> +/// Calculates the offset of a field from the beginning of the struct i=
t belongs to.
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// #[repr(C)]
>> +/// struct Test {
>> +///     a: u64,
>> +///     b: u32,
>> +/// }
>> +///
>> +/// assert_eq!(kernel::offset_of!(Test, b), 8);
>> +/// ```
>> +#[macro_export]
>> +macro_rules! offset_of {
>> +    ($type:ty, $field:ident) =3D> {{
>> +        let tmp =3D ::core::mem::MaybeUninit::<$type>::uninit();
>> +        let outer =3D tmp.as_ptr();
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The pointer is valid and aligned, just not initialis=
ed; `addr_of` ensures that
>> +        // we don't actually read from `outer` (which would be UB) nor =
create an intermediate
>> +        // reference.
>> +        let inner =3D unsafe { ::core::ptr::addr_of!((*outer).$field) }=
 as *const u8;
>> +        // To avoid warnings when nesting `unsafe` blocks.
>> +        #[allow(unused_unsafe)]
>> +        // SAFETY: The two pointers are within the same allocation bloc=
k.
>> +        unsafe {
>> +            inner.offset_from(outer as *const u8) as usize
>> +        }
>
> This has no protection against using `Deref`. The memoffset crate has a=20
>
> let $type { $field: _, .. };
>
> line to ensure that the field is a direct member of type and deref is
> not happening.

Good catch =F0=9F=91=8D

>
>> +    }};
>> +}
>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
>> index 9f94fc83f086..ec583d13dde2 100644
>> --- a/scripts/Makefile.build
>> +++ b/scripts/Makefile.build
>> @@ -277,7 +277,7 @@ $(obj)/%.lst: $(src)/%.c FORCE
>>  # Compile Rust sources (.rs)
>>  # ---------------------------------------------------------------------=
------
>>=20=20
>> -rust_allowed_features :=3D core_ffi_c,explicit_generic_args_with_impl_t=
rait,new_uninit,pin_macro
>> +rust_allowed_features :=3D const_ptr_offset_from,const_refs_to_cell,cor=
e_ffi_c,explicit_generic_args_with_impl_trait,new_uninit,pin_macro
>
> Side note: once we bump our compiler to 1.71, we should switch to the
> built-in `offset_of!` macro and get rid of these unstable features.
>
> Best,
> Gary