Page MenuHomeFreeBSD

LinuxKPI: add proper support for charp module parameters
Needs ReviewPublic

Authored by val_packett.cool on Feb 11 2022, 4:51 PM.
Referenced Files
Unknown Object (File)
Mon, Mar 11, 5:23 PM
Unknown Object (File)
Jan 2 2024, 12:49 PM
Unknown Object (File)
Jan 2 2024, 12:49 PM
Unknown Object (File)
Jan 2 2024, 12:49 PM
Unknown Object (File)
Jan 2 2024, 12:49 PM
Unknown Object (File)
Jan 2 2024, 12:49 PM
Unknown Object (File)
Jan 2 2024, 12:49 PM
Unknown Object (File)
Jan 2 2024, 12:40 PM

Details

Reviewers
manu
hselasky
Group Reviewers
linuxkpi
Summary

sysctl_handle_string does not actually work on a double pointer.
Implement a handler that manages a dynamically allocated string instead of a static one.

This is required for hw.amdgpu.virtual_display.

Theoretically this code should even support read-write sysctls, but we only have boot-time-only tunables in amdgpu.

Related to: https://github.com/freebsd/drm-kmod/issues/134


BTW, can someone please document what exactly are the "indexes" (oldidx/newidx) in the sysctl req struct? sysctl_handle_string fiddles with them but I don't understand why.

Test Plan

I have only tested with amdgpu's virtual_display tunable.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

val_packett.cool edited the summary of this revision. (Show Details)
val_packett.cool edited the test plan for this revision. (Show Details)
val_packett.cool added a project: linuxkpi.

add missing + 1 for strlen

sys/compat/linuxkpi/common/include/linux/moduleparam.h
139–141

Style:
Function prototype should go after the macros:

extern int lkpi_sysctl_handle_charp(SYSCTL_HANDLER_ARGS);

I also like to use extern, though some see it redundant.

sys/compat/linuxkpi/common/src/linux_compat.c
120

Just use the Linux allocator, kalloc(GFP_KERNEL) ?

The function is very small and leaks are impossible!

wulf added inline comments.
sys/compat/linuxkpi/common/src/linux_compat.c
2644

Take this lock exclusively right here. Otherwise SYSCTL_IN and SYSCTL_OUT gets a great chance to see different versions of *charp when try_upgrade is failed

2647

SYSCTL_OUT can sleep. You should call sysctl_wire_old_buffer() before taking lkpi_charp_lock

2669

You cannot do malloc(M_WAITOK) with non-sleepable lock held.

2673

How these operations on *charp are synchronized with drm-kmod? Are they protected by external lock?

It looks that sx lock suits our needs better than rw here.

sys/compat/linuxkpi/common/src/linux_compat.c
2673

In the kmods we have, charp parameters are only used for boot-only tunables. I'm not sure if they even make sense with things that can change at runtime. There is no synchronization, the module just expects the kernel to modify the pointer on load and reads it when initializing the hardware.

sys/compat/linuxkpi/common/include/linux/moduleparam.h
110

var must be freed on module unload

sys/compat/linuxkpi/common/src/linux_compat.c
2656

IMO charp should be freed only after successfull SYSCTL_IN(). I think *charp should not be reseted after e.g. EFAULT

2673

Ok. It looks that drm-kmod is awared of absence of synchronization and does strdup() before parsing of data referenced by charp.

sys/compat/linuxkpi/common/src/linux_compat.c
2656

hmm well, I put it here because if (req->newlen == 0) I replace it with NULL without doing SYSCTL_IN

  • rebase
  • add SYSUNINIT for deallocating the strings
  • remove useless null check (kfree supports null)
sys/compat/linuxkpi/common/include/linux/moduleparam.h
107–110

This kfree() looks incorrect. Should be kfree(var); ???