[PATCH v2] rust: place generated init_module() function in .init.text

[PATCH v2] rust: place generated init_module() function in .init.text

[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 `.init.text`.

This patch places the macro-generated `init_module()` Rust function in
the `.init.text` section. It also marks that function as unsafe--now it
may not be called again after returning because it may be freed already.

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>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
---
V2: add "unsafe" to the init_module() function

 rust/macros/module.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/rust/macros/module.rs b/rust/macros/module.rs
index d62d8710d77a..e54293faf523 100644
--- a/rust/macros/module.rs
+++ b/rust/macros/module.rs
@@ -222,10 +222,14 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
             }};
 
             // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
+            /// # Safety
+            /// This function must only be called once, during module initialization, because it
+            /// may be freed after it returns.
             #[cfg(MODULE)]
             #[doc(hidden)]
             #[no_mangle]
-            pub extern \"C\" fn init_module() -> core::ffi::c_int {{
+            #[link_section = \".init.text\"]
+            pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {{
                 __init()
             }}
 
-- 
2.43.0

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Martin Rodriguez Reboredo]

On 2/5/24 22:25, Thomas Bertschinger wrote:
> [...]
>               // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> +            /// # Safety
> +            /// This function must only be called once, during module initialization, because it
> +            /// may be freed after it returns.

The thing with this doc comment is that it's going to be hidden because
of the `#[doc(hidden)]` attribute. I think that it should either be a
common comment, placed elsewhere or removed.

> [...]

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Thomas Bertschinger]

On Mon, Feb 05, 2024 at 10:39:40PM -0300, Martin Rodriguez Reboredo wrote:
> On 2/5/24 22:25, Thomas Bertschinger wrote:
> > [...]
> >               // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> > +            /// # Safety
> > +            /// This function must only be called once, during module initialization, because it
> > +            /// may be freed after it returns.
> 
> The thing with this doc comment is that it's going to be hidden because
> of the `#[doc(hidden)]` attribute. I think that it should either be a
> common comment, placed elsewhere or removed.

I went back and forth on the best way to comment this (or not). I am
fine with any of the proposals, with perhaps a slight preference for
removing it so as to not clutter the already somewhat busy macro. If it
is important to keep, then I think a common comment in this location
makes more sense than moving it elsewhere. Anyone else have an opinion?

- Thomas Bertschinger

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Trevor Gross]

On Mon, Feb 5, 2024 at 9:13 PM Thomas Bertschinger
<tahbertschinger@gmail.com> wrote:
>
> On Mon, Feb 05, 2024 at 10:39:40PM -0300, Martin Rodriguez Reboredo wrote:
> > On 2/5/24 22:25, Thomas Bertschinger wrote:
> > > [...]
> > >               // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> > > +            /// # Safety
> > > +            /// This function must only be called once, during module initialization, because it
> > > +            /// may be freed after it returns.
> >
> > The thing with this doc comment is that it's going to be hidden because
> > of the `#[doc(hidden)]` attribute. I think that it should either be a
> > common comment, placed elsewhere or removed.
>
> I went back and forth on the best way to comment this (or not). I am
> fine with any of the proposals, with perhaps a slight preference for
> removing it so as to not clutter the already somewhat busy macro. If it
> is important to keep, then I think a common comment in this location
> makes more sense than moving it elsewhere. Anyone else have an opinion?
>
> - Thomas Bertschinger
>

My 2cents says to keep it as-is - it's useful, doc comment form makes
it clear what it is in reference to, and I think even hidden items
show up if rustdoc is run with --document-private-items.

- Trevor

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Trevor Gross]

On Mon, Feb 5, 2024 at 8:26 PM Thomas Bertschinger
<tahbertschinger@gmail.com> 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 `.init.text`.
>
> This patch places the macro-generated `init_module()` Rust function in
> the `.init.text` section. It also marks that function as unsafe--now it
> may not be called again after returning because it may be freed already.
>
> 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>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> ---
> V2: add "unsafe" to the init_module() function
>
>  rust/macros/module.rs | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index d62d8710d77a..e54293faf523 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -222,10 +222,14 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              }};
>
>              // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> +            /// # Safety
> +            /// This function must only be called once, during module initialization, because it
> +            /// may be freed after it returns.

Extremely minor nit, there's usually a line under section headings.
Miguel will probably add this when he picks it up.

>              #[cfg(MODULE)]
>              #[doc(hidden)]
>              #[no_mangle]
> -            pub extern \"C\" fn init_module() -> core::ffi::c_int {{
> +            #[link_section = \".init.text\"]
> +            pub unsafe extern \"C\" fn init_module() -> core::ffi::c_int {{
>                  __init()
>              }}
>
> --
> 2.43.0
>
>

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Greg KH]

On Mon, Feb 05, 2024 at 06:25:35PM -0700, Thomas Bertschinger wrote:
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index d62d8710d77a..e54293faf523 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -222,10 +222,14 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              }};
>  
>              // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> +            /// # Safety
> +            /// This function must only be called once, during module initialization, because it
> +            /// may be freed after it returns.

"once" is not right, it can be called zillions of times, it's just that
it can't be called _AFTER_ module_init() is over for the file, as it
will no longer be present in memory.

So technically, the wording you added here isn't correct, sorry.

thanks,

greg k-h

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 11:01 AM Greg KH <greg@kroah.com> wrote:
>
> "once" is not right, it can be called zillions of times, it's just that
> it can't be called _AFTER_ module_init() is over for the file, as it
> will no longer be present in memory.
>
> So technically, the wording you added here isn't correct, sorry.

The safety preconditions may be stronger than needed. For instance, if
we do not expect anybody to manually call this, then we can say so. In
fact, we could even say Rust code cannot call this function.

Cheers,
Miguel

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Greg KH]

On Tue, Feb 06, 2024 at 11:51:35AM +0100, Miguel Ojeda wrote:
> On Tue, Feb 6, 2024 at 11:01 AM Greg KH <greg@kroah.com> wrote:
> >
> > "once" is not right, it can be called zillions of times, it's just that
> > it can't be called _AFTER_ module_init() is over for the file, as it
> > will no longer be present in memory.
> >
> > So technically, the wording you added here isn't correct, sorry.
> 
> The safety preconditions may be stronger than needed. For instance, if
> we do not expect anybody to manually call this, then we can say so. In
> fact, we could even say Rust code cannot call this function.

But why not?  Rust code can call these functions, and you should be
writing functions in rust code in the init section.  There's nothing
"special" about this other than it is a memory size savings, and if you
get it wrong, the linker will tell you at build time.

thanks,

greg k-h

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 11:58 AM Greg KH <greg@kroah.com> wrote:
>
> But why not?  Rust code can call these functions, and you should be
> writing functions in rust code in the init section.  There's nothing
> "special" about this other than it is a memory size savings, and if you
> get it wrong, the linker will tell you at build time.

I think you are talking about init-section functions in general -- I
agree we could have safe `__init` Rust functions if there is something
else that guarantees no case escapes (and, of course, we can also have
unsafe `__init` Rust functions if there is no such system that
guarantees that).

But here I was talking about the particular case of these module
initialization functions that are generated from a macro and are
explicitly not documented publicly (not just the one in this patch,
also e.g. `__init()`). These could be even more restricted than
`unsafe` and completely hidden away (in a "private-in-private module"
or an unnamed constant or similar hacks to accomplish that).

Cheers,
Miguel

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Thomas Bertschinger]

On Tue, Feb 06, 2024 at 11:51:35AM +0100, Miguel Ojeda wrote:
> On Tue, Feb 6, 2024 at 11:01 AM Greg KH <greg@kroah.com> wrote:
> >
> > "once" is not right, it can be called zillions of times, it's just that
> > it can't be called _AFTER_ module_init() is over for the file, as it
> > will no longer be present in memory.
> >
> > So technically, the wording you added here isn't correct, sorry.
> 
> The safety preconditions may be stronger than needed. For instance, if
> we do not expect anybody to manually call this, then we can say so. In
> fact, we could even say Rust code cannot call this function.
> 
> Cheers,
> Miguel

I can send a v3 that fixes the wording of the comment; it's worth being
correct.

That said, my intention was more aligned with what Miguel suggested, to
say that user-written Rust code should not call this function, rather
than to claim that it's impossible. If there is a need for the
init_module() to run multiple times, it might make more sense to put
that work in a user-defined function that gets called multiple times.

On that note, does anyone know any examples of modules that call their
init function more than once? I might go searching myself later to see
if I can find any, but in case anyone knows any off-hand. If there are
real use cases for this, it could be good to consider those here.

- Thomas Bertschinger

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Martin Rodriguez Reboredo]

On 2/6/24 11:49, Thomas Bertschinger wrote:
> [...]
> On that note, does anyone know any examples of modules that call their
> init function more than once? I might go searching myself later to see
> if I can find any, but in case anyone knows any off-hand. If there are
> real use cases for this, it could be good to consider those here.

Un/loading modules using `modprobe` to do troubleshooting per the
manpage on the `-r` flag, although that case is very rare by itself. I
don't know of any other case besides it.

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 5:07 PM Martin Rodriguez Reboredo
<yakoyoku@gmail.com> wrote:
>
> Un/loading modules using `modprobe` to do troubleshooting per the
> manpage on the `-r` flag, although that case is very rare by itself. I
> don't know of any other case besides it.

I am not sure I follow -- if you reload the module, the functions will
be reloaded too.

Cheers,
Miguel

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Martin Rodriguez Reboredo]

On 2/6/24 14:19, Miguel Ojeda wrote:
> On Tue, Feb 6, 2024 at 5:07 PM Martin Rodriguez Reboredo
> <yakoyoku@gmail.com> wrote:
>>
>> Un/loading modules using `modprobe` to do troubleshooting per the
>> manpage on the `-r` flag, although that case is very rare by itself. I
>> don't know of any other case besides it.
> 
> I am not sure I follow -- if you reload the module, the functions will
> be reloaded too.

Ah, I've said the only case that I knew but I've overlooked that detail.
@_@

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 3:50 PM Thomas Bertschinger
<tahbertschinger@gmail.com> wrote:
>
> I can send a v3 that fixes the wording of the comment; it's worth being
> correct.

(Perhaps you didn't mean this, but just to be clear...)

No, safety sections are not incorrect just because they are not as
relaxed as they could be. They aren't "normal" comments -- they don't
document what the function does or could do.

In many cases, relaxing the preconditions is what you want, of course,
because that makes the function easier to call (and ideally make them
safe); but in some cases you may not want that, e.g. because you don't
want callers to start to rely on something you may change later even
if you happen to support it now.

Because of this, the absence of a safety section can actually be what
makes it incorrect, unlike "normal" comments where removing them is a
easy way to avoid mistakes in the comment :)

Now, we may nevertheless want to relax the preconditions here or
document (somewhere that is not the safety section) what the function
could offer, but my point is that it is perfectly OK to have a
function with restricted preconditions.

And this is one of those cases where it can make sense: unless we
expect to have a use case in-tree for this, I don't see the need to
let others call this.

Cheers,
Miguel

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Greg KH]

On Mon, Feb 05, 2024 at 06:25:35PM -0700, 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 `.init.text`.
> 
> This patch places the macro-generated `init_module()` Rust function in
> the `.init.text` section. It also marks that function as unsafe--now it
> may not be called again after returning because it may be freed already.
> 
> 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>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> ---
> V2: add "unsafe" to the init_module() function
> 
>  rust/macros/module.rs | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/macros/module.rs b/rust/macros/module.rs
> index d62d8710d77a..e54293faf523 100644
> --- a/rust/macros/module.rs
> +++ b/rust/macros/module.rs
> @@ -222,10 +222,14 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream {
>              }};
>  
>              // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> +            /// # Safety
> +            /// This function must only be called once, during module initialization, because it
> +            /// may be freed after it returns.

Oh, and the compiler will catch this if you get it wrong, correct?

If not, you all should work on that, as it is caught in C code :)

thanks,

greg k-h

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Alice Ryhl]

On Tue, Feb 6, 2024 at 11:01 AM Greg KH <greg@kroah.com> wrote:
>
> On Mon, Feb 05, 2024 at 06:25:35PM -0700, Thomas Bertschinger wrote:
> >              // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> > +            /// # Safety
> > +            /// This function must only be called once, during module initialization, because it
> > +            /// may be freed after it returns.
>
> Oh, and the compiler will catch this if you get it wrong, correct?
>
> If not, you all should work on that, as it is caught in C code :)

Based on the commit message, it sounds like the warning that C uses to
catch this happens a link-time, which means that it is also triggered
on Rust code:

> 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 suggested that we make this function unsafe too, to make it even
harder to call incorrectly. But I think for the more general case of
this problem, it makes sense to rely on the existing warning for this.

Alice

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Greg KH]

On Tue, Feb 06, 2024 at 11:29:05AM +0100, Alice Ryhl wrote:
> On Tue, Feb 6, 2024 at 11:01 AM Greg KH <greg@kroah.com> wrote:
> >
> > On Mon, Feb 05, 2024 at 06:25:35PM -0700, Thomas Bertschinger wrote:
> > >              // Loadable modules need to export the `{{init,cleanup}}_module` identifiers.
> > > +            /// # Safety
> > > +            /// This function must only be called once, during module initialization, because it
> > > +            /// may be freed after it returns.
> >
> > Oh, and the compiler will catch this if you get it wrong, correct?
> >
> > If not, you all should work on that, as it is caught in C code :)
> 
> Based on the commit message, it sounds like the warning that C uses to
> catch this happens a link-time, which means that it is also triggered
> on Rust code:
> 
> > 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`.

Cool, but has anyone tested it?

> I suggested that we make this function unsafe too, to make it even
> harder to call incorrectly.

It's not "unsafe" at all!  It's totally normal, and "safe" and should be
used for many types of modules.  If you get it "wrong" the linker will
catch the issue, so what is unsafe?

> But I think for the more general case of this problem, it makes sense
> to rely on the existing warning for this.

If the linker catches this, yes.

thanks,

greg k-h

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 11:50 AM Greg KH <greg@kroah.com> wrote:
>
> Cool, but has anyone tested it?

Yeah, in C, I see some warnings from modpost (section mismatch, plus
the explicitly exported symbol), but it does not seem to catch all
cases under `defconfig` at least.

In Rust, similarly, I also get the warning in some cases, but not all;
e.g. I had to force the function to be non-inline so that modpost can
notice.

Anyway, the "unsafe" approach (or hiding the functions completely
inside a `const` if possible, but IIRC from a previous discussion that
required rearranging things a bit due to the global asm block --
ideally Rust would provide anonymous modules) would be even better.

> It's not "unsafe" at all!  It's totally normal, and "safe" and should be
> used for many types of modules.  If you get it "wrong" the linker will
> catch the issue, so what is unsafe?

In Rust's terminology, "unsafe function" does not mean "wrong" or
"broken" or "always triggers UB", it just means there are safety
preconditions, i.e. "it is UB if you break my preconditions;
otherwise, there isn't UB".

In other words, it means that callers need to be careful (and cannot
call the function without using unsafe Rust) and they also are
required (via a comment) why their call is correct.

So if we do not want any Rust caller to call this function, we could
say "Rust callers must not call this function". Even if the function
could be obviously called in other cases.

Put another way, as a function writer, you can hide "use cases" if you
do not want to support them. Even if they happen to work today. You
can think of it as opaque/private members in structs, but for
functions, i.e. contracts.

So if it is an unintended use case, it is better to catch it as early
as possible, even assuming there is another safety net somewhere else
that catches that too.

Cheers,
Miguel

Re: [PATCH v2] rust: place generated init_module() function in .init.text

[Miguel Ojeda]

On Tue, Feb 6, 2024 at 11:01 AM Greg KH <greg@kroah.com> wrote:
>
> Oh, and the compiler will catch this if you get it wrong, correct?

The patch makes it an unsafe function, which means that the compiler
will forbid "normal" calls to it, requiring unsafe code to do so (and
thus explaining why you are upholding the precondition), so yes in
that sense.

In addition, these are supposed to be an undocumented / hidden
function / implementation details. In fact, all these ones should be
unsafe (or even inside an unnamed constant as IIRC Benno proposed a
long time ago).

Having said that, I suspect what you are talking about modpost
(section mismatch and the explicit warning about exported symbols) or
perhaps objtool, not the C compiler?

Cheers,
Miguel