linux-kernel - Re: [PATCH 1/2] rust: introduce `InPlaceModule`

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]

Date: Wed, 27 Mar 2024 15:56:28 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Wedson Almeida Filho <wedsonaf@...il.com>
Cc: Valentin Obst <kernel@...entinobst.de>, a.hindborg@...sung.com, alex.gaynor@...il.com, aliceryhl@...gle.com, bjorn3_gh@...tonmail.com, boqun.feng@...il.com, gary@...yguo.net, linux-kernel@...r.kernel.org, ojeda@...nel.org, rust-for-linux@...r.kernel.org, walmeida@...rosoft.com, Trevor Gross <tmgross@...ch.edu>, FUJITA Tomonori <fujita.tomonori@...il.com>
Subject: Re: [PATCH 1/2] rust: introduce `InPlaceModule`

On 27.03.24 15:23, Wedson Almeida Filho wrote:
> On Wed, 27 Mar 2024 at 05:13, Valentin Obst <kernel@...entinobst.de> wrote:
>>
>>> This allows modules to be initialised in-place in pinned memory, which
>>> enables the usage of pinned types (e.g., mutexes, spinlocks, driver
>>> registrations, etc.) in modules without any extra allocations.
>>>
>>> Drivers that don't need this may continue to implement `Module` without
>>> any changes.
>>>
>>> Signed-off-by: Wedson Almeida Filho <walmeida@...rosoft.com>
>>> ---
>>>   rust/kernel/lib.rs    | 25 ++++++++++++++++++++++++-
>>>   rust/macros/module.rs | 18 ++++++------------
>>>   2 files changed, 30 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 5c641233e26d..64aee4fbc53b 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -62,7 +62,7 @@
>>>   /// The top level entrypoint to implementing a kernel module.
>>>   ///
>>>   /// For any teardown or cleanup operations, your type may implement [`Drop`].
>>> -pub trait Module: Sized + Sync {
>>> +pub trait Module: Sized + Sync + Send {
>>
>> This does not compile with `CONFIG_AX88796B_RUST_PHY=y || m` (or the
>> phylib abstractions' doctests) since the module `Registration` is not
>> `Send`.
> 
> Thanks for the heads up. I thought I had enabled all rust code but
> indeed I was missing this. I will fix it in v2.
> 
>> I remember Trevor raising the question whether we want to require modules
>> to be `Send`. I am not aware of any examples of `!Send` modules but I guess
>> it would be possible to write code that is only correct under the
>> assumption that it is loaded/unloaded in the same context.
> 
> It might be possible in the future, but I don't believe it is now
> because all rust modules support unloading. And there is no guarantee
> that the thread unloading (and therefore calling module_exit) is the
> same that loaded (and called module_init), so a module must be Send to
> properly handle drop being called from a different thread.
> 
> Not requiring Send on the original Module trait was an oversight that
> I don't want to repeat in InPlaceModule.

I think that this change should go to the stable tree, can you split it
into its own patch?

-- 
Cheers,
Benno

> 
>>
>> @Trevor: Are you aware of any modules with that requirement?
>>
>> I have been using this patch for quite a while with my TCP CCAs now
>> (without the `Send` bound) and did not experience any other issues; thus
>> offering:
>>          Tested-by: Valentin Obst <kernel@...entinobst.de>
> 
> Thanks!
> 
>>
>>          - Best Valentin
>>