diff mbox series

[v2,09/12] rust: init: implement Zeroable for Opaque<T>

Message ID 20230719141918.543938-10-benno.lossin@proton.me
State New
Headers
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

Alice Ryhl July 20, 2023, 1:34 p.m. UTC | #1 
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
Martin Rodriguez Reboredo July 20, 2023, 2:03 p.m. UTC | #2 
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>
Benno Lossin July 24, 2023, 2:16 p.m. UTC | #3 
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).
Alice Ryhl July 25, 2023, 11:57 a.m. UTC | #4 
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
Benno Lossin July 29, 2023, 4:11 a.m. UTC | #5 
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)?
Alice Ryhl July 29, 2023, 8:14 a.m. UTC | #6 
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 mbox series

Patch

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,