"Alice Ryhl" <alicer...@google.com> writes:
> On Fri, May 02, 2025 at 02:16:35PM +0200, Andreas Hindborg wrote:
>> Add support for module parameters to the `module!` macro. Implement read
>> only support for integer types without `sysfs` support.
>>
>> Acked-by: Petr Pavlu <petr.pa...@suse.com> # from modules perspective
>> Tested-by: Daniel Gomez <da.go...@samsung.com>
>> Signed-off-by: Andreas Hindborg <a.hindb...@kernel.org>
>
>> +unsafe extern "C" fn set_param<T>(
>> + val: *const kernel::ffi::c_char,
>> + param: *const crate::bindings::kernel_param,
>> +) -> core::ffi::c_int
>> +where
>> + T: ModuleParam,
>> +{
>> + // NOTE: If we start supporting arguments without values, val _is_
>> allowed
>> + // to be null here.
>> + if val.is_null() {
>> + // TODO: Use pr_warn_once available.
>> + crate::pr_warn!("Null pointer passed to `module_param::set_param`");
>> + return EINVAL.to_errno();
>> + }
>> +
>> + // SAFETY: By function safety requirement, val is non-null and
>> + // null-terminated. By C API contract, `val` is live and valid for reads
>> + // for the duration of this function.
>> + let arg = unsafe { CStr::from_char_ptr(val) };
>> +
>> + crate::error::from_result(|| {
>> + let new_value = T::try_from_param_arg(arg)?;
>> +
>> + // SAFETY: `param` is guaranteed to be valid by C API contract
>> + // and `arg` is guaranteed to point to an instance of `T`.
>> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T };
>> +
>> + // SAFETY: `old_value` is valid for writes, as we have exclusive
>> + // access. `old_value` is pointing to an initialized static, and
>> + // so it is properly initialized.
>> + unsafe { core::ptr::replace(old_value, new_value) };
>
> You don't use the return value of this, so this is equivalent to
> unsafe { *old_value = new_value };
Thanks.
>
>> +macro_rules! make_param_ops {
>> + ($ops:ident, $ty:ty) => {
>> + ///
>> + /// Static [`kernel_param_ops`](srctree/include/linux/moduleparam.h)
>> + /// struct generated by `make_param_ops`
>> + #[doc = concat!("for [`", stringify!($ty), "`].")]
>> + pub static $ops: $crate::bindings::kernel_param_ops =
>> $crate::bindings::kernel_param_ops {
>> + flags: 0,
>> + set: Some(set_param::<$ty>),
>> + get: None,
>> + free: Some(free::<$ty>),
>
> You could potentially only include `free` if
> `core::mem::needs_drop::<T>()` as an optimization.
Right, nice 👍
>
>> + fn emit_params(&mut self, info: &ModuleInfo) {
>> + let Some(params) = &info.params else {
>> + return;
>> + };
>> +
>> + for param in params {
>> + let ops = param_ops_path(¶m.ptype);
>> +
>> + // Note: The spelling of these fields is dictated by the user
>> space
>> + // tool `modinfo`.
>> + self.emit_param("parmtype", ¶m.name, ¶m.ptype);
>> + self.emit_param("parm", ¶m.name, ¶m.description);
>> +
>> + write!(
>> + self.param_buffer,
>> + "
>> + pub(crate) static {param_name}:
>> +
>> ::kernel::module_param::ModuleParamAccess<{param_type}> =
>> +
>> ::kernel::module_param::ModuleParamAccess::new({param_default});
>
> Is this global accessible to the user?
Yes.
> It would be a use-after-free to
> access it during module teardown. For example, what if I access this
> static during its own destructor? Or during the destructor of another
> module parameter?
Yes, that is a problem.
We can get around it for now by just not calling `free` for now. We only
support simple types that do not need drop. I think we would have to
seal the `ModuleParam` trait for this.
For a proper solution, we could
- Require a token to read the parameter.
- Synchronize on a module private field and return an option from the
parameter getter. This would require module exit to run before param
free. I think this is the case, but I did not check.
- Use a `Revocable` and revoke the parameter in `free`.
Any other ideas or comments on the outlined solutions?
Best regards,
Andreas Hindborg
