linux-kernel - Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

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: <ZnRUXdMaFJydAn__@cassiopeiae>
Date: Thu, 20 Jun 2024 18:10:05 +0200
From: Danilo Krummrich <dakr@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: rafael@...nel.org, bhelgaas@...gle.com, ojeda@...nel.org,
	alex.gaynor@...il.com, wedsonaf@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...sung.com, aliceryhl@...gle.com, airlied@...il.com,
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org,
	daniel.almeida@...labora.com, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v2 01/10] rust: pass module name to `Module::init`

On Thu, Jun 20, 2024 at 04:19:48PM +0200, Greg KH wrote:
> On Wed, Jun 19, 2024 at 01:39:47AM +0200, Danilo Krummrich wrote:
> > In a subsequent patch we introduce the `Registration` abstraction used
> > to register driver structures. Some subsystems require the module name on
> > driver registration (e.g. PCI in __pci_register_driver()), hence pass
> > the module name to `Module::init`.
> 
> I understand the need/want here, but it feels odd that you have to
> change anything to do it.
> 
> > 
> > Signed-off-by: Danilo Krummrich <dakr@...hat.com>
> > ---
> >  rust/kernel/lib.rs           | 14 ++++++++++----
> >  rust/kernel/net/phy.rs       |  2 +-
> >  rust/macros/module.rs        |  3 ++-
> >  samples/rust/rust_minimal.rs |  2 +-
> >  samples/rust/rust_print.rs   |  2 +-
> >  5 files changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> > index a791702b4fee..5af00e072a58 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -71,7 +71,7 @@ pub trait Module: Sized + Sync + Send {
> >      /// should do.
> >      ///
> >      /// Equivalent to the `module_init` macro in the C API.
> > -    fn init(module: &'static ThisModule) -> error::Result<Self>;
> > +    fn init(name: &'static str::CStr, module: &'static ThisModule) -> error::Result<Self>;
> 
> Why can't the name come directly from the build system?  Why must it be
> passed into the init function of the module "class"?  What is it going
> to do with it?

The name does come from the build system, that's where `Module::init` gets it
from.

> 
> A PCI, or other bus, driver "knows" it's name already by virtue of the
> build system, so it can pass that string into whatever function needs

Let's take pci_register_driver() as example.

```
#define pci_register_driver(driver)		\
	__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
```

In C drivers this works because (1) it's a macro and (2) it's called directly
from the driver code.

In Rust, for very good reasons, we have abstractions for C APIs, hence the
actual call to __pci_register_driver() does not come from code within the
module, but from the PCI Rust abstraction `Module::init` calls into instead.

Consequently, we have to pass things through. This also isn't new, please note
that the current code already does the same thing: `Module::init` (without this
patch) is already declared as

`fn init(module: &'static ThisModule) -> error::Result<Self>`

passing through `ThisModule` for the exact same reason.

Please also note that in the most common case (one driver per module) we don't
see any of this anyway.

Just like the C macro module_pci_driver(), Rust drivers can use the
`module_pci_driver!` macro.

Example from Nova:

```
    kernel::module_pci_driver! {
        // The driver type that implements the corresponding probe() and
        // remove() driver callbacks.
        type: NovaDriver,
        name: "Nova",
        author: "Danilo Krummrich",
        description: "Nova GPU driver",
        license: "GPL v2",
    }
```

> that, but the module init function itself does NOT need that.
> 
> So I fail to understand why we need to burden ALL module init functions
> with this, when only a very very very tiny subset of all drivers will
> ever need to know this, and even then, they don't need to know it at
> init module time, they know it at build time and it will be a static
> string at that point, it will not be coming in through an init call.
> 
> thanks,
> 
> greg k-h
>