Message-ID: <f84e9189-b64a-4761-86f5-ccd50fb62f36@proton.me>
Date: Thu, 01 Aug 2024 19:28:50 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <nmi@...aspace.dk>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, rust-for-linux@...r.kernel.org, linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org, Andreas Hindborg <a.hindborg@...sung.com>, Adam Bratschi-Kaye <ark.email@...il.com>
Subject: Re: [PATCH] rust: add `module_params` macro

On 01.08.24 17:11, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@...ton.me> writes:
>> On 01.08.24 15:40, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@...ton.me> writes:
>>>> On 01.08.24 13:29, Andreas Hindborg wrote:
>>>>> "Benno Lossin" <benno.lossin@...ton.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 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

Powered by blists - more mailing lists