From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me --- rust/kernel/init.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index 424257284d16..538e03cfc84a 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -1292,8 +1292,8 @@ 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. + {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
// SAFETY: Type is allowed to take any value, including all zeros. {<T>} MaybeUninit<T>,
base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
On Thu, Mar 14, 2024 at 12:09 AM Benno Lossin benno.lossin@proton.me wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
Reviewed-by: Alice Ryhl aliceryhl@google.com
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Reviewed-by: Boqun Feng boqun.feng@gmail.com
Regards, Boqun
rust/kernel/init.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index 424257284d16..538e03cfc84a 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -1292,8 +1292,8 @@ 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.
- {<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
// SAFETY: Type is allowed to take any value, including all zeros. {<T>} MaybeUninit<T>,
base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
2.42.0
On Mar 18, 2024, at 10:25 AM, Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Would the following be okay for that purpose?
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. Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
Thanks, Laine
Reviewed-by: Boqun Feng boqun.feng@gmail.com
Regards, Boqun
rust/kernel/init.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index 424257284d16..538e03cfc84a 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -1292,8 +1292,8 @@ 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.
{<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
// SAFETY: Type is allowed to take any value, including all zeros. {<T>} MaybeUninit<T>,
base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
2.42.0
On Mon, Mar 18, 2024 at 08:17:07PM -0700, Laine Taffin Altman wrote:
On Mar 18, 2024, at 10:25 AM, Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Would the following be okay for that purpose?
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. Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
Yeah, this works for me. Thanks!
Regards, Boqun
Thanks, Laine
Reviewed-by: Boqun Feng boqun.feng@gmail.com
Regards, Boqun
rust/kernel/init.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index 424257284d16..538e03cfc84a 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -1292,8 +1292,8 @@ 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.
{<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
// SAFETY: Type is allowed to take any value, including all zeros. {<T>} MaybeUninit<T>,
base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
2.42.0
On Mar 18, 2024, at 9:39 PM, Boqun Feng boqun.feng@gmail.com wrote:
On Mon, Mar 18, 2024 at 08:17:07PM -0700, Laine Taffin Altman wrote:
On Mar 18, 2024, at 10:25 AM, Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Would the following be okay for that purpose?
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. Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
Yeah, this works for me. Thanks!
Great! Should it be re-sent or can the new wording be incorporated upon merge?
Thank, Laine
Regards, Boqun
Thanks, Laine
Reviewed-by: Boqun Feng boqun.feng@gmail.com
Regards, Boqun
rust/kernel/init.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/init.rs b/rust/kernel/init.rs index 424257284d16..538e03cfc84a 100644 --- a/rust/kernel/init.rs +++ b/rust/kernel/init.rs @@ -1292,8 +1292,8 @@ 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.
{<T: ?Sized>} PhantomData<T>, core::marker::PhantomPinned, (),
// SAFETY: Type is allowed to take any value, including all zeros. {<T>} MaybeUninit<T>,
base-commit: 768409cff6cc89fe1194da880537a09857b6e4db
2.42.0
On 3/19/24 06:28, Laine Taffin Altman wrote:
On Mar 18, 2024, at 9:39 PM, Boqun Feng boqun.feng@gmail.com wrote:
On Mon, Mar 18, 2024 at 08:17:07PM -0700, Laine Taffin Altman wrote:
On Mar 18, 2024, at 10:25 AM, Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Would the following be okay for that purpose?
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. Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
Yeah, this works for me. Thanks!
Great! Should it be re-sent or can the new wording be incorporated upon merge?
I can re-send it for you again, or do you want to send it yourself? I think it is also a good idea to add a link to [1] in the code, since the above explanation is rather long and fits better in the commit message.
On Tue, Mar 19, 2024 at 11:34 AM Benno Lossin benno.lossin@proton.me wrote:
I can re-send it for you again, or do you want to send it yourself? I think it is also a good idea to add a link to [1] in the code, since the above explanation is rather long and fits better in the commit message.
Agreed, if you want to have a note in the code itself (to avoid mistakes re-adding them, I imagine), then I would say a short sentence + link is best.
Your link is a good one for an explanation, since it mentions explicitly the UB. The reference's list [1] would be a good fit for non-explanation purposes -- it mentions explicitly `!` (and `Infallible` is supposed to eventually be an alias as far as I know).
In addition, while it is not important in this case (i.e. most likely nobody is affected), it doesn't hurt to include an example that shows the issue in general for this sort of patches, i.e. what kind of code will be prevented from compiling, e.g.
pr_info!("{}", Box::core::convert::Infallible::init(kernel::init::zeroed())?);
In any case, even v1 looks good to me -- thanks!
[1] https://doc.rust-lang.org/reference/behavior-considered-undefined.html
Cheers, Miguel
On Mar 19, 2024, at 3:34 AM, Benno Lossin benno.lossin@proton.me wrote:
On 3/19/24 06:28, Laine Taffin Altman wrote:
On Mar 18, 2024, at 9:39 PM, Boqun Feng boqun.feng@gmail.com wrote:
On Mon, Mar 18, 2024 at 08:17:07PM -0700, Laine Taffin Altman wrote:
On Mar 18, 2024, at 10:25 AM, Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote:
From: Laine Taffin Altman alexanderaltman@me.com
It is not enough for a type to be a ZST to guarantee that zeroed memory is a valid value for it; it must also be inhabited. Creating a value of an uninhabited type, ZST or no, is immediate UB. Thus remove the implementation of `Zeroable` for `Infallible`, since that type is not inhabited.
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 Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Would the following be okay for that purpose?
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. Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
Yeah, this works for me. Thanks!
Great! Should it be re-sent or can the new wording be incorporated upon merge?
I can re-send it for you again, or do you want to send it yourself? I think it is also a good idea to add a link to [1] in the code, since the above explanation is rather long and fits better in the commit message.
I’ll try and do it myself; thank you for sending the first round for me and illustrating procedures! What Reviewed-By’s/Signed-Off-By's should I retain?
Thanks, Laine
-- Cheers, Benno
On Thu, Mar 21, 2024 at 5:53 AM Laine Taffin Altman alexanderaltman@me.com wrote:
I’ll try and do it myself; thank you for sending the first round for me and illustrating procedures! What Reviewed-By’s/Signed-Off-By's should I retain?
For the Signed-off-by, only yours is OK. For the Reviewed-by, it depends on how much you have changed, i.e. whether you consider their review does not apply anymore -- please see https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement...
Cheers, Miguel
On 21.03.24 05:53, Laine Taffin Altman wrote:
On Mar 19, 2024, at 3:34 AM, Benno Lossin benno.lossin@proton.me wrote:
On 3/19/24 06:28, Laine Taffin Altman wrote:
On Mar 18, 2024, at 9:39 PM, Boqun Feng boqun.feng@gmail.com wrote:
On Mon, Mar 18, 2024 at 08:17:07PM -0700, Laine Taffin Altman wrote:
On Mar 18, 2024, at 10:25 AM, Boqun Feng boqun.feng@gmail.com wrote:
On Wed, Mar 13, 2024 at 11:09:37PM +0000, Benno Lossin wrote: > From: Laine Taffin Altman alexanderaltman@me.com > > It is not enough for a type to be a ZST to guarantee that zeroed memory > is a valid value for it; it must also be inhabited. Creating a value of > an uninhabited type, ZST or no, is immediate UB. > Thus remove the implementation of `Zeroable` for `Infallible`, since > that type is not inhabited. > > 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 > Signed-off-by: Benno Lossin benno.lossin@proton.me
I think either in the commit log or in the code comment, there better be a link or explanation on "(un)inhabited type". The rest looks good to me.
Would the following be okay for that purpose?
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. Thus, remove the implementation of `Zeroable` for `Infallible`, thereby avoiding the UB.
Yeah, this works for me. Thanks!
Great! Should it be re-sent or can the new wording be incorporated upon merge?
I can re-send it for you again, or do you want to send it yourself? I think it is also a good idea to add a link to [1] in the code, since the above explanation is rather long and fits better in the commit message.
I’ll try and do it myself; thank you for sending the first round for me and illustrating procedures! What Reviewed-By’s/Signed-Off-By's should I retain?
Do you still want to send it yourself? If you don't have the time, no problem, I can send it again.
I’ll do it myself tomorrow night; sorry for the delay!
Thanks, Laine
On Mar 30, 2024, at 5:03 AM, Benno Lossin benno.lossin@proton.me wrote:
<mime-attachment> <encrypted.asc>
On 30.03.24 17:36, Laine Taffin Altman wrote:
I’ll do it myself tomorrow night; sorry for the delay!
Great, and no worries (it's fine if you still need a couple days)!
Thanks, Laine
On Mar 30, 2024, at 5:03 AM, Benno Lossin benno.lossin@proton.me wrote:
<mime-attachment> <encrypted.asc>
Sorry about that, I hope this one is not encrypted.
linux-stable-mirror@lists.linaro.org
-
Alice Ryhl -
Benno Lossin -
Boqun Feng -
Laine Taffin Altman -
Miguel Ojeda