linux-kernel - Re: [PATCH 3/4] rust: configfs: introduce rust support for configfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87frkrjobu.fsf@kernel.org>
Date: Thu, 06 Feb 2025 12:37:57 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Fiona Behrens" <me@...enk.dev>
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>,  "Christoph Hellwig" <hch@....de>,
  <rust-for-linux@...r.kernel.org>,  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] rust: configfs: introduce rust support for configfs

"Fiona Behrens" <me@...enk.dev> writes:

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

[cut]

>> +/// A `configfs` subsystem.
>> +///
>> +/// This is the top level entrypoint for a `configfs` hierarchy. Embed a field
>> +/// of this type into a struct and implement [`HasSubsystem`] for the struct
>> +/// with the [`kernel::impl_has_subsystem`] macro. Instantiate the subsystem with
>> +/// [`Subsystem::register`].
>> +///
>> +/// A [`Subsystem`] is also a [`Group`], and implementing [`HasSubsystem`] for a
>> +/// type will automatically implement [`HasGroup`] for the type.
>
> Rustdoc is unhappy about this (and other lines where `HasGroup` is used)
> as `HasGroup` is private and therefore rustdoc cannot resolve this link.

Right, I'll remove this link.

>
>> +#[pin_data(PinnedDrop)]
>> +pub struct Subsystem<DATA> {
>
> I would favour this as PascalCase (so `Subsystem<Data>`), as this is a
> generic type and not a generic const (I always see all uppercase for
> const generics).

Yea, that makes sense. I somehow thought all generics had to be
screaming snake.


[cut]

>> +
>> +#[pinned_drop]
>> +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.subsystemget()) };
>
> I see other users (e.g. gpio-virtuser[0]) to also destroy the mutex, is
> that a required action?
>
> [0]: https://elixir.bootlin.com/linux/v6.13.1/source/drivers/gpio/gpio-virtuser.c#L1841

Referring to C null_blk, they do not destroy the mutex when tearing
down. I am not sure what the correct thing to do is. Perhaps lockdep
wants it destroyed? I'll investigate.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/block/null_blk/main.c?h=v6.14-rc1#n2105


[cut]

>> +
>> +impl<PAR, CHLD, CPTR, PCPTR> GroupOperationsVTable<PAR, CHLD, CPTR, PCPTR>
>> +where
>> +    PAR: GroupOperations<Child = CHLD, ChildPointer = CPTR, PinChildPointer = PCPTR>,
>> +    CPTR: InPlaceInit<Group<CHLD>, PinnedSelf = PCPTR>,
>> +    PCPTR: ForeignOwnable<PointedTo = Group<CHLD>>,
>> +{
>
> I usualy favour having struct and then impls for the functions direcly
> above each other. Here you have th `get_group_data` function between
> that, maybe it makes sense to move that function either further up or
> down.

I agree, I'll move it 👍
>
> So far this is all that I did direcly see.

Thanks!


Best regards,
Andreas Hindborg