mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl
@ 2026-09-13 15:34 Yilin Chen
  2026-09-13 19:37 ` Gary Guo
  0 siblings, 1 reply; 6+ messages in thread
From: Yilin Chen @ 2026-09-13 15:34 UTC (permalink / raw)
  To: rust-for-linux; +Cc: linux-kernel, acourbot, yury.norov, ojeda, Yilin Chen

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl
  2026-09-13 15:34 [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl Yilin Chen
@ 2026-09-13 19:37 ` Gary Guo
  2026-09-21  6:32   ` Yilin Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Gary Guo @ 2026-09-13 19:37 UTC (permalink / raw)
  To: Yilin Chen, rust-for-linux; +Cc: linux-kernel, acourbot, yury.norov, ojeda

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)]



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl
  2026-09-13 19:37 ` Gary Guo
@ 2026-09-21  6:32   ` Yilin Chen
  2026-09-21  7:18     ` Alexandre Courbot
  0 siblings, 1 reply; 6+ messages in thread
From: Yilin Chen @ 2026-09-21  6:32 UTC (permalink / raw)
  To: Gary Guo
  Cc: Yilin Chen, rust-for-linux, linux-kernel, acourbot, yury.norov, ojeda

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl
  2026-09-21  6:32   ` Yilin Chen
@ 2026-09-21  7:18     ` Alexandre Courbot
  2026-09-21 10:05       ` Gary Guo
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Courbot @ 2026-09-21  7:18 UTC (permalink / raw)
  To: Yilin Chen; +Cc: Gary Guo, rust-for-linux, linux-kernel, yury.norov, ojeda

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>`

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl
  2026-09-21  7:18     ` Alexandre Courbot
@ 2026-09-21 10:05       ` Gary Guo
  2026-09-22  5:28         ` [PATCH v2] rust: bitfield: require integer storage Yilin Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Gary Guo @ 2026-09-21 10:05 UTC (permalink / raw)
  To: Alexandre Courbot, Yilin Chen
  Cc: Gary Guo, rust-for-linux, linux-kernel, yury.norov, ojeda

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2] rust: bitfield: require integer storage
  2026-09-21 10:05       ` Gary Guo
@ 2026-09-22  5:28         ` Yilin Chen
  0 siblings, 0 replies; 6+ messages in thread
From: Yilin Chen @ 2026-09-22  5:28 UTC (permalink / raw)
  To: gary, acourbot
  Cc: rust-for-linux, linux-kernel, yury.norov, ojeda, Yilin Chen

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-09-22  5:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-13 15:34 [PATCH] rust: bitfield: require Zeroable storage for Zeroable impl Yilin Chen
2026-09-13 19:37 ` Gary Guo
2026-09-21  6:32   ` Yilin Chen
2026-09-21  7:18     ` Alexandre Courbot
2026-09-21 10:05       ` Gary Guo
2026-09-22  5:28         ` [PATCH v2] rust: bitfield: require integer storage Yilin Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®