On 01.08.24 17:11, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>> On 01.08.24 15:40, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>>>> On 01.08.24 13:29, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <benno.lossin@xxxxxxxxx> 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 am a bit confused, there are two default values here:
(1) the value returned by `try_from_param_arg(None)`.
(2) the value given by the user to the `module!` macro.
I am talking about changing the definition of ModuleParam, so (1). I get
the feeling that you are talking about (2), is that correct?
> I think we need to drop the default value if we adopt the arg without
> value scheme.
Yes definitely. I don't see anything in the code doing this currently,
right?
We could also only allow `Copy` values to be used as Parameters (but
then `str` cannot be used as a parameter...), since they can't implement
`Drop`.
>>> 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.
Eh, why do we want to give that up? I don't think it's difficult to do.
>>>>>>> + // 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`.
Oh then the original code is definitely wrong, since it creates a shared
reference. Yeah then you should use `addr_of_mut!`.
---
Cheers,
Benno
