Re: [PATCH] rust: add `module_params` macro

[Andreas Hindborg <nmi@metaspace.dk>]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 01.08.24 15:40, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>> On 01.08.24 13:29, Andreas Hindborg wrote:
>>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>>> On 05.07.24 13:15, Andreas Hindborg wrote:
>>>>>> +
>>>>>> +/// Types that can be used for module parameters.
>>>>>> +///
>>>>>> +/// Note that displaying the type in `sysfs` will fail if
>>>>>> +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`]
>>>>>> +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null
>>>>>> +/// terminator).
>>>>>> +///
>>>>>> +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE`
>>>>>> +pub trait ModuleParam: core::fmt::Display + core::marker::Sized {
>>>>>> +    /// The `ModuleParam` will be used by the kernel module through this type.
>>>>>> +    ///
>>>>>> +    /// This may differ from `Self` if, for example, `Self` needs to track
>>>>>> +    /// ownership without exposing it or allocate extra space for other possible
>>>>>> +    /// parameter values. This is required to support string parameters in the
>>>>>> +    /// future.
>>>>>> +    type Value: ?Sized;
>>>>>> +
>>>>>> +    /// Whether the parameter is allowed to be set without an argument.
>>>>>> +    ///
>>>>>> +    /// Setting this to `true` allows the parameter to be passed without an
>>>>>> +    /// argument (e.g. just `module.param` instead of `module.param=foo`).
>>>>>> +    const NOARG_ALLOWED: bool;
>>>>>
>>>>> I think, there is a better way of doing this. Instead of this bool, we
>>>>> do the following:
>>>>> 1. have a `const DEFAULT: Option<Self>`
>>>>> 2. change the type of the argument of `try_from_param_arg` to
>>>>>    `&'static [u8]`
>>>>>
>>>>> That way we don't have the weird behavior of `try_from_param_arg` that
>>>>> for params that don't have a default value.
>>>>
>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in
>>>> this patch set, it is effectively dead code. I will remove it.
>>>
>>> Hmm what parameters actually are optional? I looked at the old rust
>>> branch and only `bool` is marked as optional. Are there others?
>>>
>>> If it is used commonly for custom parameters (I could imagine that Rust
>>> modules have enums as parameters and specifying nothing could mean the
>>> default value), then it might be a good idea to just include it now.
>>> (otherwise we might forget the design later)
>> 
>> As far as I can tell from the C code, all parameters are able to have
>> the `NOARG` flag set. We get a null pointer in the callback in that
>> case.
>> 
>> If we want to handle this now, we could drop the `default` field
>> in the Rust module macro. There is no equivalent in the C macros.
>> And then use an `Option<Option<_>>` to represent the value. `None` would
>> be an unset parameter. `Some(None)` would be a parameter without a
>> value. `Some(Some(_))` would be a set parameter with a value. We could
>> probably fix the types so that only parameters with the `NOARG` flag use
>> the double option, others use a single option.
>
> What did you think of my approach that I detailed above? I would like to
> avoid `Option<Option<_>>` if we can.

How would you represent the case when the parameter is passed without a
value and a default is given in `module!`?

I think we need to drop the default value if we adopt the arg without
value scheme.

>
>> Or we could just not adopt this feature in the Rust abstractions.
>
> Depends on how common this is and if people need to use it. I think that
> what I proposed above isn't that complex, so it should be easy to
> implement.

Rust modules would just force people to add "my_module.param=1" instead
of just "my_module.param". I think that is reasonable.

>
>>>>>> +                    param_type.to_string(),
>>>>>> +                    param_ops_path(&param_type).to_string(),
>>>>>> +                );
>>>>>> +
>>>>>> +                self.emit_param("parmtype", &param_name, &param_kernel_type);
>>>>>
>>>>> Is the spelling intentional? "parmtype"?
>>>>
>>>> This is intentional. I don't think the kernel is ever parsing this, but
>>>> it is parsed by the `modinfo` tool.
>>>
>>> Hmm, why is it not `paramtype`? Does that conflict with something?
>> 
>> You would have to take that up with the maintainer(s) of the `modinfo`
>> tool. The name is externally dictated [1].
>
> Thanks for the pointer that's what I wanted to know (is it given from
> somewhere else? or is it a name that we came up with), then it's fine :)
>
>>>>>> +                            // Note: when we enable r/w parameters, we need to lock here.
>>>>>> +
>>>>>> +                            // SAFETY: Parameters do not need to be locked because they are
>>>>>> +                            // read only or sysfs is not enabled.
>>>>>> +                            unsafe {{
>>>>>> +                                <{param_type_internal} as kernel::module_param::ModuleParam>::value(
>>>>>> +                                    &__{name}_{param_name}_value
>>>>>> +                                )
>>>>>> +                            }}
>>>>>> +                        }}
>>>>>> +                    ",
>>>>>> +                    name = info.name,
>>>>>> +                    param_name = param_name,
>>>>>> +                    param_type_internal = param_type_internal,
>>>>>> +                );
>>>>>> +
>>>>>> +                let kparam = format!(
>>>>>> +                    "
>>>>>> +                    kernel::bindings::kernel_param__bindgen_ty_1 {{
>>>>>> +                        // SAFETY: Access through the resulting pointer is
>>>>>> +                        // serialized by C side and only happens before module
>>>>>> +                        // `init` or after module `drop` is called.
>>>>>> +                        arg: unsafe {{ &__{name}_{param_name}_value }}
>>>>>> +                            as *const _ as *mut core::ffi::c_void,
>>>>>
>>>>> Here you should use `addr_of[_mut]!` instead of taking a reference.
>>>>
>>>> This is a static initializer, so it would be evaluated in const context.
>>>> At that time, this is going to be the only reference to
>>>> `&__{name}_{param_name}_value` which would be const. So it should be
>>>> fine?
>>>
>>> When compiling this [1] with a sufficiently new Rust version, you will
>>> get an error:
>>>
>>>     warning: creating a shared reference to mutable static is discouraged
>>>      --> src/main.rs:4:22
>>>       |
>>>     4 |     let x = unsafe { &foo };
>>>       |                      ^^^^ shared reference to mutable static
>>>       |
>>>       = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
>>>       = note: this will be a hard error in the 2024 edition
>>>       = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
>>>       = note: `#[warn(static_mut_refs)]` on by default
>>>     help: use `addr_of!` instead to create a raw pointer
>>>       |
>>>     4 |     let x = unsafe { addr_of!(foo) };
>>>       |                      ~~~~~~~~~~~~~
>>>
>>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1
>>>
>>> So I think we should start using `addr_of!` for mutable static now.
>> 
>> Oh. Thanks for the pointer.
>> 
>> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes
>> away as well with the feature you linked as well.
>
> I think that will take some time until it is gone.
>
>> This also requires `const_mut_refs`, but as I recall that is going to be
>> stabilized soon.
>
> That should only be needed if you need `addr_of_mut!`, but IIUC, you
> only need `addr_of!`, right?

The pointer we create here is the one passed to `free` in
module_param.rs, so it will eventually be used as `&mut T`.


Best regards,
Andreas