Re: [PATCH v3] rust: configfs: fix object initialization cleanup

[Andreas Hindborg <a.hindborg@kernel.org>]

Younes Akhouayri via B4 Relay <devnull+git.younes.io@kernel.org> writes:

> From: Younes Akhouayri <git@younes.io>
>
> Subsystem::new() calls configfs_register_subsystem() at the end of its
> pin initializer. If registration returns an error, release the config
> item's initial reference and destroy the initialized mutex before
> returning. Otherwise, long subsystem names allocated by
> config_item_set_name() leak.
>
> The initial reference also remains after a successful subsystem is
> unregistered. Release it from PinnedDrop before the Rust container is
> destroyed.
>
> Initialize driver data before the C configfs object in Subsystem::new()
> and Group::new(). Then failure while initializing driver data cannot
> leave an initialized config group, and its allocated name, behind.
>
> Keeping registration inside try_pin_init! also means PinnedDrop is
> installed only after registration succeeds. A duplicate name therefore
> returns -EEXIST without attempting to unregister a subsystem whose
> ci_dentry was never set.
>
> Fixes: 446cafc295bf ("rust: configfs: introduce rust support for configfs")
> Signed-off-by: Younes Akhouayri <git@younes.io>
> ---
> Changes in v3:
> - Initialize driver data before configfs groups.
> - Release the initial group reference after registration failure and unregister.
> - Link to v2: https://patch.msgid.link/20260818-fix-rust-configfs-registration-state-v1-v2-1-9acedd3070f7@younes.io
>
> Changes in v2:
> - Register the subsystem at the end of try_pin_init!.
> - Destroy su_mutex when registration fails.
> - Remove the registered flag.
> - Link to v1: https://patch.msgid.link/20260818-fix-rust-configfs-registration-state-v1-v1-1-c929990bc8ef@younes.io
>
> To: Andreas Hindborg <a.hindborg@kernel.org>
> To: Breno Leitao <leitao@debian.org>
> To: Miguel Ojeda <ojeda@kernel.org>
> To: Boqun Feng <boqun@kernel.org>
> To: Gary Guo <gary@garyguo.net>
> To: Björn Roy Baron <bjorn3_gh@protonmail.com>
> To: Benno Lossin <lossin@kernel.org>
> To: Alice Ryhl <aliceryhl@google.com>
> To: Trevor Gross <tmgross@umich.edu>
> To: Danilo Krummrich <dakr@kernel.org>
> To: Daniel Almeida <daniel.almeida@collabora.com>
> To: Tamir Duberstein <tamird@kernel.org>
> To: Alexandre Courbot <acourbot@nvidia.com>
> To: Onur Özkan <work@onurozkan.dev>
> Cc: rust-for-linux@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  rust/kernel/configfs.rs | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
> index cd082b83e9e7..d28ac9a648f1 100644
> --- a/rust/kernel/configfs.rs
> +++ b/rust/kernel/configfs.rs
> @@ -150,6 +150,7 @@ pub fn new(
>          data: impl PinInit<Data, Error>,
>      ) -> impl PinInit<Self, Error> {
>          try_pin_init!(Self {
> +            data <- data,
>              subsystem <- pin_init::init_zeroed().chain(
>                  |place: &mut Opaque<bindings::configfs_subsystem>| {
>                      // SAFETY: We initialized the required fields of `place.group` above.
> @@ -172,13 +173,23 @@ pub fn new(
>                      Ok(())
>                  }
>              ),
> -            data <- data,
> -        })
> -        .pin_chain(|this| {
> -            crate::error::to_result(
> -                // SAFETY: We initialized `this.subsystem` according to C API contract above.
> -                unsafe { bindings::configfs_register_subsystem(this.subsystem.get()) },
> -            )
> +            _: {
> +                let result = crate::error::to_result(
> +                    // SAFETY: We initialized `subsystem` according to the C API contract above.
> +                    unsafe { bindings::configfs_register_subsystem(subsystem.get()) },
> +                );
> +                if result.is_err() {
> +                    // SAFETY: The group and mutex were initialized above, and registration
> +                    // failed, so configfs does not hold references to the group.
> +                    unsafe {
> +                        bindings::config_item_put(

I think this should be `config_group_put`, although it does make a
difference in practice right now.

> +                            &raw mut (*subsystem.get()).su_group.cg_item,
> +                        );
> +                        bindings::mutex_destroy(&raw mut (*subsystem.get()).su_mutex);
> +                    }
> +                }
> +                result?
> +            }
>          })
>      }
>  }
> @@ -188,8 +199,12 @@ impl<Data> PinnedDrop for Subsystem<Data> {
>      fn drop(self: Pin<&mut Self>) {
>          // SAFETY: We registered `self.subsystem` in the initializer returned by `Self::new`.
>          unsafe { bindings::configfs_unregister_subsystem(self.subsystem.get()) };
> -        // SAFETY: We initialized the mutex in `Subsystem::new`.
> -        unsafe { bindings::mutex_destroy(&raw mut (*self.subsystem.get()).su_mutex) };
> +        // SAFETY: Unregistering drops configfs's references to the group, so it is safe to drop
> +        // the initial group reference and destroy the initialized mutex.
> +        unsafe {
> +            bindings::config_item_put(&raw mut (*self.subsystem.get()).su_group.cg_item);

Similar, should be `config_group_put`.

> +            bindings::mutex_destroy(&raw mut (*self.subsystem.get()).su_mutex);
> +        }
>      }
>  }
>  
> @@ -260,6 +275,7 @@ pub fn new(
>          data: impl PinInit<Data, Error>,
>      ) -> impl PinInit<Self, Error> {
>          try_pin_init!(Self {
> +            data <- data,
>              group <- pin_init::init_zeroed().chain(|v: &mut Opaque<bindings::config_group>| {
>                  let place = v.get();
>                  let name = name.to_bytes_with_nul().as_ptr();
> @@ -269,7 +285,6 @@ pub fn new(
>                  };
>                  Ok(())
>              }),
> -            data <- data,

Without adding a `PinnedDrop` implementation, this change has no effect.
We should add `PinnedDrop` that does `config_group_put` on `group`,
right?

We could also consider adding `SubsystemInner` and `GroupInner` and
implement `PinnedDrop` on those, to handle the C interface. We would
then have fields of type `SubsystemInner` in `Subsystem` instead of
`Opaque<_>` and similar for `Group<_>`. Then we would not have to
consider the initialization ordering with respect to the `data` field of
`Subsystem` and `Group`. What do you think?

Best regards,
Andreas Hindborg