Message-ID: <ed2f7416-2631-411d-bb49-5a580dbf51b8@proton.me>
Date: Thu, 01 Aug 2024 14:21:13 +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 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.

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

>>>>> +                    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?

---
Cheers,
Benno

Powered by blists - more mailing lists