Re: [PATCH v13 2/6] rust: introduce module_param module

[Andreas Hindborg]

"Benno Lossin" <lossin@xxxxxxxxxx> writes:

> On Tue Jul 1, 2025 at 4:14 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@xxxxxxxxxx> writes:
>>> On Tue Jul 1, 2025 at 10:43 AM CEST, Andreas Hindborg wrote:
>>>> No, I am OK for now with configfs.
>>>>
>>>> But, progress is still great. How about if we add a copy accessor
>>>> instead for now, I think you proposed that a few million emails ago:
>>>>
>>>>     pub fn get(&self) -> T;
>>>>
>>>> or maybe rename:
>>>>
>>>>     pub fn copy(&self) -> T;
>>>>
>>>> Then we are fine safety wise for now, right? It is even sensible for
>>>> these `T: Copy` types.
>>>
>>> That is better than getting a reference, but still someone could read at
>>> the same time that a write is happening (though we need some new
>>> abstractions AFAIK?). But I fear that we forget about this issue,
>>> because it'll be some time until we land parameters that are `!Copy` (if
>>> at all...)
>>
>> No, that could not happen when we are not allowing custom parsing or
>> sysfs access. Regarding forgetting, I already added a `NOTE` on `!Copy`,
>> and I would add one on this issue as well.
>
> Ultimately this is something for Miguel to decide. I would support an
> unsafe accessor (we should also make it `-> T`), since there it "can't
> go wrong", any UB is the fault of the user of the API. It also serves as
> a good reminder, since a `NOTE` comment shouldn't be something
> guaranteeing safety (we do have some of these global invariants, but I
> feel like this one is too tribal and doesn't usually come up, so I feel
> like it's more dangerous).

I see no reason for making it unsafe when it is safe?


    /// Get a copy of the parameter value.
    ///
    /// # Note
    ///
    /// When this method is called in `const` context, the default
    /// parameter value will be returned. During module initialization, the
    /// kernel will populate the parameter with the value supplied at module
    /// load time or kernel boot time.
    // NOTE: When sysfs access to parameters are enabled, we have to pass in a
    // held lock guard here.
    //
    // NOTE: When we begin supporting custom parameter parsing with user
    // supplied code, this method must be synchronized.
    pub const fn copy(&self) -> T {
        // SAFETY: As we only support read only parameters with no sysfs
        // exposure, the kernel will not touch the parameter data after module
        // initialization. The kernel will update the parameters serially, so we
        // will not race with other accesses.
        unsafe { *self.data.get() }
    }

>
> I think we should also move this patchset along, we could also opt for
> no accessor at all :) Then it isn't really useful without the downstream
> accessor function, but that can come after.

I would rather not merge it without an access method. Then it is just
dead code, and we will not notice if we break it.


Best regards,
Andreas Hindborg