rust: pin-init: internal: init: remove `#[disable_initialized_field_access]`
Subsystems (3)
Benno Lossin
maintainerPATCH v2 1/2 rust: pin-init: internal: init: remove `#[disable_initialized_field_access]`
Gary noticed [1] that the initializer macros as well as the [Pin]Init traits cannot support unaligned fields, since they use operations that require aligned pointers. This means that any code using structs with unaligned fields in pin-init is unsound.
By default, the init! macro generates references to initialized fields, which makes the compiler check that those fields are aligned. However, we added the #[disable_initialized_field_access] attribute to avoid this behavior in ceca298c53f9 ("rust: pin-init: internal: init:
add escape hatch for referencing initialized fields"). Thus remove the #[disable_initialized_field_access] attribute from init!, which is the only safe way to create an initializer handling unaligned fields.
If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
---
Changelog:
* changes since v1: https://lore.kernel.org/all/[email protected]
- improved note added to the code in patch 2
- improved commit messages
---
This commit does not need backporting, as ceca298c53f9 is not yet in any stable tree.
However, the unsoundness still affects two stable trees, because it was unknowingly fixed in commit 42415d163e5d ("rust: pin-init: add references to previously initialized fields"). Before then, packed structs compiled without any issues with pin-init and thus all prior kernel versions with pin-init that do not contain that commit are affected.
We introduced pin-init in 90e53c5e70a6 ("rust: add pin-init API core"), which was included in 6.4. The affected stable trees that are still maintained are: 6.12 and 6.6. Note that 6.18 and 6.19 already contain 42415d163e5d, so they are unaffected.
I will prepare a separate patch series to backport 42415d163e5d to each of the affected trees, including the second patch of this series that documents the fact that field accessors are load-bearing for soundness.
Janne Grunau has worked around [3] this problem in the AOP audio driver downstream, which was the reason for adding the #[disable_initialized_field_access] attribute in the first place.
[3]: https://lore.kernel.org/all/[email protected]
--- rust/pin-init/internal/src/init.rs | 39 ++++++------------------------ 1 file changed, 8 insertions(+), 31 deletions(-)
rust/pin-init/internal/src/init.rs +8 -31
@@ -62,7 +62,6 @@ fn ident(&self) -> Option<&Ident> {enum InitializerAttribute {DefaultError (DefaultErrorAttribute ), -DisableInitializedFieldAccess , }struct DefaultErrorAttribute {
@@ -86,6 +85,7 @@ pub(crate) fn expand( let error = error.map_or_else( || { if let Some(default_error) = attrs.iter().fold(None, |acc, attr| { + #[expect(irrefutable_let_patterns)] if let InitializerAttribute::DefaultError(DefaultErrorAttribute { ty }) = attr { Some(ty.clone()) } else {
@@ -145,15 +145,7 @@ fn assert_zeroable<T: ?::core::marker::Sized>(_: *mut T) };// `mixed_site` ensures that the data is not accessible to the user-controlled code. let data =Ident ::new ("__data" ,Span ::mixed_site ()); -let init_fields =init_fields ( - &fields, - pinned, - !attrs - .iter () - .any (|attr|matches! (attr,InitializerAttribute ::DisableInitializedFieldAccess )), - &data, - &slot, - ); +let init_fields =init_fields (&fields, pinned, &data, &slot);let field_check =make_field_check (&fields, init_kind, &path);Ok (quote! {{// We do not want to allow arbitrary returns, so we declare this type as the `Ok` return
@@ -236,7 +228,6 @@ fn get_init_kind(rest: Option<(Token![..], Expr)>, dcx: &mut DiagCtxt) -> InitKifn init_fields ( fields: &Punctuated <InitializerField ,Token! [,]>, pinned:bool , - generate_initialized_accessors:bool , data: &Ident , slot: &Ident , ) ->TokenStream {
@@ -272,13 +263,6 @@ fn init_fields( unsafe { &mut (*#slot).#ident } } }; -let accessor = generate_initialized_accessors.then (|| { -quote! { - #(#cfgs)* -#[allow(unused_variables)] -let #ident = #accessor; - } - });quote! { #(#attrs)* {
@@ -286,7 +270,9 @@ fn init_fields(// SAFETY: TODO unsafe { #write(::core::ptr::addr_of_mut!((*#slot).#ident), #value_ident) }; } - #accessor + #(#cfgs)* +#[allow(unused_variables)] + let #ident = #accessor; } } InitializerKind::Init { ident, value, .. } => {
@@ -326,20 +312,15 @@ fn init_fields( }, ) }; -let accessor = generate_initialized_accessors.then (|| { -quote! { - #(#cfgs)* -#[allow(unused_variables)] -let #ident = #accessor; - } - });quote! { #(#attrs)* {let #init = #value; #value_init } - #accessor + #(#cfgs)* +#[allow(unused_variables)] +let #ident = #accessor; } } InitializerKind::Code { block: value, .. } => quote! {
@@ -466,10 +447,6 @@ fn parse(input: syn::parse::ParseStream<'_>) -> syn::Result<Self> {if a.path ().is_ident ("default_error" ) { a.parse_args ::<DefaultErrorAttribute >() .map (InitializerAttribute ::DefaultError ) - }else if a.path ().is_ident ("disable_initialized_field_access" ) { - a.meta - .require_path_only () - .map (|_|InitializerAttribute ::DisableInitializedFieldAccess ) }else {Err (syn::Error ::new_spanned (a,"unknown initializer attribute" )) }
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
Signature · 1 line
2.53.0
Benno Lossin
maintainerPATCH v2 2/2 rust: pin-init: internal: init: document load-bearing fact of field accessors
The functions [Pin]Init::__[pinned_]init and ptr::write called from the init! macro require the passed pointer to be aligned. This fact is ensured by the creation of field accessors to previously initialized fields.
Since we missed this very important fact from the beginning [1], document it in the code.
---
As already explained in the previous email, we discovered an unsoundness in pin-init that exists since the beginning, but was unknowingly fixed in commit 42415d163e5d ("rust: pin-init: add references to previously initialized fields").
We introduced pin-init in 90e53c5e70a6 ("rust: add pin-init API core"), which was included in 6.4. The affected stable trees that are still maintained are: 6.12 and 6.6. Note that 6.18 and 6.19 already contain 42415d163e5d, so they are unaffected.
We still should backport this piece of documentation explaining the need for the field accessors for soundness. For this reasons we also want to backport it to 6.18 and 6.19.
Note that this patch depends on 42415d163e5d; so the only versions this patch can go in directly are 6.18 and 6.19. I will send separate patch series' for the older versions. The series' will include a backport of 42415d163e5d as well as a modified version of this patch, since this patch depends on the syn rewrite, which is not present in older versions.
--- rust/pin-init/internal/src/init.rs | 8 ++++++++ 1 file changed, 8 insertions(+)
rust/pin-init/internal/src/init.rs +8 -0
@@ -251,6 +251,10 @@ fn init_fields( });// Again span for better diagnostics let write =quote_spanned! (ident.span ()=> ::core::ptr::write); +// NOTE: the field accessor ensures that the initialized field is properly aligned. +// Unaligned fields will cause the compiler to emit E0793. We do not support +// unaligned fields since `Init::__init` requires an aligned pointer; the call to +// `ptr::write` below has the same requirement. let accessor =if pinned {let project_ident =format_ident! ("__project_{ident}" );quote! {
@@ -278,6 +282,10 @@ fn init_fields( InitializerKind::Init { ident, value, .. } => { // Again span for better diagnostics let init = format_ident!("init", span = value.span()); + // NOTE: the field accessor ensures that the initialized field is properly aligned. + // Unaligned fields will cause the compiler to emit E0793. We do not support + // unaligned fields since `Init::__init` requires an aligned pointer; the call to + // `ptr::write` below has the same requirement. let (value_init, accessor) = if pinned { let project_ident = format_ident!("__project_{ident}"); (
Signature · 1 line
2.53.0
Re: [PATCH v2 2/2] rust: pin-init: internal: init: document load-bearing fact of field accessors
Quoted 5 lines · Benno Lossin
The functions
[Pin]Init::__[pinned_]initandptr::writecalled from theinit!macro require the passed pointer to be aligned. This fact is ensured by the creation of field accessors to previously initialized fields.
Quoted 7 lines
Since we missed this very important fact from the beginning [1], document it in the code.
Fixes 90e53c5e70a6 ("rust: add pin-init API core")Cc [email protected] # 6.19.y and 6.18.y: patch should apply without issuesCc [email protected] # 6.12.y and 6.6.y: need prerequisite see below `---` for more info
Hmm, if this patch is applied as is, the --- below is going to be cut out and this line wouldn't make sense.
Perhaps we should just put
Or leave this cc out and ask for manual picking?
Quoted 1 line
Signed-off-by Benno Lossin <[email protected]>
Best,
Gary
Quoted 24 lines
---
As already explained in the previous email, we discovered an unsoundness in pin-init that exists since the beginning, but was unknowingly fixed in commit42415d163e5d("rust: pin-init: add references to previously initialized fields").We introduced pin-init in
90e53c5e70a6("rust: add pin-init API core"), which was included in 6.4. The affected stable trees that are still maintained are: 6.12 and 6.6. Note that 6.18 and 6.19 already contain42415d163e5d, so they are unaffected.We still should backport this piece of documentation explaining the need for the field accessors for soundness. For this reasons we also want to backport it to 6.18 and 6.19.
Note that this patch depends on
42415d163e5d; so the only versions this patch can go in directly are 6.18 and 6.19. I will send separate patch series' for the older versions. The series' will include a backport of42415d163e5das well as a modified version of this patch, since this patch depends on thesynrewrite, which is not present in older versions.--- rust/pin-init/internal/src/init.rs | 8 ++++++++ 1 file changed, 8 insertions(+)
Re: [PATCH v2 2/2] rust: pin-init: internal: init: document load-bearing fact of field accessors
Quoted 2 lines · Gary Guo
Cc [email protected] # 6.12.y and 6.6.y: need commit 42415d163e5d ("rust: pin-init: add references to previously initialized fields")
Yeah, something like that is what I would have probably written. The docs seem to suggest a format like this:
Cc: <[email protected]> # 6.6.y, 6.12.y: 42415d163e5d: rust:
pin-init: add references to previously initialized fields
Cc: <[email protected]> # 6.6.y, 6.12.y, 6.18.y, 6.19.y
i.e. first the prerequisite, then a line without it to indicate "this commit".
Cheers,
Miguel
Benno Lossin
maintainerRe: [PATCH v2 2/2] rust: pin-init: internal: init: document load-bearing fact of field accessors
Quoted 4 lines · Miguel Ojeda
Quoted 2 lines · Gary Guo
Cc [email protected] # 6.12.y and 6.6.y: need commit 42415d163e5d ("rust: pin-init: add references to previously initialized fields")
Quoted 8 lines
Yeah, something like that is what I would have probably written. The docs seem to suggest a format like this:
Cc: <[email protected]> # 6.6.y, 6.12.y:
42415d163e5d: rust:
pin-init: add references to previously initialized fields
Cc: <[email protected]> # 6.6.y, 6.12.y, 6.18.y, 6.19.yi.e. first the prerequisite, then a line without it to indicate "this commit".
Yeah I saw that in the docs as well, but I thought that since the cherry-pick wouldn't succeed (due to the syn rewrite). However, I wrote that 6.18.y and 6.19.y applying the patch would succeed, but that's also not true, there we also don't have the syn rewrite...
The two Cc lines you gave seem like the correct thing :)
So when you pick them, change the Cc's to that (unless I need a new version of course).
Cheers,
Benno
Quoted 17 lines · Benno Lossin
Gary noticed [1] that the initializer macros as well as the
[Pin]Inittraits cannot support unaligned fields, since they use operations that require aligned pointers. This means that any code using structs with unaligned fields in pin-init is unsound.By default, the
init!macro generates references to initialized fields, which makes the compiler check that those fields are aligned. However, we added the#[disable_initialized_field_access]attribute to avoid this behavior inceca298c53f9("rust: pin-init: internal: init:
add escape hatch for referencing initialized fields"). Thus remove the#[disable_initialized_field_access]attribute frominit!, which is the only safe way to create an initializer handling unaligned fields.If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
Quoted 5 lines
Reported-by Gary Guo <[email protected]>Fixes ceca298c53f9 ("rust: pin-init: internal: init: add escape hatch for referencing initialized fields")Signed-off-by Benno Lossin <[email protected]>
Quoted 33 lines
---
Changelog:
* changes since v1: https://lore.kernel.org/all/[email protected]
- improved note added to the code in patch 2
- improved commit messages
---
This commit does not need backporting, asceca298c53f9is not yet in any stable tree.However, the unsoundness still affects two stable trees, because it was unknowingly fixed in commit
42415d163e5d("rust: pin-init: add references to previously initialized fields"). Before then, packed structs compiled without any issues with pin-init and thus all prior kernel versions with pin-init that do not contain that commit are affected.We introduced pin-init in
90e53c5e70a6("rust: add pin-init API core"), which was included in 6.4. The affected stable trees that are still maintained are: 6.12 and 6.6. Note that 6.18 and 6.19 already contain42415d163e5d, so they are unaffected.I will prepare a separate patch series to backport
42415d163e5dto each of the affected trees, including the second patch of this series that documents the fact that field accessors are load-bearing for soundness.Janne Grunau has worked around [3] this problem in the AOP audio driver downstream, which was the reason for adding the
#[disable_initialized_field_access]attribute in the first place.[3]: https://lore.kernel.org/all/[email protected]
--- rust/pin-init/internal/src/init.rs | 39 ++++++------------------------ 1 file changed, 8 insertions(+), 31 deletions(-)
Quoted 17 lines · Benno Lossin
Gary noticed [1] that the initializer macros as well as the
[Pin]Inittraits cannot support unaligned fields, since they use operations that require aligned pointers. This means that any code using structs with unaligned fields in pin-init is unsound.By default, the
init!macro generates references to initialized fields, which makes the compiler check that those fields are aligned. However, we added the#[disable_initialized_field_access]attribute to avoid this behavior inceca298c53f9("rust: pin-init: internal: init:
add escape hatch for referencing initialized fields"). Thus remove the#[disable_initialized_field_access]attribute frominit!, which is the only safe way to create an initializer handling unaligned fields.If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
Quoted 5 lines
Reported-by Gary Guo <[email protected]>Fixes ceca298c53f9 ("rust: pin-init: internal: init: add escape hatch for referencing initialized fields")Signed-off-by Benno Lossin <[email protected]>
Janne
Quoted 9 lines · Benno Lossin
Gary noticed [1] that the initializer macros as well as the
[Pin]Inittraits cannot support unaligned fields, since they use operations that require aligned pointers. This means that any code using structs with unaligned fields in pin-init is unsound.By default, the
init!macro generates references to initialized fields, which makes the compiler check that those fields are aligned. However, we added the#[disable_initialized_field_access]attribute to avoid this behavior inceca298c53f9("rust: pin-init: internal: init:
checkpatch here: missing 'commit' before ceca298c53f9
Quoted 10 lines
add escape hatch for referencing initialized fields"). Thus remove the
#[disable_initialized_field_access]attribute frominit!, which is the only safe way to create an initializer handling unaligned fields.If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
Should probably be Closes: rather than Link:.
Quoted 3 lines
Fixes ceca298c53f9 ("rust: pin-init: internal: init: add escape hatch for referencing initialized fields")Signed-off-by Benno Lossin <[email protected]>
change itself LGTM
Benno Lossin
maintainerQuoted 6 lines · Alice Ryhl
Quoted 5 lines · Benno Lossin
Gary noticed [1] that the initializer macros as well as the
[Pin]Inittraits cannot support unaligned fields, since they use operations that require aligned pointers. This means that any code using structs with unaligned fields in pin-init is unsound.
Quoted 6 lines
Quoted 4 lines
By default, the
init!macro generates references to initialized fields, which makes the compiler check that those fields are aligned. However, we added the#[disable_initialized_field_access]attribute to avoid this behavior inceca298c53f9("rust: pin-init: internal: init:checkpatch here: missing 'commit' before
ceca298c53f9
I'll let Miguel adjust that when picking the patch :)
Quoted 4 lines
Quoted 4 lines
add escape hatch for referencing initialized fields"). Thus remove the
#[disable_initialized_field_access]attribute frominit!, which is the only safe way to create an initializer handling unaligned fields.
Quoted 8 lines
Quoted 6 lines
If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
Should probably be Closes: rather than Link:.
I figured that we can't really "close" a Zulip topic (we can mark it as completed, but that topic was originally about the field accessors, so not sure if this makes much sense.
Cheers,
Benno
Alice Ryhl
reviewerQuoted 8 lines · Benno Lossin
Quoted 8 lines
Quoted 8 lines
add escape hatch for referencing initialized fields"). Thus remove the
#[disable_initialized_field_access]attribute frominit!, which is the only safe way to create an initializer handling unaligned fields.If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Should probably be Closes: rather than Link:.
I figured that we can't really "close" a Zulip topic (we can mark it as completed, but that topic was originally about the field accessors, so not sure if this makes much sense.
I mean, the Reported-by / Closes pair is most commonly used with lore links to syzbot reports, which isn't much different.
Alice
Quoted 4 lines · Benno Lossin
I figured that we can't really "close" a Zulip topic (we can mark it as completed, but that topic was originally about the field accessors, so not sure if this makes much sense.
Using Link: instead of Closes: is supposed to mean that only part of the issue is fixed, i.e. whether you can "technically close" it or not is not important (e.g. Closes: to an archive like lore.kernel.org are very common and actually what the docs mention as the first use case).
So I would say Closes: should be the "default", and Link: is for particular cases where an actual issue (or message) carries several topics and could be confusing to have several commits with the exact same Closes: line.
In the case of Zulip, we can link to particular messages in a single thread, so typically it is possible to use Closes:, though the current UI isn't great to see where exactly it was linked to... :(
Anyway, I guess the inspiration was the similar feature from forges, and perhaps in case someone has a custom tracking system that actually auto-closes things in a forge, but it is a bit confusing. Having allowed "# ..." for Closes: or a similar Closes(foo): instead of reusing Link: may have been better.
I hope that helps!
Cheers,
Miguel
Benno Lossin
maintainerQuoted 20 lines · Miguel Ojeda
Quoted 4 lines · Benno Lossin
I figured that we can't really "close" a Zulip topic (we can mark it as completed, but that topic was originally about the field accessors, so not sure if this makes much sense.
Using Link: instead of Closes: is supposed to mean that only part of the issue is fixed, i.e. whether you can "technically close" it or not is not important (e.g. Closes: to an archive like lore.kernel.org are very common and actually what the docs mention as the first use case).
So I would say Closes: should be the "default", and Link: is for particular cases where an actual issue (or message) carries several topics and could be confusing to have several commits with the exact same Closes: line.
In the case of Zulip, we can link to particular messages in a single thread, so typically it is possible to use Closes:, though the current UI isn't great to see where exactly it was linked to... :(
Quoted 7 lines
Anyway, I guess the inspiration was the similar feature from forges, and perhaps in case someone has a custom tracking system that actually auto-closes things in a forge, but it is a bit confusing. Having allowed "# ..." for Closes: or a similar Closes(foo): instead of reusing Link: may have been better.
I hope that helps!
Yeah that explains it, thanks!
In that case you can make it a closes tag when picking.
Cheers,
Benno
Miguel Ojeda
AppliedQuoted 18 lines · Benno Lossin
Gary noticed [1] that the initializer macros as well as the
[Pin]Inittraits cannot support unaligned fields, since they use operations that require aligned pointers. This means that any code using structs with unaligned fields in pin-init is unsound.By default, the
init!macro generates references to initialized fields, which makes the compiler check that those fields are aligned. However, we added the#[disable_initialized_field_access]attribute to avoid this behavior inceca298c53f9("rust: pin-init: internal: init:
add escape hatch for referencing initialized fields"). Thus remove the#[disable_initialized_field_access]attribute frominit!, which is the only safe way to create an initializer handling unaligned fields.If support for in-place initializing structs with unaligned fields is required in the future, we could figure out a solution. This is tracked in [2].
Quoted 5 lines
Reported-by Gary Guo <[email protected]>Fixes ceca298c53f9 ("rust: pin-init: internal: init: add escape hatch for referencing initialized fields")Signed-off-by Benno Lossin <[email protected]>
Applied to rust-fixes -- thanks everyone!
[ Adjusted tags and reworded as discussed. - Miguel ]
[ Updated Cc: stable@ tags as discussed. - Miguel ]
Cheers,
Miguel