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 `Box::<core::convert::Infallible>::init(kernel::init::zeroed())`.

Thus, remove the implementation of `Zeroable` for `Infallible`, thereby
avoiding the unsoundness (potential for future 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>
---
V3 -> V4: Address review nits; run checkpatch properly.
V2 -> V3: Email formatting correction.
V1 -> V2: Added more documentation to the comment, with links; also added more details to the commit message.

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

diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
index 424257284d16..3859c7ff81b7 100644
--- a/rust/kernel/init.rs
+++ b/rust/kernel/init.rs
@@ -1292,8 +1292,15 @@ 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, (),
+    // Note: do not add uninhabited types (such as `!` or `core::convert::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
+    //
+    // SAFETY: These are inhabited ZSTs; there is nothing to zero and a valid value exists.
+    {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
 
     // SAFETY: Type is allowed to take any value, including all zeros.
     {<T>} MaybeUninit<T>,

base-commit: c85af715cac0a951eea97393378e84bb49384734
-- 
2.44.0

On Wed, Apr 3, 2024 at 11:07 PM Laine Taffin Altman
<alexanderaltman@me.com> wrote:
>
> 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 `Box::<core::convert::Infallible>::init(kernel::init::zeroed())`.
>
> Thus, remove the implementation of `Zeroable` for `Infallible`, thereby
> avoiding the unsoundness (potential for future 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>

[ Reformatted the comment slightly. ]

Applied to `rust-fixes` -- thanks everyone! Please feel free to still send tags.

Cheers,
Miguel

On 03.04.24 23:06, Laine Taffin Altman wrote:
> 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 `Box::<core::convert::Infallible>::init(kernel::init::zeroed())`.
> 
> Thus, remove the implementation of `Zeroable` for `Infallible`, thereby
> avoiding the unsoundness (potential for future 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>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
> V3 -> V4: Address review nits; run checkpatch properly.
> V2 -> V3: Email formatting correction.
> V1 -> V2: Added more documentation to the comment, with links; also added more details to the commit message.
> 
>  rust/kernel/init.rs | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs
> index 424257284d16..3859c7ff81b7 100644
> --- a/rust/kernel/init.rs
> +++ b/rust/kernel/init.rs
> @@ -1292,8 +1292,15 @@ 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, (),
> +    // Note: do not add uninhabited types (such as `!` or `core::convert::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
> +    //
> +    // SAFETY: These are inhabited ZSTs; there is nothing to zero and a valid value exists.
> +    {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
> 
>      // SAFETY: Type is allowed to take any value, including all zeros.
>      {<T>} MaybeUninit<T>,
> 
> base-commit: c85af715cac0a951eea97393378e84bb49384734

I don't see this commit in the kernel tree, what did you specify as
`--base` when running `git format`?

-- 
Cheers,
Benno

On Thu, Apr 4, 2024 at 11:01 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> I don't see this commit in the kernel tree, what did you specify as
> `--base` when running `git format`?

Yeah, I don't have it either, but it seems to apply cleanly.

Cheers,
Miguel

On Apr 4, 2024, at 4:03 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Thu, Apr 4, 2024 at 11:01 AM Benno Lossin <benno.lossin@proton.me> wrote:
>> 
>> I don't see this commit in the kernel tree, what did you specify as
>> `--base` when running `git format`?
> 
> Yeah, I don't have it either, but it seems to apply cleanly.
> 
> Cheers,
> Miguel

I ran `git format-patch origin/master --base=origin/master`.  I
can’t imagine how that could have resulted in a nonexistent commit
hash?

Thanks,
Laine

On 04.04.24 19:28, Laine Taffin Altman wrote:
> On Apr 4, 2024, at 4:03 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> On Thu, Apr 4, 2024 at 11:01 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>> I don't see this commit in the kernel tree, what did you specify as
>>> `--base` when running `git format`?
>>
>> Yeah, I don't have it either, but it seems to apply cleanly.
>>
>> Cheers,
>> Miguel
> 
> I ran `git format-patch origin/master --base=origin/master`.  I
> can’t imagine how that could have resulted in a nonexistent commit
> hash?

That heavily depends on what `origin/master` is. Is `origin` pointing to
Torvald's git? In that case it would explain why we don't have that
commit hash yet.
Normally you should base your work on the tree listed in the `T:` entry
of the subsystem in the MAINTAINERS file. In our case it is `rust-next`.
But no worries, since it applies cleanly it should be fine for this
patch. Just something to keep in mind if you submit any future patches.

-- 
Cheers,
Benno

On Apr 7, 2024, at 1:56 AM, Benno Lossin <benno.lossin@proton.me> wrote:
> On 04.04.24 19:28, Laine Taffin Altman wrote:
>>> On Apr 4, 2024, at 4:03 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>>> 
>>> On Thu, Apr 4, 2024 at 11:01 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>>> 
>>>> I don't see this commit in the kernel tree, what did you specify as
>>>> `--base` when running `git format`?
>>> 
>>> Yeah, I don't have it either, but it seems to apply cleanly.
>>> 
>>> Cheers,
>>> Miguel
>> 
>> I ran `git format-patch origin/master --base=origin/master`.  I
>> can’t imagine how that could have resulted in a nonexistent commit
>> hash?
> 
> That heavily depends on what `origin/master` is. Is `origin` pointing to
> Torvald's git? In that case it would explain why we don't have that
> commit hash yet.
> Normally you should base your work on the tree listed in the `T:` entry
> of the subsystem in the MAINTAINERS file. In our case it is `rust-next`.
> But no worries, since it applies cleanly it should be fine for this
> patch. Just something to keep in mind if you submit any future patches.
> 
> --
> Cheers,
> Benno

Ah, that makes sense!  Indeed it was pointing there.  I’ll remember for next time!

Thanks,
Laine