rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rust: place generated init_module() function in .init.text
@ 2024-02-03 18:27 Thomas Bertschinger
  2024-02-03 18:50 ` Martin Rodriguez Reboredo
  2024-02-05  9:45 ` Alice Ryhl
  0 siblings, 2 replies; 6+ messages in thread
From: Thomas Bertschinger @ 2024-02-03 18:27 UTC (permalink / raw)
  To: rust-for-linux, ojeda, alex.gaynor, wedsonaf; +Cc: Thomas Bertschinger

Currently Rust kernel modules have their init code placed in the `.text`
section of the .ko file. I don't think this causes any real problems
for Rust modules but it does present a challenge with implementing Rust
functionality for existing C modules.

If a Rust `init_module()` function (that lives in `.text`) calls a C
function annotated with `__init`, then a warning is generated because
the C function lives in `.init.text`.

I ran into this while experimenting with converting the bcachefs kernel
module from C to Rust. The module's `init()`, written in Rust, calls C
functions like `bch2_vfs_init()` which are placed in the `.init.text`
section.

This patch places the macro-generated `init_module()` Rust function in
the `.init.text` section.

Note that this is not enough on its own to actually get all the module
initialization code in that section. The module author must still add
the `#[link_section = ".init.text"]` attribute to the Rust `init()` in
the `impl kernel::Module` block in order to then call C `__init`
functions. However, this patch enables module authors do so, when
previously it would not be possible (without warnings).

Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
 rust/macros/module.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index d62d8710d77a..d09be1bee397 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -225,6 +225,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             #[cfg(MODULE)]
             #[doc(hidden)]
             #[no_mangle]
+            #[link_section = \".init.text\"]
             pub extern \"C\" fn init_module() -> core::ffi::c_int {{
                 __init()
             }}
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: place generated init_module() function in .init.text
  2024-02-03 18:27 [PATCH] rust: place generated init_module() function in .init.text Thomas Bertschinger
@ 2024-02-03 18:50 ` Martin Rodriguez Reboredo
  2024-02-03 20:26   ` Thomas Bertschinger
  2024-02-05  9:45 ` Alice Ryhl
  1 sibling, 1 reply; 6+ messages in thread
From: Martin Rodriguez Reboredo @ 2024-02-03 18:50 UTC (permalink / raw)
  To: Thomas Bertschinger, rust-for-linux, ojeda, alex.gaynor, wedsonaf

On 2/3/24 15:27, Thomas Bertschinger wrote:
> Currently Rust kernel modules have their init code placed in the `.text`
> section of the .ko file. I don't think this causes any real problems
> for Rust modules but it does present a challenge with implementing Rust
> functionality for existing C modules.
> 
> If a Rust `init_module()` function (that lives in `.text`) calls a C
> function annotated with `__init`, then a warning is generated because
> the C function lives in `.init.text`.
> 
> I ran into this while experimenting with converting the bcachefs kernel
> module from C to Rust. The module's `init()`, written in Rust, calls C
> functions like `bch2_vfs_init()` which are placed in the `.init.text`
> section.
> 
> This patch places the macro-generated `init_module()` Rust function in
> the `.init.text` section.
> 
> Note that this is not enough on its own to actually get all the module
> initialization code in that section. The module author must still add
> the `#[link_section = ".init.text"]` attribute to the Rust `init()` in
> the `impl kernel::Module` block in order to then call C `__init`
> functions. However, this patch enables module authors do so, when
> previously it would not be possible (without warnings).
> 
> Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
> ---
> [...]

In resumi, to call `init` funcs in a module you must do it in
`.init.text`. Amirite?

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: place generated init_module() function in .init.text
  2024-02-03 18:50 ` Martin Rodriguez Reboredo
@ 2024-02-03 20:26   ` Thomas Bertschinger
  0 siblings, 0 replies; 6+ messages in thread
From: Thomas Bertschinger @ 2024-02-03 20:26 UTC (permalink / raw)
  To: Martin Rodriguez Reboredo; +Cc: rust-for-linux, ojeda, alex.gaynor, wedsonaf

On Sat, Feb 03, 2024 at 03:50:02PM -0300, Martin Rodriguez Reboredo wrote:
> In resumi, to call `init` funcs in a module you must do it in
> `.init.text`. Amirite?

Yeah, well, I'd say that's needed to do it without warnings from
modpost at least.

In theory it "shouldn't" cause any problems to call __init functions
from the Rust init() even if it's in .text, since that init() should
only ever run once. But then if anything does run Rust's init() after
module initialization, that will definitely cause problems. I don't
think that should ever happen though...

- Thomas Bertschinger

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: place generated init_module() function in .init.text
  2024-02-03 18:27 [PATCH] rust: place generated init_module() function in .init.text Thomas Bertschinger
  2024-02-03 18:50 ` Martin Rodriguez Reboredo
@ 2024-02-05  9:45 ` Alice Ryhl
  2024-02-05 14:46   ` Thomas Bertschinger
  1 sibling, 1 reply; 6+ messages in thread
From: Alice Ryhl @ 2024-02-05  9:45 UTC (permalink / raw)
  To: Thomas Bertschinger; +Cc: rust-for-linux, ojeda, alex.gaynor, wedsonaf

On Sat, Feb 3, 2024 at 7:28 PM Thomas Bertschinger
<tahbertschinger@gmail.com> wrote:
>
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index d62d8710d77a..d09be1bee397 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -225,6 +225,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              #[cfg(MODULE)]
>              #[doc(hidden)]
>              #[no_mangle]
> +            #[link_section = \".init.text\"]
>              pub extern \"C\" fn init_module() -> core::ffi::c_int {{
>                  __init()
>              }}

Perhaps this function should be made unsafe? I don't think there's
currently anything preventing safe non-init Rust code from calling it
directly as-is.

Alice

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: place generated init_module() function in .init.text
  2024-02-05  9:45 ` Alice Ryhl
@ 2024-02-05 14:46   ` Thomas Bertschinger
  2024-02-05 14:48     ` Alice Ryhl
  0 siblings, 1 reply; 6+ messages in thread
From: Thomas Bertschinger @ 2024-02-05 14:46 UTC (permalink / raw)
  To: Alice Ryhl; +Cc: rust-for-linux, ojeda, alex.gaynor, wedsonaf

On Mon, Feb 05, 2024 at 10:45:19AM +0100, Alice Ryhl wrote:
> On Sat, Feb 3, 2024 at 7:28 PM Thomas Bertschinger
> <tahbertschinger@gmail.com> wrote:
> >
> > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > index d62d8710d77a..d09be1bee397 100644
> > --- a/rust/macros/module.rs
> > +++ b/rust/macros/module.rs
> > @@ -225,6 +225,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> >              #[cfg(MODULE)]
> >              #[doc(hidden)]
> >              #[no_mangle]
> > +            #[link_section = \".init.text\"]
> >              pub extern \"C\" fn init_module() -> core::ffi::c_int {{
> >                  __init()
> >              }}
> 
> Perhaps this function should be made unsafe? I don't think there's
> currently anything preventing safe non-init Rust code from calling it
> directly as-is.
> 
> Alice

That's a good point. That makes sense to me; I can work on a v2 that does
this.

- Thomas Bertschinger

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: place generated init_module() function in .init.text
  2024-02-05 14:46   ` Thomas Bertschinger
@ 2024-02-05 14:48     ` Alice Ryhl
  0 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2024-02-05 14:48 UTC (permalink / raw)
  To: Thomas Bertschinger; +Cc: rust-for-linux, ojeda, alex.gaynor, wedsonaf

On Mon, Feb 5, 2024 at 3:46 PM Thomas Bertschinger
<tahbertschinger@gmail.com> wrote:
>
> On Mon, Feb 05, 2024 at 10:45:19AM +0100, Alice Ryhl wrote:
> > On Sat, Feb 3, 2024 at 7:28 PM Thomas Bertschinger
> > <tahbertschinger@gmail.com> wrote:
> > >
> > > diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> > > index d62d8710d77a..d09be1bee397 100644
> > > --- a/rust/macros/module.rs
> > > +++ b/rust/macros/module.rs
> > > @@ -225,6 +225,7 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
> > >              #[cfg(MODULE)]
> > >              #[doc(hidden)]
> > >              #[no_mangle]
> > > +            #[link_section = \".init.text\"]
> > >              pub extern \"C\" fn init_module() -> core::ffi::c_int {{
> > >                  __init()
> > >              }}
> >
> > Perhaps this function should be made unsafe? I don't think there's
> > currently anything preventing safe non-init Rust code from calling it
> > directly as-is.
> >
> > Alice
>
> That's a good point. That makes sense to me; I can work on a v2 that does
> this.

With that fixed, you may add

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-02-05 14:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-03 18:27 [PATCH] rust: place generated init_module() function in .init.text Thomas Bertschinger
2024-02-03 18:50 ` Martin Rodriguez Reboredo
2024-02-03 20:26   ` Thomas Bertschinger
2024-02-05  9:45 ` Alice Ryhl
2024-02-05 14:46   ` Thomas Bertschinger
2024-02-05 14:48     ` Alice Ryhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).