On Thu Jun 19, 2025 at 2:20 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <los...@kernel.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....@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
