[RFC PATCH 0/3] rust abstractions for interacting with sysfs

[RFC PATCH 0/3] rust abstractions for interacting with sysfs

[Daniel Sedlak]

Hello,
For now, there are no facilities for the Rust side to tune modules written
in Rust in runtime or for just simple debug output/statistics from
the module. For exactly this purpose, the C has the sysfs. In this
patch series, I implemented a minimal sysfs wrapper utilizing kobject API.

I would like to collect your thoughts on whether:
  - Do we want something like this in the rust part of the kernel?
  - Do we want to support all sysfs capabilities?

I implemented a sample to present the API I have chosen for the Rust part.
It is a straightforward example that does not represent all capabilities
of the sysfs. The documentation will be potentially improved, but I would
like to first settle the abstractions.

Thank you

Daniel

Daniel Sedlak (3):
  rust: kernel: types: add mode wrapper
  rust: kernel: kobject: basic sysfs implementation
  samples: rust: add kobject sample

 rust/kernel/kobject.rs          | 271 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/types.rs            |  17 ++
 samples/rust/Kconfig            |   8 +
 samples/rust/Makefile           |   1 +
 samples/rust/kobject_example.rs |  66 ++++++++
 6 files changed, 365 insertions(+)
 create mode 100644 rust/kernel/kobject.rs
 create mode 100644 samples/rust/kobject_example.rs


base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
-- 
2.47.1

[RFC PATCH 1/3] rust: kernel: types: add mode wrapper

[Daniel Sedlak]

Add rust variant for representing file mode. This is needed
in order to set file permissions.

Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/types.rs | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index ec6457bb3084..35ac2898433a 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -517,3 +517,20 @@ pub enum Either<L, R> {
 /// [`NotThreadSafe`]: type@NotThreadSafe
 #[allow(non_upper_case_globals)]
 pub const NotThreadSafe: NotThreadSafe = PhantomData;
+
+/// Mode represents file permissions.
+///
+/// TODO: add link to header files.
+pub struct Mode(bindings::umode_t);
+
+impl Mode {
+    /// Creates [`Mode`] from the number.
+    pub const fn from_u16(num: u16) -> Self {
+        Self(num)
+    }
+
+    /// Returns [`Mode`] as an number.
+    pub const fn as_u16(&self) -> u16 {
+        self.0
+    }
+}
-- 
2.47.1

Re: [RFC PATCH 1/3] rust: kernel: types: add mode wrapper

[Alice Ryhl]

On Sun, Dec 8, 2024 at 2:15 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>
> Add rust variant for representing file mode. This is needed
> in order to set file permissions.
>
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> ---
>  rust/kernel/types.rs | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index ec6457bb3084..35ac2898433a 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs

Perhaps this should be defined in rust/kernel/fs.rs?

Alice

Re: [RFC PATCH 1/3] rust: kernel: types: add mode wrapper

[Daniel Sedlak]

On 12/9/24 8:21 AM, Alice Ryhl wrote:
> On Sun, Dec 8, 2024 at 2:15 PM Daniel Sedlak <daniel@sedlak.dev> wrote:
>>
>> Add rust variant for representing file mode. This is needed
>> in order to set file permissions.
>>
>> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
>> ---
>>   rust/kernel/types.rs | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index ec6457bb3084..35ac2898433a 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
> 
> Perhaps this should be defined in rust/kernel/fs.rs?

Yes, likely it is a better place. I was somewhat surprised, that we have 
dedicated `kernel/fs.rs` file with all that flags, but none of them 
regarding the permissions, so I thought that it was on purpose missing.

Daniel

[RFC PATCH 2/3] rust: kernel: kobject: basic sysfs implementation

[Daniel Sedlak]

Implement initial support for interacting with sysfs
via kobject API from the Rust world. Rust for now does
not expose any debug interface or interface for tuning
knobs of the kernel module. Sysfs is used for exporting
relevant debugging metrics or it can be used for
tuning module configuration in the runtime (apart from
module parameters).

This patch builds on a prior work [1] by Martin RR, which
is listed in the old patch registry [2]. However the [1]
seems to be only focused on exposing devices to the sysfs,
where this patch tries to be more broad, not only specific
to the devices.

Link: https://github.com/YakoYakoYokuYoku/linux/commits/sysfs-support [1]
Link: https://github.com/tgross35/RFL-patch-registry [2]
Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 rust/kernel/kobject.rs | 271 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |   2 +
 2 files changed, 273 insertions(+)
 create mode 100644 rust/kernel/kobject.rs

diff --git a/rust/kernel/kobject.rs b/rust/kernel/kobject.rs
new file mode 100644
index 000000000000..9fcc026c83db
--- /dev/null
+++ b/rust/kernel/kobject.rs
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0
+// TODO: Add unsafe doc documentation.
+#![allow(clippy::undocumented_unsafe_blocks, clippy::missing_safety_doc)]
+
+//! KObject wrappers.
+//!
+//! TODO: Write more
+
+use bindings::PAGE_SIZE;
+use core::marker::PhantomData;
+use core::marker::PhantomPinned;
+use core::mem;
+use core::ptr;
+use kernel::c_str;
+use kernel::container_of;
+use kernel::error::code::*;
+use kernel::prelude::*;
+use kernel::str::{CStr, CString};
+use kernel::types::Mode;
+use kernel::types::Opaque;
+
+/// Reference to the subsystems from the non Rust world.
+pub mod subsystems {
+    use super::DynamicKObject;
+
+    macro_rules! declare_kobject {
+        ($name:tt, $subsystem_ptr:expr, $($doc:expr),+) => {
+            $(
+            #[doc = $doc]
+            )*
+            pub static $name: DynamicKObject = DynamicKObject {
+                pointer: kernel::types::Opaque::new(
+                    core::ptr::addr_of!($subsystem_ptr).cast_mut(),
+                ),
+            };
+        };
+    }
+
+    declare_kobject!(
+        FIRMWARE_KOBJECT,
+        bindings::firmware_kobj,
+        "Firmware subsystem dynamic KObject."
+    );
+    declare_kobject!(
+        KERNEL_KOBJECT,
+        bindings::kernel_kobj,
+        "Kernel subsystem dynamic KObject."
+    );
+    declare_kobject!(
+        MM_KOBJECT,
+        bindings::mm_kobj,
+        "Memory management subsystem dynamic KObject."
+    );
+    declare_kobject!(
+        POWER_KOBJECT,
+        bindings::power_kobj,
+        "Power subsystem dynamic KObject."
+    );
+
+    // TODO: Add rests of the exported Kobjects.
+}
+
+/// DynamicKObject is same as [`KObject`], however it does not have any state.
+pub struct DynamicKObject {
+    // The bindings export `extern static *mut bindings::kobejct`, however
+    // we need to have double pointer because rustc complains with
+    // `could not evaluate static initializer`.
+    pointer: Opaque<*mut *mut bindings::kobject>,
+}
+
+unsafe impl Send for DynamicKObject {}
+unsafe impl Sync for DynamicKObject {}
+
+/// KObject represent wrapper structure enables interaction with
+/// the sysfs, where the KObject represents directory.
+///
+/// TODO: Add examples?
+pub struct KObject<K> {
+    data: K,
+    pointer: Opaque<bindings::kobject>,
+
+    // Struct must be `!Unpin`, because kobject pointer is self referential.
+    _m: PhantomPinned,
+}
+
+impl<K> Drop for KObject<K> {
+    fn drop(&mut self) {
+        // SAFETY: This KObject holds a valid reference, because it was allocated
+        // through kobject API.
+        unsafe { bindings::kobject_put(self.pointer.get()) }
+    }
+}
+
+#[doc(hidden)]
+struct KObjTypeVTable;
+#[doc(hidden)]
+impl KObjTypeVTable {
+    unsafe extern "C" fn release_callback(kobj: *mut bindings::kobject) {
+        unsafe { bindings::kfree(kobj.cast()) }
+    }
+
+    pub(crate) const KOBJ_TYPE: bindings::kobj_type = bindings::kobj_type {
+        release: Some(Self::release_callback),
+        sysfs_ops: ptr::addr_of!(bindings::kobj_sysfs_ops),
+        ..unsafe { mem::zeroed() }
+    };
+
+    pub(crate) fn kobj_type(&self) -> &'static bindings::kobj_type {
+        &Self::KOBJ_TYPE
+    }
+}
+
+unsafe impl<K> Send for KObject<K> where K: Send {}
+unsafe impl<K> Sync for KObject<K> where K: Sync {}
+
+impl<K> KObject<K> {
+    /// Attaches KObject to the sysfs root.
+    pub fn new_in_root(name: CString, data: K) -> Result<Pin<KBox<Self>>> {
+        Self::new(name, ptr::null_mut(), data)
+    }
+
+    /// Attaches new KObject to the sysfs with specified KObject parent.
+    pub fn new_with_kobject_parent<L>(
+        name: CString,
+        parent: &KObject<L>,
+        data: K,
+    ) -> Result<Pin<KBox<Self>>> {
+        Self::new(name, parent.pointer.get(), data)
+    }
+
+    /// Attaches new KObject to the sysfs with specified **dynamic** KObject parent.
+    pub fn new_with_dynamic_kobject_parent(
+        name: CString,
+        parent: &DynamicKObject,
+        data: K,
+    ) -> Result<Pin<KBox<Self>>> {
+        Self::new(name, unsafe { **parent.pointer.get() }, data)
+    }
+
+    fn new(name: CString, parent: *mut bindings::kobject, data: K) -> Result<Pin<KBox<Self>>> {
+        let this = KBox::pin(
+            Self {
+                pointer: Opaque::new(unsafe { mem::zeroed() }),
+                data,
+                _m: PhantomPinned,
+            },
+            GFP_KERNEL,
+        )?;
+
+        let code = unsafe {
+            bindings::kobject_init_and_add(
+                this.pointer.get(),
+                KObjTypeVTable.kobj_type(),
+                parent,
+                c_str!("%s").as_char_ptr(),
+                name.as_char_ptr(),
+            )
+        };
+        if code < 0 {
+            return Err(Error::from_errno(code));
+        }
+
+        // TODO: Not sure whether we must check for return code of `kobject_uevent`.
+        unsafe {
+            // We are  responsible for sending uevent that kobject was added to the system.
+            bindings::kobject_uevent(this.pointer.get(), bindings::kobject_action_KOBJ_ADD)
+        };
+
+        Ok(this)
+    }
+
+    /// Creates a file for a given KObject.
+    pub fn add_attribute<A: KObjectTextAttribute<K>>(
+        self: &mut Pin<KBox<KObject<K>>>,
+        attribute: A,
+    ) -> Result<()> {
+        let vtable = TextAttributeVTable::<K, A>::new();
+        // TODO: Can we use it later?
+        let _ = attribute;
+
+        let code = unsafe {
+            bindings::sysfs_create_file_ns(
+                self.pointer.get(),
+                ptr::addr_of!(*vtable.attr()) as *const bindings::attribute,
+                ptr::null(),
+            )
+        };
+
+        if code == 0 {
+            Ok(())
+        } else {
+            Err(kernel::error::Error::from_errno(code))
+        }
+    }
+}
+
+/// Attribute represents a file in a directory.
+#[macros::vtable]
+pub trait KObjectTextAttribute<K>
+where
+    Self: Sized,
+{
+    /// File permissions.
+    const MODE: Mode = Mode::from_u16(0o777);
+
+    /// Name of the file in the sysfs.
+    const NAME: &'static CStr;
+
+    /// Gets called when the read is called on the file.
+    fn show(this: &mut K) -> Result<CString> {
+        let _ = this;
+        Err(EIO)
+    }
+
+    /// Gets called when the write is called on the file.
+    fn store(this: &mut K, input: &CStr) -> Result {
+        let _ = this;
+        let _ = input;
+        Err(EIO)
+    }
+}
+
+#[doc(hidden)]
+struct TextAttributeVTable<K, A: KObjectTextAttribute<K>>(PhantomData<(A, K)>);
+#[doc(hidden)]
+impl<K, A: KObjectTextAttribute<K>> TextAttributeVTable<K, A> {
+    #[doc(hidden)]
+    const fn new() -> Self {
+        Self(PhantomData)
+    }
+    unsafe extern "C" fn show_callback(
+        kobj: *mut bindings::kobject,
+        _attr: *mut bindings::kobj_attribute,
+        buf: *mut core::ffi::c_char,
+    ) -> isize {
+        match A::show(
+            &mut unsafe { &mut *container_of!(kobj, KObject<K>, pointer).cast_mut() }.data,
+        ) {
+            Ok(cstring) => unsafe {
+                bindings::sized_strscpy(buf.cast(), cstring.as_char_ptr(), PAGE_SIZE)
+            },
+            Err(error) => error.to_errno() as isize,
+        }
+    }
+    unsafe extern "C" fn store_callback(
+        kobj: *mut bindings::kobject,
+        _attr: *mut bindings::kobj_attribute,
+        buf: *const core::ffi::c_char,
+        count: usize,
+    ) -> isize {
+        match A::store(
+            &mut unsafe { &mut *container_of!(kobj, KObject<K>, pointer).cast_mut() }.data,
+            unsafe { kernel::str::CStr::from_char_ptr(buf) },
+        ) {
+            Ok(()) => count as isize,
+            Err(error) => error.to_errno() as isize,
+        }
+    }
+    pub(crate) const ATTR: bindings::kobj_attribute = bindings::kobj_attribute {
+        attr: bindings::attribute {
+            name: A::NAME.as_char_ptr(),
+            mode: A::MODE.as_u16(),
+        },
+        show: Some(Self::show_callback),
+        store: Some(Self::store_callback),
+    };
+
+    const fn attr(&self) -> &'static bindings::kobj_attribute {
+        &TextAttributeVTable::<K, A>::ATTR
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a3..f2a921ff38b1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -42,6 +42,8 @@
 pub mod init;
 pub mod ioctl;
 pub mod jump_label;
+#[cfg(CONFIG_SYSFS)]
+pub mod kobject;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
 pub mod list;
-- 
2.47.1

Re: [RFC PATCH 2/3] rust: kernel: kobject: basic sysfs implementation

[Greg KH]

On Sun, Dec 08, 2024 at 02:15:44PM +0100, Daniel Sedlak wrote:
> Implement initial support for interacting with sysfs
> via kobject API from the Rust world. Rust for now does
> not expose any debug interface or interface for tuning
> knobs of the kernel module. Sysfs is used for exporting
> relevant debugging metrics or it can be used for
> tuning module configuration in the runtime (apart from
> module parameters).

sysfs is NOT for debugging, it's for system parameters of devices or
other things.

debugfs is for debugging, if you want debugging interfaces, use that
please!

> This patch builds on a prior work [1] by Martin RR, which
> is listed in the old patch registry [2]. However the [1]
> seems to be only focused on exposing devices to the sysfs,
> where this patch tries to be more broad, not only specific
> to the devices.
> 
> Link: https://github.com/YakoYakoYokuYoku/linux/commits/sysfs-support [1]
> Link: https://github.com/tgross35/RFL-patch-registry [2]
> Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
> ---
>  rust/kernel/kobject.rs | 271 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   2 +
>  2 files changed, 273 insertions(+)
>  create mode 100644 rust/kernel/kobject.rs
> 
> diff --git a/rust/kernel/kobject.rs b/rust/kernel/kobject.rs
> new file mode 100644
> index 000000000000..9fcc026c83db
> --- /dev/null
> +++ b/rust/kernel/kobject.rs
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// TODO: Add unsafe doc documentation.
> +#![allow(clippy::undocumented_unsafe_blocks, clippy::missing_safety_doc)]
> +
> +//! KObject wrappers.
> +//!
> +//! TODO: Write more
> +
> +use bindings::PAGE_SIZE;
> +use core::marker::PhantomData;
> +use core::marker::PhantomPinned;
> +use core::mem;
> +use core::ptr;
> +use kernel::c_str;
> +use kernel::container_of;
> +use kernel::error::code::*;
> +use kernel::prelude::*;
> +use kernel::str::{CStr, CString};
> +use kernel::types::Mode;
> +use kernel::types::Opaque;
> +
> +/// Reference to the subsystems from the non Rust world.
> +pub mod subsystems {
> +    use super::DynamicKObject;
> +
> +    macro_rules! declare_kobject {
> +        ($name:tt, $subsystem_ptr:expr, $($doc:expr),+) => {
> +            $(
> +            #[doc = $doc]
> +            )*
> +            pub static $name: DynamicKObject = DynamicKObject {
> +                pointer: kernel::types::Opaque::new(
> +                    core::ptr::addr_of!($subsystem_ptr).cast_mut(),
> +                ),
> +            };
> +        };
> +    }
> +
> +    declare_kobject!(
> +        FIRMWARE_KOBJECT,
> +        bindings::firmware_kobj,
> +        "Firmware subsystem dynamic KObject."
> +    );
> +    declare_kobject!(
> +        KERNEL_KOBJECT,
> +        bindings::kernel_kobj,
> +        "Kernel subsystem dynamic KObject."
> +    );
> +    declare_kobject!(
> +        MM_KOBJECT,
> +        bindings::mm_kobj,
> +        "Memory management subsystem dynamic KObject."
> +    );
> +    declare_kobject!(
> +        POWER_KOBJECT,
> +        bindings::power_kobj,
> +        "Power subsystem dynamic KObject."
> +    );

Please don't export these unless you have a real user.  I doubt rust
code will be placing stuff in the power of mm kobjects (and if so, the
maintainers of those subsystems MUST review that.)  Same for the other
kobjects as well.

> +
> +    // TODO: Add rests of the exported Kobjects.
> +}
> +
> +/// DynamicKObject is same as [`KObject`], however it does not have any state.
> +pub struct DynamicKObject {

Why the funny name, why not just Kobject?

> +    // The bindings export `extern static *mut bindings::kobejct`, however
> +    // we need to have double pointer because rustc complains with
> +    // `could not evaluate static initializer`.
> +    pointer: Opaque<*mut *mut bindings::kobject>,
> +}
> +
> +unsafe impl Send for DynamicKObject {}
> +unsafe impl Sync for DynamicKObject {}

All kobjects should be dynamic, so why is this needed?

Yes, a few are not, but I really really really do not want any rust code
to follow the mistakes of C code where that happens if at all possible.
Let's take the chance to fix the mistakes of our youth when we can.

> +/// KObject represent wrapper structure enables interaction with
> +/// the sysfs, where the KObject represents directory.
> +///
> +/// TODO: Add examples?
> +pub struct KObject<K> {

Why the "O"?

> +    data: K,
> +    pointer: Opaque<bindings::kobject>,
> +
> +    // Struct must be `!Unpin`, because kobject pointer is self referential.
> +    _m: PhantomPinned,
> +}

So that's all?  Where is the kobject_get() call?

> +impl<K> Drop for KObject<K> {
> +    fn drop(&mut self) {
> +        // SAFETY: This KObject holds a valid reference, because it was allocated
> +        // through kobject API.
> +        unsafe { bindings::kobject_put(self.pointer.get()) }
> +    }
> +}
> +
> +#[doc(hidden)]
> +struct KObjTypeVTable;
> +#[doc(hidden)]
> +impl KObjTypeVTable {
> +    unsafe extern "C" fn release_callback(kobj: *mut bindings::kobject) {
> +        unsafe { bindings::kfree(kobj.cast()) }
> +    }
> +
> +    pub(crate) const KOBJ_TYPE: bindings::kobj_type = bindings::kobj_type {
> +        release: Some(Self::release_callback),

Note, release callbacks are REQUIRED for a kobject, can we enforce that
here somehow?

> +        sysfs_ops: ptr::addr_of!(bindings::kobj_sysfs_ops),

Why would you allow access to a kobject's sysfs_ops?  What needs that?

> +        ..unsafe { mem::zeroed() }

What does this do?

> +    };
> +
> +    pub(crate) fn kobj_type(&self) -> &'static bindings::kobj_type {
> +        &Self::KOBJ_TYPE
> +    }
> +}
> +
> +unsafe impl<K> Send for KObject<K> where K: Send {}
> +unsafe impl<K> Sync for KObject<K> where K: Sync {}
> +
> +impl<K> KObject<K> {
> +    /// Attaches KObject to the sysfs root.
> +    pub fn new_in_root(name: CString, data: K) -> Result<Pin<KBox<Self>>> {
> +        Self::new(name, ptr::null_mut(), data)
> +    }

Please never create a kobject in sysfs's root.  Let's let C code only do
that for now.  If this does come up in the future, talk to me and we can
reconsider it.

> +    /// Attaches new KObject to the sysfs with specified KObject parent.
> +    pub fn new_with_kobject_parent<L>(
> +        name: CString,
> +        parent: &KObject<L>,
> +        data: K,
> +    ) -> Result<Pin<KBox<Self>>> {
> +        Self::new(name, parent.pointer.get(), data)
> +    }

Where is the parent dropped?  And these function names are not matching
up with the C api, why not?

> +
> +    /// Attaches new KObject to the sysfs with specified **dynamic** KObject parent.

All kobjects are dynamic.  If not, please point them out to me (hint, I
know a few but we really should fix them.)

> +    pub fn new_with_dynamic_kobject_parent(

You should never know, or care, about the lifetyle/cycle of your parent
kobject, so why need two different functions?

I'm going to stop here, let's see a real example please.  Wrapping a
kobject is only a last-resort for me, I really want to see proper users
of the in-kernel apis we have already before resorting to "raw"
kobjects.

Note, the one big offender right now is filesystems, and I will push to
finally get a "real" filesystem kobject api created so that filesystems
in rust do NOT have to deal with raw kobjects, but can instead use a
correct api instead, which will prevent all of the current problems that
filesystems have when dealing with raw kobjects (see the mailing list
archives for loads of examples of this.)

Also, a final nit, why didn't you cc: the kobject maintainer on all of
this?  You're lucky I'm on the rust mailing list and happened to noice
this series to hopefully save you some time :)

thanks,

greg k-h

Re: [RFC PATCH 2/3] rust: kernel: kobject: basic sysfs implementation

[Daniel Sedlak]

On 12/8/24 2:43 PM, Greg KH wrote:
> On Sun, Dec 08, 2024 at 02:15:44PM +0100, Daniel Sedlak wrote:
>> Implement initial support for interacting with sysfs
>> via kobject API from the Rust world. Rust for now does
>> not expose any debug interface or interface for tuning
>> knobs of the kernel module. Sysfs is used for exporting
>> relevant debugging metrics or it can be used for
>> tuning module configuration in the runtime (apart from
>> module parameters).
> 
> sysfs is NOT for debugging, it's for system parameters of devices or
> other things.
> 
> debugfs is for debugging, if you want debugging interfaces, use that
> please!

Your are right, I used wrong wording. What I meant, is statistics (for 
example: `/sys/class/net/*/statistics`).

>> +
>> +/// Reference to the subsystems from the non Rust world.
>> +pub mod subsystems {
>> +    use super::DynamicKObject;
>> +
>> +    macro_rules! declare_kobject {
>> +        ($name:tt, $subsystem_ptr:expr, $($doc:expr),+) => {
>> +            $(
>> +            #[doc = $doc]
>> +            )*
>> +            pub static $name: DynamicKObject = DynamicKObject {
>> +                pointer: kernel::types::Opaque::new(
>> +                    core::ptr::addr_of!($subsystem_ptr).cast_mut(),
>> +                ),
>> +            };
>> +        };
>> +    }
>> +
>> +    declare_kobject!(
>> +        FIRMWARE_KOBJECT,
>> +        bindings::firmware_kobj,
>> +        "Firmware subsystem dynamic KObject."
>> +    );
>> +    declare_kobject!(
>> +        KERNEL_KOBJECT,
>> +        bindings::kernel_kobj,
>> +        "Kernel subsystem dynamic KObject."
>> +    );
>> +    declare_kobject!(
>> +        MM_KOBJECT,
>> +        bindings::mm_kobj,
>> +        "Memory management subsystem dynamic KObject."
>> +    );
>> +    declare_kobject!(
>> +        POWER_KOBJECT,
>> +        bindings::power_kobj,
>> +        "Power subsystem dynamic KObject."
>> +    );
> 
> Please don't export these unless you have a real user.  I doubt rust
> code will be placing stuff in the power of mm kobjects (and if so, the
> maintainers of those subsystems MUST review that.)  Same for the other
> kobjects as well.

Noted.
> 
>> +
>> +    // TODO: Add rests of the exported Kobjects.
>> +}
>> +
>> +/// DynamicKObject is same as [`KObject`], however it does not have any state.
>> +pub struct DynamicKObject {
> 
> Why the funny name, why not just Kobject?
> 
>> +    // The bindings export `extern static *mut bindings::kobejct`, however
>> +    // we need to have double pointer because rustc complains with
>> +    // `could not evaluate static initializer`.
>> +    pointer: Opaque<*mut *mut bindings::kobject>,
>> +}
>> +
>> +unsafe impl Send for DynamicKObject {}
>> +unsafe impl Sync for DynamicKObject {}
> 
> All kobjects should be dynamic, so why is this needed?
> 
> Yes, a few are not, but I really really really do not want any rust code
> to follow the mistakes of C code where that happens if at all possible.
> Let's take the chance to fix the mistakes of our youth when we can.

I wanted to distinguish between kobjects that have a "local" state and 
those that have "global" state. What I mean by that is that for example 
`samples/kobject/kobject-example.c` is setting only a global variable to 
some "global" state, that is why it is called a dynamic kobject without 
generic parameter. On the other hand, `samples/kobject/kset-example.c` 
defines custom structure which represent "local" state, and that is why 
I called it just kobject with generic parameter representing type of 
that "local" state.
> 
>> +/// KObject represent wrapper structure enables interaction with
>> +/// the sysfs, where the KObject represents directory.
>> +///
>> +/// TODO: Add examples?
>> +pub struct KObject<K> {
> 
> Why the "O"?

I was reading The sysfs Filesystem [1], and there it is called Kernel 
Object, so I though that K(ernel)Object looks better, but I am really 
open to anything.

Link: https://www.kernel.org/doc/ols/2005/ols2005v1-pages-321-334.pdf [1]
> 
>> +    data: K,
>> +    pointer: Opaque<bindings::kobject>,
>> +
>> +    // Struct must be `!Unpin`, because kobject pointer is self referential.
>> +    _m: PhantomPinned,
>> +}
> 
> So that's all?  Where is the kobject_get() call?

For now, I did not implement cloning for the KObject. I do have only 
single owned "copy" for which the counter should be incremented in 
`kobject_init_and_add` so I should not need it. Or Am I overlooking 
something?
> 
>> +impl<K> Drop for KObject<K> {
>> +    fn drop(&mut self) {
>> +        // SAFETY: This KObject holds a valid reference, because it was allocated
>> +        // through kobject API.
>> +        unsafe { bindings::kobject_put(self.pointer.get()) }
>> +    }
>> +}
>> +
>> +#[doc(hidden)]
>> +struct KObjTypeVTable;
>> +#[doc(hidden)]
>> +impl KObjTypeVTable {
>> +    unsafe extern "C" fn release_callback(kobj: *mut bindings::kobject) {
>> +        unsafe { bindings::kfree(kobj.cast()) }
>> +    }
>> +
>> +    pub(crate) const KOBJ_TYPE: bindings::kobj_type = bindings::kobj_type {
>> +        release: Some(Self::release_callback),
> 
> Note, release callbacks are REQUIRED for a kobject, can we enforce that
> here somehow?

Yes, it is enforced by the trait `KObjectTextAttribute`, where is 
default implementation for `show()` and `store()`, if user does not 
provide them. If user does not implement the callback then the default 
implementation just return `EIO` for that callback.
> 
>> +        sysfs_ops: ptr::addr_of!(bindings::kobj_sysfs_ops),
> 
> Why would you allow access to a kobject's sysfs_ops?  What needs that?

I wanted to reuse already existing facilities and I did not find the 
need of re implementing that and duplicate that code in Rust.
> 
>> +        ..unsafe { mem::zeroed() }
> 
> What does this do?
> 
This could be implemented more gently, possibly without the `unsafe`, 
however the outcome would be the same. I am initializing the structure 
`release` and `sysfs_ops` pointers and initializing the rest to 0 
effectively setting that to NULL pointers.

>> +
>> +impl<K> KObject<K> {
>> +    /// Attaches KObject to the sysfs root.
>> +    pub fn new_in_root(name: CString, data: K) -> Result<Pin<KBox<Self>>> {
>> +        Self::new(name, ptr::null_mut(), data)
>> +    }
> 
> Please never create a kobject in sysfs's root.  Let's let C code only do
> that for now.  If this does come up in the future, talk to me and we can
> reconsider it.
> 
Noted.
>> +    /// Attaches new KObject to the sysfs with specified KObject parent.
>> +    pub fn new_with_kobject_parent<L>(
>> +        name: CString,
>> +        parent: &KObject<L>,
>> +        data: K,
>> +    ) -> Result<Pin<KBox<Self>>> {
>> +        Self::new(name, parent.pointer.get(), data)
>> +    }
> 
> Where is the parent dropped?  And these function names are not matching
> up with the C api, why not?
> 
A module using this API has an ownership of KObject (possibly the parent 
as well as the returned, newly created, KObject), when the KObject is 
dropped by the module, then the destructor is called for that KObject 
releasing resources.

As for the naming. I am not sure whether we are able (or event want) to 
support all capabilities of the sysfs in Rust and I am unsure whether we 
are able to model the API capabilities **with safety guarantees**, so I 
am more in favor of giving it more Rust idiomatic names.
>> +
>> +    /// Attaches new KObject to the sysfs with specified **dynamic** KObject parent.
> 
> All kobjects are dynamic.  If not, please point them out to me (hint, I
> know a few but we really should fix them.)
> 
>> +    pub fn new_with_dynamic_kobject_parent(
> 
> You should never know, or care, about the lifetyle/cycle of your parent
> kobject, so why need two different functions?
> 
> I'm going to stop here, let's see a real example please.  Wrapping a
> kobject is only a last-resort for me, I really want to see proper users
> of the in-kernel apis we have already before resorting to "raw"
> kobjects.
> 
> Note, the one big offender right now is filesystems, and I will push to
> finally get a "real" filesystem kobject api created so that filesystems
> in rust do NOT have to deal with raw kobjects, but can instead use a
> correct api instead, which will prevent all of the current problems that
> filesystems have when dealing with raw kobjects (see the mailing list
> archives for loads of examples of this.)

Thank you, for highlighting that, I will check that out.
> 
> Also, a final nit, why didn't you cc: the kobject maintainer on all of
> this?  You're lucky I'm on the rust mailing list and happened to noice
> this series to hopefully save you some time :)

I am sorry, still getting familiar with the mailing list process, and I 
just forgot.

Thank you for your valuable feedback

Daniel

Re: [RFC PATCH 2/3] rust: kernel: kobject: basic sysfs implementation

[Greg KH]

Some more review after I saw your examples:

On Sun, Dec 08, 2024 at 02:15:44PM +0100, Daniel Sedlak wrote:
> +/// Attribute represents a file in a directory.

Not always, but sometimes, be careful here :)

> +#[macros::vtable]
> +pub trait KObjectTextAttribute<K>
> +where
> +    Self: Sized,
> +{
> +    /// File permissions.
> +    const MODE: Mode = Mode::from_u16(0o777);
> +
> +    /// Name of the file in the sysfs.
> +    const NAME: &'static CStr;

No.  Well yes.   Kind of.  Why aren't you wrapping "struct attribute"
here instead?

> +
> +    /// Gets called when the read is called on the file.
> +    fn show(this: &mut K) -> Result<CString> {
> +        let _ = this;
> +        Err(EIO)
> +    }
> +
> +    /// Gets called when the write is called on the file.
> +    fn store(this: &mut K, input: &CStr) -> Result {
> +        let _ = this;
> +        let _ = input;
> +        Err(EIO)
> +    }

I see you called this "Text" attribute, but yet that's not what the
kernel calls them in the C side.  Again, let's not create multiple names
for the same thing as that's going to cause nothing but headaches for
decades as most of us flit from both C and Rust constantly.

Naming matters, be consistent.

Also, attributes in the kernel do NOT have show/store functions by
default.  The "wrapper" attribute type has them.  So again, be very
careful here.

sysfs/kobjects are tricky and slippery and take advantage of some crazy
things in C (i.e. object structure layouts and looney casts "just
because we know what we are doing".)  Representing this in rust is also
going to have those same types of issues, so watch out.  You simplified
it all here, but note that the C side is complex for real reasons (i.e.
we started out simple, and then got complex to solve real problems.)  Be
aware of our history here first please and don't make the same mistakes
we did in our youth, you have the benifit of learning from how foolish
we were back then :)

> +}
> +
> +#[doc(hidden)]
> +struct TextAttributeVTable<K, A: KObjectTextAttribute<K>>(PhantomData<(A, K)>);
> +#[doc(hidden)]
> +impl<K, A: KObjectTextAttribute<K>> TextAttributeVTable<K, A> {
> +    #[doc(hidden)]
> +    const fn new() -> Self {
> +        Self(PhantomData)
> +    }
> +    unsafe extern "C" fn show_callback(
> +        kobj: *mut bindings::kobject,
> +        _attr: *mut bindings::kobj_attribute,
> +        buf: *mut core::ffi::c_char,
> +    ) -> isize {
> +        match A::show(
> +            &mut unsafe { &mut *container_of!(kobj, KObject<K>, pointer).cast_mut() }.data,
> +        ) {
> +            Ok(cstring) => unsafe {
> +                bindings::sized_strscpy(buf.cast(), cstring.as_char_ptr(), PAGE_SIZE)
> +            },
> +            Err(error) => error.to_errno() as isize,
> +        }
> +    }

You should ALWAYS call sysfs_emit() when printing out data (for a
variety of reasons).  Here you are hard-coding the PAGE_SIZE reference
of the buffer that the kobject core gave you, are you SURE that is
always going to work?  Please just use the apis the C code gives you and
don't attempt to re-write them (again, learn from our mistakes of the
past...

thanks,

greg k-h

[RFC PATCH 3/3] samples: rust: add kobject sample

[Daniel Sedlak]

Add basic example using the kobject API using Rust.
The example is similar to the already existing
examples samples/kobject/{kobject-example.c,kset-example.c}.

Signed-off-by: Daniel Sedlak <daniel@sedlak.dev>
---
 samples/rust/Kconfig            |  8 ++++
 samples/rust/Makefile           |  1 +
 samples/rust/kobject_example.rs | 66 +++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 samples/rust/kobject_example.rs

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index b0f74a81c8f9..75e6db1ba6c0 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -37,4 +37,12 @@ config SAMPLE_RUST_HOSTPROGS
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_KOBJECT
+	bool "Build kobject example in Rust"
+	help
+	  This config option allows to build kobject
+	  exampel written in Rust.
+
+	  If unsure, say N.
+
 endif # SAMPLES_RUST
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index c1a5c1655395..b3ae09ef2c24 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -3,6 +3,7 @@ ccflags-y += -I$(src)				# needed for trace events
 
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_KOBJECT)		+= kobject_example.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
 
diff --git a/samples/rust/kobject_example.rs b/samples/rust/kobject_example.rs
new file mode 100644
index 000000000000..90db937dea5a
--- /dev/null
+++ b/samples/rust/kobject_example.rs
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust example using kobjects.
+
+use kernel::{
+    c_str,
+    kobject::{subsystems, KObject, KObjectTextAttribute},
+    prelude::*,
+    str::CString,
+};
+
+module! {
+    type: RustKObject,
+    name: "rust_kobject",
+    author: "Rust for Linux Contributors",
+    license: "GPL",
+}
+
+struct RustKObject {
+    // This kobject may be referenced later by other parts of code to update
+    // the values stored within [`MyKObject`].
+    _kobject: Pin<KBox<KObject<MyKObject>>>,
+}
+
+/// Sample KObject state which creates directory in the `/sys/kernel/my_kobject`.
+#[derive(Default)]
+pub struct MyKObject {
+    text: Option<CString>,
+}
+
+/// Attribute of the [`MyKObject`], creates file in the `/sys/kernel/my_kobject/my-text`.
+/// Where `show()` is triggered when read is called and `store()` is called
+/// when new data are written.
+pub struct MyKObjectAttribute;
+#[vtable]
+impl KObjectTextAttribute<MyKObject> for MyKObjectAttribute {
+    const NAME: &'static CStr = c_str!("my-text");
+
+    /// Called for example by `cat /sys/kernel/my_kobject/my-text`.
+    fn show(this: &mut MyKObject) -> Result<CString> {
+        if let Some(text) = &this.text {
+            CString::try_from_fmt(fmt!("Text that was stored: '{text:?}'\n"))
+        } else {
+            CString::try_from_fmt(fmt!("No one stored anything yet, value {}.\n", this.value))
+        }
+    }
+
+    /// Called for example by `echo "Hi Rust!" > /sys/kernel/my_kobject/my-text`.
+    fn store(this: &mut MyKObject, input: &CStr) -> Result {
+        this.text = Some(input.to_cstring()?);
+        Ok(())
+    }
+}
+
+impl kernel::Module for RustKObject {
+    fn init(_module: &'static ThisModule) -> kernel::error::Result<Self> {
+        let mut kobject = KObject::new_with_dynamic_kobject_parent(
+            CString::try_from_fmt(fmt!("my_kobject"))?,
+            &subsystems::KERNEL_KOBJECT,
+            MyKObject::default(),
+        )?;
+        kobject.add_attribute(MyKObjectAttribute)?;
+
+        Ok(RustKObject { _kobject: kobject })
+    }
+}
-- 
2.47.1

Re: [RFC PATCH 3/3] samples: rust: add kobject sample

[Greg KH]

On Sun, Dec 08, 2024 at 02:15:45PM +0100, Daniel Sedlak wrote:
> Add basic example using the kobject API using Rust.
> The example is similar to the already existing
> examples samples/kobject/{kobject-example.c,kset-example.c}.

No, please no.  let's not have "raw kobjects in rust" even be an example
of something that is a good idea to use...

That being said:

> +config SAMPLE_RUST_KOBJECT
> +	bool "Build kobject example in Rust"
> +	help
> +	  This config option allows to build kobject
> +	  exampel written in Rust.

spelling check :)

And wrap your lines at a longer length please.

> index 000000000000..90db937dea5a
> --- /dev/null
> +++ b/samples/rust/kobject_example.rs
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust example using kobjects.

No copyright?   No authorship?

> +use kernel::{
> +    c_str,
> +    kobject::{subsystems, KObject, KObjectTextAttribute},
> +    prelude::*,
> +    str::CString,
> +};
> +
> +module! {
> +    type: RustKObject,
> +    name: "rust_kobject",
> +    author: "Rust for Linux Contributors",

No taking ownership of this?  I REALLY want someone to own this if it
ever happens, so that they can handle all of the fallout over time with
it :)

> +    license: "GPL",
> +}
> +
> +struct RustKObject {
> +    // This kobject may be referenced later by other parts of code to update
> +    // the values stored within [`MyKObject`].
> +    _kobject: Pin<KBox<KObject<MyKObject>>>,
> +}
> +
> +/// Sample KObject state which creates directory in the `/sys/kernel/my_kobject`.
> +#[derive(Default)]
> +pub struct MyKObject {
> +    text: Option<CString>,
> +}
> +
> +/// Attribute of the [`MyKObject`], creates file in the `/sys/kernel/my_kobject/my-text`.
> +/// Where `show()` is triggered when read is called and `store()` is called
> +/// when new data are written.
> +pub struct MyKObjectAttribute;
> +#[vtable]
> +impl KObjectTextAttribute<MyKObject> for MyKObjectAttribute {
> +    const NAME: &'static CStr = c_str!("my-text");
> +
> +    /// Called for example by `cat /sys/kernel/my_kobject/my-text`.
> +    fn show(this: &mut MyKObject) -> Result<CString> {
> +        if let Some(text) = &this.text {
> +            CString::try_from_fmt(fmt!("Text that was stored: '{text:?}'\n"))
> +        } else {
> +            CString::try_from_fmt(fmt!("No one stored anything yet, value {}.\n", this.value))

Wouldn't this also happen if the text was ""?  If not, why isn't an
empty string the default to start with?

thanks,

greg k-h

Re: [RFC PATCH 3/3] samples: rust: add kobject sample

[Daniel Sedlak]

On 12/8/24 2:46 PM, Greg KH wrote:
> On Sun, Dec 08, 2024 at 02:15:45PM +0100, Daniel Sedlak wrote:
>> Add basic example using the kobject API using Rust.
>> The example is similar to the already existing
>> examples samples/kobject/{kobject-example.c,kset-example.c}.
> 
> No, please no.  let's not have "raw kobjects in rust" even be an example
> of something that is a good idea to use...

Noted.
> 
> That being said:
> 
>> +config SAMPLE_RUST_KOBJECT
>> +	bool "Build kobject example in Rust"
>> +	help
>> +	  This config option allows to build kobject
>> +	  exampel written in Rust.
> 
> spelling check :)
> 
> And wrap your lines at a longer length please.
> 
>> index 000000000000..90db937dea5a
>> --- /dev/null
>> +++ b/samples/rust/kobject_example.rs
>> @@ -0,0 +1,66 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Rust example using kobjects.
> 
> No copyright?   No authorship?

I copied it from other rust samples, looks like collective "issue".

>> +
>> +module! {
>> +    type: RustKObject,
>> +    name: "rust_kobject",
>> +    author: "Rust for Linux Contributors",
> 
> No taking ownership of this?  I REALLY want someone to own this if it
> ever happens, so that they can handle all of the fallout over time with
> it :)
> 

Also copied from other rust samples, it looks like even some C samples 
have not author specified, but that not justify not specifying one. Will 
remember that in the future.

>> +            CString::try_from_fmt(fmt!("No one stored anything yet, value {}.\n", this.value))
> 
> Wouldn't this also happen if the text was ""?  If not, why isn't an
> empty string the default to start with?
This line in the sample is result of invalid fixup and should be:

	CString::try_from_fmt(fmt!("No one stored anything yet.\n"))



Thank you for your feedback

Daniel

Re: [RFC PATCH 0/3] rust abstractions for interacting with sysfs

[Greg KH]

On Sun, Dec 08, 2024 at 02:15:42PM +0100, Daniel Sedlak wrote:
> Hello,
> For now, there are no facilities for the Rust side to tune modules written
> in Rust in runtime or for just simple debug output/statistics from
> the module. For exactly this purpose, the C has the sysfs. In this
> patch series, I implemented a minimal sysfs wrapper utilizing kobject API.
> 
> I would like to collect your thoughts on whether:
>   - Do we want something like this in the rust part of the kernel?

No.  Module parameters are from the 1990's, please let's not perpetuate
that madness into the next century if possible.

>   - Do we want to support all sysfs capabilities?

For many things, yes.  But let's add that when needed.  I'd like to see
a real user before accepting these types of things, don't just add
bindings when you don't have them as it's impossible to determine if
they are correct or not (see the recent miscdev sample driver for proof
of that, turns out that the bindings we took weren't really right, which
is fine, but it's sometimes easier to figure that out when you have a
user.)

> I implemented a sample to present the API I have chosen for the Rust part.
> It is a straightforward example that does not represent all capabilities
> of the sysfs. The documentation will be potentially improved, but I would
> like to first settle the abstractions.

What exactly do you want to represent in sysfs here?  Start with that
and we can work backwards to say what types of bindings are needed.

Almost NO kernel code should ever call "raw" kobject or sysfs calls,
with some exceptions.  Is your use case one of those exceptions?

thanks,

greg k-h

Re: [RFC PATCH 0/3] rust abstractions for interacting with sysfs

[Daniel Sedlak]

On 12/8/24 2:34 PM, Greg KH wrote:

>> I implemented a sample to present the API I have chosen for the Rust part.
>> It is a straightforward example that does not represent all capabilities
>> of the sysfs. The documentation will be potentially improved, but I would
>> like to first settle the abstractions.
> 
> What exactly do you want to represent in sysfs here?  Start with that
> and we can work backwards to say what types of bindings are needed.

Great question, I should have started with that earlier. Lets say I 
wrote a module that attaches some hooks to kernel function calls and the 
module does some accounting on that. What is the correct way to expose 
the statistics from that module? From my point of view this module does 
not fit into any subsystem already defined in `/sys/`, and procfs should 
be abandoned. Should I use debugfs for that?

Thank you for your feedback

Daniel

Re: [RFC PATCH 0/3] rust abstractions for interacting with sysfs

[Greg KH]

On Mon, Dec 09, 2024 at 05:12:44PM +0100, Daniel Sedlak wrote:
> 
> On 12/8/24 2:34 PM, Greg KH wrote:
> 
> > > I implemented a sample to present the API I have chosen for the Rust part.
> > > It is a straightforward example that does not represent all capabilities
> > > of the sysfs. The documentation will be potentially improved, but I would
> > > like to first settle the abstractions.
> > 
> > What exactly do you want to represent in sysfs here?  Start with that
> > and we can work backwards to say what types of bindings are needed.
> 
> Great question, I should have started with that earlier. Lets say I wrote a
> module that attaches some hooks to kernel function calls and the module does
> some accounting on that.

Modules do not add hooks to function calls :)

> What is the correct way to expose the statistics
> from that module?

debugfs.

> From my point of view this module does not fit into any
> subsystem already defined in `/sys/`, and procfs should be abandoned. Should
> I use debugfs for that?

As it sounds like you are attempting to be a "security module", just use
securityfs instead.

And really, don't hook function calls, or if you must, just use ebpf and
the functionality it provides (all the stats you could ask for!)  No
need to write a kernel module at all.

sysfs is for drivers and the devices they bind to (and devices that do
not have drivers) and sometimes filesystems and occasionally memory
stuff as well (mm is really just a driver for memory).  Don't use it for
things it is not designed for (i.e. module-wide things.)

thanks,

greg k-h