Skip to contentSkip to conversation

rust: pin-init: internal: init: remove `#[disable_initialized_field_access]`

PATCH Applied Benno Lossin 5 participants 13 messages Mar 2 – Mar 6

Subsystems (3)
lore
Share & tools
lore

Benno Lossin

author maintainer
#0 lore raw

PATCH 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
index 42936f915a07..da53adc44ecf 100644
@@ -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) -> InitKi
 fn 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
author maintainer
#1 lore raw

PATCH 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.

Fixes 90e53c5e70a6 ("rust: add pin-init API core")
Cc [email protected] # 6.19.y and 6.18.y: patch should apply without issues
Cc [email protected] # 6.12.y and 6.6.y: need prerequisite see below `---` for more info

---
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
index da53adc44ecf..738f62c8105c 100644
@@ -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
maintainer Reviewed-by on v2 2/2
#2 lore raw

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_]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.

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 issues
Cc [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

Cc [email protected] # 6.12.y and 6.6.y: need commit 42415d163e5d ("rust: pin-init: add references to previously initialized fields")

Or leave this cc out and ask for manual picking?

Quoted 1 line

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 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(+)
#3 lore raw

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

author maintainer
#4 lore raw

Re: [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.y

i.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

maintainer Reviewed-by on v2 1/2
#5 lore raw
In reply to Benno Lossin
Quoted 17 lines · Benno Lossin

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

Quoted 5 lines
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, 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(-)
Acked-by on v2 1/2
#6 lore raw
In reply to Benno Lossin
Quoted 17 lines · Benno Lossin

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

Quoted 5 lines

Janne

reviewer Reviewed-by on v2 1/2
#7 lore raw
In reply to Benno Lossin
Quoted 9 lines · Benno Lossin

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:

checkpatch here: missing 'commit' before ceca298c53f9

Quoted 10 lines

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

Should probably be Closes: rather than Link:.

Quoted 3 lines
Fixes ceca298c53f9 ("rust: pin-init: internal: init: add escape hatch for referencing initialized fields")

change itself LGTM

author maintainer
#8 lore raw
Quoted 6 lines · Alice Ryhl
Quoted 5 lines · Benno Lossin

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.

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 in ceca298c53f9 ("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 from init!, 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

reviewer
#9 lore raw
Quoted 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 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].

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

#10 lore raw
In reply to Benno Lossin
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

author maintainer
#11 lore raw
Quoted 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

Applied
#12 lore raw
In reply to Benno Lossin
Quoted 18 lines · Benno Lossin

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

Quoted 5 lines

Applied to rust-fixes -- thanks everyone!

[ Adjusted tags and reworded as discussed. - Miguel ]

[ Updated Cc: stable@ tags as discussed. - Miguel ]

Cheers,
Miguel