[PATCH v2] rust: init: remove impl Zeroable for Infallible

[PATCH v2] rust: init: remove impl Zeroable for Infallible

[Laine Taffin Altman]

A type is inhabited if at least one valid value of that type exists; a type is uninhabited if no valid values of that type exist.  The terms "inhabited" and "uninhabited" in this sense originate in type theory, a branch of mathematics.

In Rust, producing an invalid value of any type is immediate undefined behavior (UB); this includes via zeroing memory.  Therefore, since an uninhabited type has no valid values, producing any values at all for it is UB.

The Rust standard library type `core::convert::Infallible` is uninhabited, by virtue of having been declared as an enum with no cases, which always produces uninhabited types in Rust.

The current kernel code allows this UB to be triggered, for example by code like:
`pr_info!("{}”, Box::<core::convert::Infallible>::init(kernel::init::zeroed())?);`

Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.

Cc: stable@vger.kernel.org
Fixes: 38cde0bd7b67 ("rust: init: add `Zeroable` trait and `init::zeroed` function")
Closes: https://github.com/Rust-for-Linux/pinned-init/pull/13
Signed-off-by: Laine Taffin Altman <alexanderaltman@me.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
—
V1 -> V2: Added more documentation to the comment, with links; also added more details to the commit message.

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

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 424257284d16..9353c9919fd4 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -1292,8 +1292,11 @@ macro_rules! impl_zeroable {
     i8, i16, i32, i64, i128, isize,
     f32, f64,
 
-    // SAFETY: These are ZSTs, there is nothing to zero.
-    {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, Infallible, (),
+    // SAFETY: These are inhabited ZSTs; there is nothing to zero and a valid value exists.
+    // Note: do not add uninhabited types (such as ! or Infallible) to this list; creating an instance of an uninhabited type is immediate undefined behavior.
+    // For more on uninhabited/empty types, consult The Rustonomicon: https://doc.rust-lang.org/stable/nomicon/exotic-sizes.html#empty-types
+    // The Rust Reference also has information on undefined behavior: https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html
+    {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
 
     // SAFETY: Type is allowed to take any value, including all zeros.
     {<T>} MaybeUninit<T>,
-- 
2.44.0

Re: [PATCH v2] rust: init: remove impl Zeroable for Infallible

[Laine Taffin Altman]

Oops!  Formatting issue; ignore this one.

Thanks,
Laine

> On Apr 1, 2024, at 6:53 PM, Laine Taffin Altman <alexanderaltman@me.com> wrote:
> 
> A type is inhabited if at least one valid value of that type exists; a type is uninhabited if no valid values of that type exist.  The terms "inhabited" and "uninhabited" in this sense originate in type theory, a branch of mathematics.
> 
> In Rust, producing an invalid value of any type is immediate undefined behavior (UB); this includes via zeroing memory.  Therefore, since an uninhabited type has no valid values, producing any values at all for it is UB.
> 
> The Rust standard library type `core::convert::Infallible` is uninhabited, by virtue of having been declared as an enum with no cases, which always produces uninhabited types in Rust.
> 
> The current kernel code allows this UB to be triggered, for example by code like:
> `pr_info!("{}”, Box::<core::convert::Infallible>::init(kernel::init::zeroed())?);`
> 
> Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
> 
> Cc: stable@vger.kernel.org
> Fixes: 38cde0bd7b67 ("rust: init: add `Zeroable` trait and `init::zeroed` function")
> Closes: https://github.com/Rust-for-Linux/pinned-init/pull/13
> Signed-off-by: Laine Taffin Altman <alexanderaltman@me.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> —
> V1 -> V2: Added more documentation to the comment, with links; also added more details to the commit message.
> 
> rust/kernel/init.rs | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 424257284d16..9353c9919fd4 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -1292,8 +1292,11 @@ macro_rules! impl_zeroable {
>     i8, i16, i32, i64, i128, isize,
>     f32, f64,
> 
> -    // SAFETY: These are ZSTs, there is nothing to zero.
> -    {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, Infallible, (),
> +    // SAFETY: These are inhabited ZSTs; there is nothing to zero and a valid value exists.
> +    // Note: do not add uninhabited types (such as ! or Infallible) to this list; creating an instance of an uninhabited type is immediate undefined behavior.
> +    // For more on uninhabited/empty types, consult The Rustonomicon: https://doc.rust-lang.org/stable/nomicon/exotic-sizes.html#empty-types
> +    // The Rust Reference also has information on undefined behavior: https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html
> +    {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
> 
>     // SAFETY: Type is allowed to take any value, including all zeros.
>     {<T>} MaybeUninit<T>,
> -- 
> 2.44.0

Re: [PATCH v2] rust: init: remove impl Zeroable for Infallible

[Miguel Ojeda]

On Tue, Apr 2, 2024 at 3:53 AM Laine Taffin Altman
<alexanderaltman@me.com> wrote:
>
> A type is inhabited if at least one valid value of that type exists; a type is uninhabited if no valid values of that type exist.  The terms "inhabited" and "uninhabited" in this sense originate in type theory, a branch of mathematics.
>
> In Rust, producing an invalid value of any type is immediate undefined behavior (UB); this includes via zeroing memory.  Therefore, since an uninhabited type has no valid values, producing any values at all for it is UB.
>
> The Rust standard library type `core::convert::Infallible` is uninhabited, by virtue of having been declared as an enum with no cases, which always produces uninhabited types in Rust.
>
> The current kernel code allows this UB to be triggered, for example by code like:
> `pr_info!("{}”, Box::<core::convert::Infallible>::init(kernel::init::zeroed())?);`
>
> Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.

Do you agree with replacing the last part here with "avoiding the
unsoundness issue" or similar instead?

i.e. there is no UB in the kernel (related to this), so it isn't
avoided in that sense. Of course, you mean that we avoid potential UB
to be written in the future, but I think it is useful to distinguish
between patches for "holes" in the extra layer of "protection" vs.
patches that actually triggered UB.

> +    // SAFETY: These are inhabited ZSTs; there is nothing to zero and a valid value exists.

Typically we would add an empty line here, and we would put the SAFETY
comment below (i.e. closer to the code) while the rest above.

> +    // Note: do not add uninhabited types (such as ! or Infallible) to this list; creating an instance of an uninhabited type is immediate undefined behavior.

Nit: this could use Markdown (`!`, `Infallible`).

Otherwise, apart from these nits and the formatting bit, it looks good to me.

If you could send a quick v4, that would be great, thanks a lot!

Cheers,
Miguel