Message-ID: <a98ddf54-3e27-4587-8e49-f19dd1ac65a6@proton.me>
Date: Fri, 02 Aug 2024 17:11:56 +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 02.08.24 12:27, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@...ton.me> writes:
>> 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 confused myself as well I think. I am talking about (1). Let me try
> again.
> 
> If we support `NOARG_ALLOWED` (`KERNEL_PARAM_OPS_FL_NOARG` in C. I
> should change the flag name in Rust), modules can optionally support
> some parameters where users can pass parameters either as
> `my_module.param=value` or `my_module.param`. Thus, at the level of
> `try_from_param_arg`, we need to represent two cases: parameter passed
> without value, and parameter passed with value. A third case, parameter
> not passed at all, is equivalent to `try_from_param_arg` never being
> called. In C this is undetectable for the predefined parameter types. I
> wanted the double option to detect this. But I guess it does not make
> sense.

My idea was to have an `const DEFAULT: Option<Self>` to represent the
following:
(1) if `DEFAULT == None`, then `KERNEL_PARAM_OPS_FL_NOARG` is not set
    and thus either the `module!` user-specified default value is used,
    or it is specified via `my_module.param=value` and
    `try_from_param_arg` is called.
(2) if `DEFAULT == Some(d)`, then `KERNEL_PARAM_OPS_FL_NOARG` is set and
    when `NULL` is given to `kernel_param_ops.set`, the parameter value
    is set to `d`, otherwise `try_from_param_arg` is called.

But I think I agree with you removing `NOARG_ALLOWED`, see below.

> At a higher level where the bindings supply the parsing functions, we
> can decide that passing an argument without a value yields a default
> parameter value. C does this for the predefined `bool` type. The
> predefined integer types does not support omitting the value.
>
> This patch only supports the higher level predefined parameter types,
> and does not allow modules to supply their own parameter parsing
> functions. None of the types we implement in this patch support passing
> the argument without a value. This is intentional to mirror the C
> implementation.
> 
> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function
> trait to:
> 
>     fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>;
> 
> If/when we start supporting types like `bool` or custom parsing
> functions provided by the module, we will have to update the signature
> to take an `Option` to represent the case where the user passed an
> argument without a value. However, to mimic C, the function must always
> return a value if successful, even if the user did not supply a value to
> the argument.
> 
> Two different default values are in flight here. 1) the value that the
> parameter will have before the kernel calls `try_from_param_arg` via
> `set_param` and 2) the value to return from `try_from_param_arg` if the
> user did not pass a value with the argument.
> 
> For a `bool` 1) would usually be `false` and 2) would always be `true`.
> 
> For predefined types the module would not customize 2), but 1) is useful
> to customize. For custom types where the module supplies the parsing
> function, 2) would be implicitly given by the module in the parsing
> function.
> 
> In this patch set, we only have 1 default value, namely 1). We do not
> need 2) because we do not support parameters without values.

I am not sure that putting the default value of `my_module.param` into
the `ModuleParam` trait is a good idea. It feels more correct to me to
add an optional field to the part in `module!` that can be set to denote
this default value -- we might also want to change the name of
`default`, what do you think of `default_inactive` and `default_active`?

Since one might want an option by default be `true` and when one writes
`my_module.param`, it should be `false`.
Also as the C side shows, having default values for integer types is not
really a good idea, since you might want a non-zero default value.
If one does not specify the `default_active` value, then the
`KERNEL_PARAM_OPS_FL_NOARG` is not set.

If you don't want to implement this (which I can fully understand, since
we might get `syn` before anyone needs params with default values), then
we should write this idea down (maybe in an issue?). But regardless, I
would like to know your opinion on this topic.

>>> 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?
> 
> The current patch uses the default value given in the `module!` macro to
> initialize the parameter value.

But what drops the default value, when an actual value is specified via
`my_module.param=value`? (or is the default value only assigned when
nothing is specified?)

>> 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`.
> 
> We should plan on eventually supporting `String`.

Yes.

>>>>> 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.
> 
> I just don't see the point. Just have the user pass `my_module.param=1`
> instead of omitting the value. Having multiple ways of specifying for
> instance a true boolean just leads to confusion. Some boolean parameters
> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In
> this case specifying `nvme.use_cmb_sqes` has no effect, even though one
> might think it has.

This just shows to me that a "global" default in `ModuleParam` is wrong,
since for `use_cmb_sqes` one could either have a negated flag, or no
default value, forcing the user to write `nvme.use_cmb_sqes=false`.

> Of course, if we are going to do things the same as C, we have to
> support it.

I think eventually this will be useful, but as you said it's not a
feature that you *need*.

>>>>>>>>> +                            // 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!`.
> 
> Right. I agree the right thing is to change to `addr_of_mut!`. But I am
> curious. If the original code was
> 
> ```rust
> arg: unsafe {{ &mut __{name}_{param_name}_value }} as *mut _ as *mut ::core::ffi::c_void,
> ```
> 
> Then it would be fine? Because we have the only mutable reference in
> existence when the code is evaluated.

That *might* be fine, but I don't know if there is anything that would
guarantee you that it is. Note that the code above uses a shared
reference, which definitely isn't OK.

---
Cheers,
Benno

Powered by blists - more mailing lists