Message-Id: <DAQIXKJ9VMS6.2044WT0FQQCVC@kernel.org>
Date: Thu, 19 Jun 2025 14:55:03 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Andreas Hindborg" <a.hindborg@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Alice Ryhl" <aliceryhl@...gle.com>, "Masahiro
 Yamada" <masahiroy@...nel.org>, "Nathan Chancellor" <nathan@...nel.org>,
 "Luis Chamberlain" <mcgrof@...nel.org>, "Danilo Krummrich"
 <dakr@...nel.org>, "Nicolas Schier" <nicolas.schier@...ux.dev>, "Trevor
 Gross" <tmgross@...ch.edu>, "Adam Bratschi-Kaye" <ark.email@...il.com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-kbuild@...r.kernel.org>, "Petr Pavlu" <petr.pavlu@...e.com>, "Sami
 Tolvanen" <samitolvanen@...gle.com>, "Daniel Gomez" <da.gomez@...sung.com>,
 "Simona Vetter" <simona.vetter@...ll.ch>, "Greg KH"
 <gregkh@...uxfoundation.org>, "Fiona Behrens" <me@...enk.dev>, "Daniel
 Almeida" <daniel.almeida@...labora.com>, <linux-modules@...r.kernel.org>
Subject: Re: [PATCH v13 2/6] rust: introduce module_param module

On Thu Jun 19, 2025 at 2:20 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@...nel.org> writes:
>> On Thu Jun 12, 2025 at 3:40 PM CEST, Andreas Hindborg wrote:
>>> +
>>> +// SAFETY: C kernel handles serializing access to this type. We never access it
>>> +// from Rust module.
>>> +unsafe impl Sync for RacyKernelParam {}
>>> +
>>> +/// Types that can be used for module parameters.
>>> +pub trait ModuleParam: Sized + Copy {
>>
>> Why the `Copy` bound?
>
> Because of potential unsoundness due to drop [1]. I should document
> this. It is noted in the change log for the series under the obscure
> entry "Assign through pointer rather than using `core::ptr::replace`."
>
> [1] https://lore.kernel.org/all/878qnbxtyi.fsf@kernel.org

Ah thanks for the pointer, yeah please mention this in a comment
somewhere.

>>> +    ///
>>> +    /// Parameters passed at boot time will be set before [`kmalloc`] is
>>> +    /// available (even if the module is loaded at a later time). However, in
>>
>> I think we should make a section out of this like `# No allocations` (or
>> something better). Let's also mention it on the trait itself, since
>> that's where implementers will most likely look.
>
> Since this series only support `Copy` types that are passed by value, I
> think we can remove this comment for now. I will also restrict the
> lifetime of the string to he duration of the call. Putting static here
> would be lying.
>
>>
>>> +    /// this case, the argument buffer will be valid for the entire lifetime of
>>> +    /// the kernel. So implementations of this method which need to allocate
>>> +    /// should first check that the allocator is available (with
>>> +    /// [`crate::bindings::slab_is_available`]) and when it is not available
>>
>> We probably shouldn't recommend directly using `bindings`.
>>
>>> +    /// provide an alternative implementation which doesn't allocate. In cases
>>> +    /// where the allocator is not available it is safe to save references to
>>> +    /// `arg` in `Self`, but in other cases a copy should be made.
>>
>> I don't understand this convention, but it also doesn't seem to
>> relevant (so feel free to leave it as is, but it would be nice if you
>> could explain it).
>
> It has become irrelevant as the series evolved. When we supported
> `!Copy` types we would use the reference if we knew it would be valid
> for the lifetime of the kernel, otherwise we would allocate [1].
>
> However, when the reference is passed at module load time, it is still
> guaranteed to be live for the lifetime of the module, and hence it can
> still be considered `'static`. But, if the reference were to find it's
> way across the module boundary, it can cause UAF issues as the reference
> is not truely `'static`, it is actually `'module`. This ties into the
> difficulty we have around safety of unloading modules. Module unload
> should be marked unsafe.

Ah so the argument should rather be an enum that is either
`Static(&'static str)` or `WithAlloc(&'short str)` with the (non-safety)
guarantee that `WithAlloc` is only passed when the allocator is
available.

> At any rate, I will remove the `'static` lifetime from the reference and
> we are all good for now.

Sounds simplest for now.

>>> +    crate::error::from_result(|| {
>>> +        let new_value = T::try_from_param_arg(arg)?;
>>> +
>>> +        // SAFETY: By function safety requirements `param` is be valid for reads.
>>> +        let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>>> +
>>> +        // SAFETY: By function safety requirements, the target of `old_value` is valid for writes
>>> +        // and is initialized.
>>> +        unsafe { *old_value = new_value };
>>
>> So if we keep the `ModuleParam: Copy` bound from above, then we don't
>> need to drop the type here (as `Copy` implies `!Drop`). So we could also
>> remove the requirement for initialized memory and use `ptr::write` here
>> instead. Thoughts?
>
> Yes, that is the rationale for the `Copy` bound. What would be the
> benefit of using `ptr::write`? They should be equivalent for `Copy`
> types, right.

They should be equivalent, but if we drop the requirement that the value
is initialized to begin with, then removing `Copy` will result in UB
here.

> I was using `ptr::replace`, but Alice suggested the pace expression
> assignment instead, since I was not using the old value.

That makes sense, but if we also remove the initialized requirement,
then I would prefer `ptr::write`.

---
Cheers,
Benno

Powered by blists - more mailing lists