Message-ID: <87zfpvmd8y.fsf@metaspace.dk>
Date: Fri, 02 Aug 2024 10:27:07 +0000
From: Andreas Hindborg <nmi@...aspace.dk>
To: Benno Lossin <benno.lossin@...ton.me>
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

"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.

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 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.

> 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`.

>
>>>> 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.

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

>
>>>>>>>> +                            // 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.


Best regards,
Andreas

Powered by blists - more mailing lists