Message-ID: <8734diq7o3.fsf@kernel.org>
Date: Tue, 06 May 2025 13:51:08 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com>
Cc: "Danilo Krummrich" <dakr@...nel.org>,  "Miguel Ojeda"
 <ojeda@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno Lossin" <benno.lossin@...ton.me>,
  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Joel Becker" <jlbec@...lplan.org>,  "Peter Zijlstra"
 <peterz@...radead.org>,  "Ingo Molnar" <mingo@...hat.com>,  "Will Deacon"
 <will@...nel.org>,  "Waiman Long" <longman@...hat.com>,  "Fiona Behrens"
 <me@...enk.dev>,  "Charalampos Mitrodimas" <charmitro@...teo.net>,
  "Daniel Almeida" <daniel.almeida@...labora.com>,  "Breno Leitao"
 <leitao@...ian.org>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 1/3] rust: configfs: introduce rust support for configfs

Andreas Hindborg <a.hindborg@...nel.org> writes:

> "Miguel Ojeda" <miguel.ojeda.sandonis@...il.com> writes:
>
>> On Mon, May 5, 2025 at 9:51 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>>>
>>> So I was thinking that because I am initializing a static with a let
>>> statement, it would run in const context. But I see that it is not
>>> actually guaranteed.
>>
>> No, that is actually guaranteed, i.e. when initializing a static. But
>> you aren't initializing a static here, no? Which static are you
>> referring to? If you were, then the "normal" `assert!` would work,
>> because it would be a const context.
>>
>> The `add` calls I see are just in the `let` statement, not
>> initializing any static:
>>
>>             {
>>                 const N: usize = 0usize;
>>                 unsafe { CONFIGURATION_ATTRS.add::<N, 0,
>> _>(&CONFIGURATION_MESSAGE_ATTR) };
>>             }
>>
>> So it also means this comment is wrong:
>>
>> +        // SAFETY: This function is only called through the `configfs_attrs`
>> +        // macro. This ensures that we are evaluating the function in const
>> +        // context when initializing a static. As such, the reference created
>> +        // below will be exclusive.
>>
>> Please double-check all this... :)
>
> Oops.
>
>>
>>> Right. Which is why I opted for `build_error`. But with the `const`
>>> block solution you suggested is better.
>>
>> I thought you opted for that because you thought the `assert!` would
>> only work if not refactored. What I tried to point out was that the
>> `assert!` wouldn't have worked even before the refactoring.
>
> I made a mistake in thinking this was in const context. I'll see if I
> can fix that.


I think I can fix it like this:


modified   rust/kernel/configfs.rs
@@ -703,8 +703,8 @@ impl<const N: usize, Data> AttributeList<N, Data> {
 
     /// # Safety
     ///
-    /// This function can only be called by the [`kernel::configfs_attrs`]
-    /// macro.
+    /// The caller must ensure that there are no other concurrent accesses to
+    /// `self`. That is, the caller has exclusive access to `self.`
     #[doc(hidden)]
     pub const unsafe fn add<const I: usize, const ID: u64, O>(
         &'static self,
@@ -715,10 +715,8 @@ impl<const N: usize, Data> AttributeList<N, Data> {
         // We need a space at the end of our list for a null terminator.
         const { assert!(I < N - 1, "Invalid attribute index") };
 
-        // SAFETY: This function is only called through the
-        // [kernel::`configfs_attrs`] macro. This ensures that we are evaluating
-        // the function in const context when initializing a static. As such,
-        // the reference created below will be exclusive.
+        // SAFETY: By function safety requirements, we have exclusive access to
+        // `self` and the reference created below will be exclusive.
         unsafe {
             (&mut *self.0.get())[I] = (attribute as *const Attribute<ID, O, Data>)
                 .cast_mut()
@@ -955,7 +953,12 @@ macro_rules! configfs_attrs {
             @assign($($assign)* {
                 const N: usize = $cnt;
                 // The following macro text expands to a call to `Attribute::add`.
-                // SAFETY: We are expanding [`kernel::configfs_attrs`].
+
+                // SAFETY: By design of this macro, the name of the variable we
+                // invoke the `add` method on below, is not visible outside of
+                // the macro expansion. The macro does not operate concurrently
+                // on this variable, and thus we have exclusive access to the
+                // variable.
                 unsafe {
                     $crate::macros::paste!(
                         [< $data:upper _ATTRS >]


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists