rust: bitfield: require Zeroable storage for Zeroable impl

[PATCH] rust: bitfield: require Zeroable storage for Zeroable impl

Posted by Yilin Chen 1 week, 4 days ago

The bitfield! macro implements Zeroable for generated wrapper
types. This assumes the storage type accepts an all-zero bit
pattern. However, the macro accepts any type and does not encode
that requirement.

Add a Zeroable bound to the generated implementation. This prevents
invalid storage types from obtaining an unsound Zeroable implementation.

Fixes: b7b8b4ccdad4 ("rust: extract `bitfield!` macro from `register!`")
Assisted-by: Codex:GPT-5.6 Sol
Signed-off-by: Yilin Chen <1479826151@qq.com>
---
 rust/kernel/bitfield.rs | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
index a0d0894..3dd4dea 100644
--- a/rust/kernel/bitfield.rs
+++ b/rust/kernel/bitfield.rs
@@ -330,8 +330,12 @@ macro_rules! bitfield {
             }
         }
 
-        // SAFETY: `$storage` is `Zeroable` and `$name` is transparent.
-        unsafe impl ::pin_init::Zeroable for $name {}
+        // SAFETY:
+        // - `$storage: Zeroable` guarantees that the all-zero bit pattern is valid.
+        // - `$name` is `repr(transparent)` over `$storage`.
+        unsafe impl ::pin_init::Zeroable for $name
+        where $storage: ::pin_init::Zeroable
+        {}
 
         impl ::core::convert::From<$name> for $storage {
             #[inline(always)]
-- 
2.25.1

Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl

Posted by Gary Guo 1 week, 4 days ago

On Sun Sep 13, 2026 at 4:34 PM BST, Yilin Chen wrote:
> The bitfield! macro implements Zeroable for generated wrapper
> types. This assumes the storage type accepts an all-zero bit
> pattern. However, the macro accepts any type and does not encode
> that requirement.

How? The bitfield macro will fail for types other than primtive integers.

Best,
Gary

>
> Add a Zeroable bound to the generated implementation. This prevents
> invalid storage types from obtaining an unsound Zeroable implementation.
>
> Fixes: b7b8b4ccdad4 ("rust: extract `bitfield!` macro from `register!`")
> Assisted-by: Codex:GPT-5.6 Sol
> Signed-off-by: Yilin Chen <1479826151@qq.com>
> ---
>  rust/kernel/bitfield.rs | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
> index a0d0894..3dd4dea 100644
> --- a/rust/kernel/bitfield.rs
> +++ b/rust/kernel/bitfield.rs
> @@ -330,8 +330,12 @@ macro_rules! bitfield {
>              }
>          }
>  
> -        // SAFETY: `$storage` is `Zeroable` and `$name` is transparent.
> -        unsafe impl ::pin_init::Zeroable for $name {}
> +        // SAFETY:
> +        // - `$storage: Zeroable` guarantees that the all-zero bit pattern is valid.
> +        // - `$name` is `repr(transparent)` over `$storage`.
> +        unsafe impl ::pin_init::Zeroable for $name
> +        where $storage: ::pin_init::Zeroable
> +        {}
>  
>          impl ::core::convert::From<$name> for $storage {
>              #[inline(always)]

Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl

Posted by Yilin Chen 4 days, 4 hours ago

Hi Gary,

Resending my reply for visibility on the mailing list.

You are right that a bitfield with at least one field generates a use of
`Bounded<$storage, ...>`, which requires the storage type to implement
`Integer`. However, `bitfield!` also accepts an empty field list. In that
case, no `Bounded` use is generated, so the `Integer` requirement is
absent, while the macro still generates the unconditional `Zeroable`
implementation.

For example:

    use core::num::NonZeroU32;
    use pin_init::Zeroable;

    bitfield! {
        struct Bad(NonZeroU32) {}
    }

    fn check_bad_zeroable() {
        let _: Bad = <Bad as Zeroable>::zeroed();
    }

This currently compiles. Since the all-zero bit pattern is invalid for
`NonZeroU32`, the generated `Zeroable` implementation is unsound. This is
why I think the explicit `$storage: Zeroable` bound is necessary.

Best regards,
Yilin

Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl

Posted by Alexandre Courbot 4 days, 3 hours ago

On Mon Sep 21, 2026 at 3:32 PM JST, Yilin Chen wrote:
> Hi Gary,
>
> Resending my reply for visibility on the mailing list.
>
> You are right that a bitfield with at least one field generates a use of
> `Bounded<$storage, ...>`, which requires the storage type to implement
> `Integer`. However, `bitfield!` also accepts an empty field list. In that
> case, no `Bounded` use is generated, so the `Integer` requirement is
> absent, while the macro still generates the unconditional `Zeroable`
> implementation.
>
> For example:
>
>     use core::num::NonZeroU32;
>     use pin_init::Zeroable;
>
>     bitfield! {
>         struct Bad(NonZeroU32) {}
>     }
>
>     fn check_bad_zeroable() {
>         let _: Bad = <Bad as Zeroable>::zeroed();
>     }
>
> This currently compiles.

No it doesn't.

error[E0277]: the trait bound `core::num::NonZero<u32>: kernel::mem::AsRepr` is not satisfied
364 | /         bitfield! {
365 | |             struct Bad(NonZeroU32) {}
366 | |         }
    | |_________^ the trait `kernel::mem::AsRepr` is not implemented for `core::num::NonZero<u32>`

Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl

Posted by Gary Guo 4 days ago

On Mon Sep 21, 2026 at 8:18 AM BST, Alexandre Courbot wrote:
> On Mon Sep 21, 2026 at 3:32 PM JST, Yilin Chen wrote:
>> Hi Gary,
>>
>> Resending my reply for visibility on the mailing list.
>>
>> You are right that a bitfield with at least one field generates a use of
>> `Bounded<$storage, ...>`, which requires the storage type to implement
>> `Integer`. However, `bitfield!` also accepts an empty field list. In that
>> case, no `Bounded` use is generated, so the `Integer` requirement is
>> absent, while the macro still generates the unconditional `Zeroable`
>> implementation.
>>
>> For example:
>>
>>     use core::num::NonZeroU32;
>>     use pin_init::Zeroable;
>>
>>     bitfield! {
>>         struct Bad(NonZeroU32) {}
>>     }
>>
>>     fn check_bad_zeroable() {
>>         let _: Bad = <Bad as Zeroable>::zeroed();
>>     }
>>
>> This currently compiles.
>
> No it doesn't.
>
> error[E0277]: the trait bound `core::num::NonZero<u32>: kernel::mem::AsRepr` is not satisfied
> 364 | /         bitfield! {
> 365 | |             struct Bad(NonZeroU32) {}
> 366 | |         }
>     | |_________^ the trait `kernel::mem::AsRepr` is not implemented for `core::num::NonZero<u32>`

On the other hand, it makes sense for `NonZero<u32>` to implement
`AsRepr<Repr = u32>`. So I think we do need better defense.

Given that fundamental `bitfield!` needs a plain integer type, I don't think we
want a `Zeroable` bound, but rather just enforce it has to be integer
primitives.

Given the recent `Integer` sealing, ad a check that $storage implements
`Integer` is probably the best check.

Best,
Gary

Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl

Posted by Alexandre Courbot 2 days, 20 hours ago

On Mon Sep 21, 2026 at 7:05 PM JST, Gary Guo wrote:
> On Mon Sep 21, 2026 at 8:18 AM BST, Alexandre Courbot wrote:
>> On Mon Sep 21, 2026 at 3:32 PM JST, Yilin Chen wrote:
>>> Hi Gary,
>>>
>>> Resending my reply for visibility on the mailing list.
>>>
>>> You are right that a bitfield with at least one field generates a use of
>>> `Bounded<$storage, ...>`, which requires the storage type to implement
>>> `Integer`. However, `bitfield!` also accepts an empty field list. In that
>>> case, no `Bounded` use is generated, so the `Integer` requirement is
>>> absent, while the macro still generates the unconditional `Zeroable`
>>> implementation.
>>>
>>> For example:
>>>
>>>     use core::num::NonZeroU32;
>>>     use pin_init::Zeroable;
>>>
>>>     bitfield! {
>>>         struct Bad(NonZeroU32) {}
>>>     }
>>>
>>>     fn check_bad_zeroable() {
>>>         let _: Bad = <Bad as Zeroable>::zeroed();
>>>     }
>>>
>>> This currently compiles.
>>
>> No it doesn't.
>>
>> error[E0277]: the trait bound `core::num::NonZero<u32>: kernel::mem::AsRepr` is not satisfied
>> 364 | /         bitfield! {
>> 365 | |             struct Bad(NonZeroU32) {}
>> 366 | |         }
>>     | |_________^ the trait `kernel::mem::AsRepr` is not implemented for `core::num::NonZero<u32>`
>
> On the other hand, it makes sense for `NonZero<u32>` to implement
> `AsRepr<Repr = u32>`. So I think we do need better defense.
>
> Given that fundamental `bitfield!` needs a plain integer type, I don't think we
> want a `Zeroable` bound, but rather just enforce it has to be integer
> primitives.
>
> Given the recent `Integer` sealing, ad a check that $storage implements
> `Integer` is probably the best check.

That, or we only derive `Zeroable` on a bitfield if its storage type
also implements `Zeroable`.

... but that possibly leaves other ways in which the invariant for
`NonZero` would not be enforced, and I don't see the point for a
bitfield to be backed by anything but a primitive type, so maybe
limiting storage to implementors of `Integer` is the right move indeed.

Yilin, do you want to send a patch for this?

[PATCH v2] rust: bitfield: require integer storage

Posted by Yilin Chen 3 days, 5 hours ago

The bitfield! macro generates an unconditional Zeroable implementation
for its wrapper type. An empty field list generates no Bounded usage, so
the storage type can bypass the Integer requirement.

Require the storage type to implement the sealed Integer trait for the
generated Zeroable implementation. This ensures that bitfield storage is
limited to primitive integer types with a valid all-zero bit pattern.

Fixes: b7b8b4ccdad4 ("rust: extract `bitfield!` macro from `register!`")
Assisted-by: GPT-5.6 Sol
Signed-off-by: Yilin Chen <1479826151@qq.com>
---
Changes in v2:
- Add `where $storage: ::kernel::num::Integer` bound.
- Update `// SAFETY` section.
---

I track the default rust-next branch, and there is not any code about
`AsRepr` in that branch. So in patch v1, I didn't know that case could
not compile. Thank you for your feedback!

 rust/kernel/bitfield.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
index a0d089423f21..b1fc98d7f8c3 100644
--- a/rust/kernel/bitfield.rs
+++ b/rust/kernel/bitfield.rs
@@ -330,8 +330,13 @@ impl $name {
             }
         }
 
-        // SAFETY: `$storage` is `Zeroable` and `$name` is transparent.
-        unsafe impl ::pin_init::Zeroable for $name {}
+        // SAFETY:
+        // - `$storage: Integer` is sealed to primitive integer types, for which the all-zero bit
+        //   pattern is valid.
+        // - `$name` is `repr(transparent)` over `$storage`.
+        unsafe impl ::pin_init::Zeroable for $name
+        where $storage: ::kernel::num::Integer
+        {}
 
         impl ::core::convert::From<$name> for $storage {
             #[inline(always)]
-- 
2.25.1

Re: [PATCH v2] rust: bitfield: require integer storage

Posted by Alexandre Courbot 2 days, 20 hours ago

On Tue Sep 22, 2026 at 2:28 PM JST, Yilin Chen wrote:
> The bitfield! macro generates an unconditional Zeroable implementation
> for its wrapper type. An empty field list generates no Bounded usage, so
> the storage type can bypass the Integer requirement.
>
> Require the storage type to implement the sealed Integer trait for the
> generated Zeroable implementation. This ensures that bitfield storage is
> limited to primitive integer types with a valid all-zero bit pattern.
>
> Fixes: b7b8b4ccdad4 ("rust: extract `bitfield!` macro from `register!`")
> Assisted-by: GPT-5.6 Sol
> Signed-off-by: Yilin Chen <1479826151@qq.com>

Looks like my wish [1] has been instantly granted.

[1] https://lore.kernel.org/rust-for-linux/DLLVY7G20JJA.2B0KTTODUU9FG@nvidia.com/

> ---
> Changes in v2:
> - Add `where $storage: ::kernel::num::Integer` bound.
> - Update `// SAFETY` section.
> ---
>
> I track the default rust-next branch, and there is not any code about
> `AsRepr` in that branch. So in patch v1, I didn't know that case could
> not compile. Thank you for your feedback!
>
>  rust/kernel/bitfield.rs | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
> index a0d089423f21..b1fc98d7f8c3 100644
> --- a/rust/kernel/bitfield.rs
> +++ b/rust/kernel/bitfield.rs
> @@ -330,8 +330,13 @@ impl $name {
>              }
>          }
>  
> -        // SAFETY: `$storage` is `Zeroable` and `$name` is transparent.
> -        unsafe impl ::pin_init::Zeroable for $name {}
> +        // SAFETY:
> +        // - `$storage: Integer` is sealed to primitive integer types, for which the all-zero bit
> +        //   pattern is valid.
> +        // - `$name` is `repr(transparent)` over `$storage`.
> +        unsafe impl ::pin_init::Zeroable for $name
> +        where $storage: ::kernel::num::Integer
> +        {}

While I guess that would somehow work, I think the proper place to do
this is the struct definition, i.e:

  $vis struct $name
  where
      $storage: $crate::num::Integer,
  {
      inner: $storage,
  }

This makes the error message also more explicit about what the problem
is (a bitfield requires an `Integer`, not merely the ability to
initialize it to zero).

Re: [PATCH v2] rust: bitfield: require integer storage

Posted by Gary Guo 2 days, 20 hours ago

On Tue Sep 22, 2026 at 2:53 PM BST, Alexandre Courbot wrote:
>>  
>> -        // SAFETY: `$storage` is `Zeroable` and `$name` is transparent.
>> -        unsafe impl ::pin_init::Zeroable for $name {}
>> +        // SAFETY:
>> +        // - `$storage: Integer` is sealed to primitive integer types, for which the all-zero bit
>> +        //   pattern is valid.
>> +        // - `$name` is `repr(transparent)` over `$storage`.
>> +        unsafe impl ::pin_init::Zeroable for $name
>> +        where $storage: ::kernel::num::Integer
>> +        {}
>
> While I guess that would somehow work, I think the proper place to do
> this is the struct definition, i.e:
>
>   $vis struct $name
>   where
>       $storage: $crate::num::Integer,
>   {
>       inner: $storage,
>   }
>
> This makes the error message also more explicit about what the problem
> is (a bitfield requires an `Integer`, not merely the ability to
> initialize it to zero).

Agreed.

Best,
Gary