Currently the only one consumer of LINUXKPI_PARAM_charp() is iwlwifi. I do not have the hardware so no am unable to test whether the function to load iwlwifi_nvm_file works or not. This is focusing only loader tunable feature.

There is prior work to fix that D34252

In D48941#1116247, @wulf wrote:

There is prior work to fix that D34252

Emm, I did not realize that a prior work exists. Should I continue working on this one, or wait for D34252 ?

In D48941#1116640, @zlei wrote:

Emm, I did not realize that a prior work exists. Should I continue working on this one, or wait for D34252 ?

I do not know. It looks abandoned

@kib Does the latest revision look good to you ?

It seems to be a bad idea to have CTLFLAG_WR flag for a charp sysctl handler. For example this concurrent case:

• Thread a, sysctl compat.linuxkpi.iwlwifi_nvm_file=foo
• Thread b, strcmp(iwlwifi_nvm_file, "bar"). Possible to access freed memory.

sysctlcharplock currently only prevents concurrent read / write via sysctl handler. Maybe developers should be warned ? Or should we not support CTLFLAG_WR right now ?

In D48941#1117861, @zlei wrote:

It seems to be a bad idea to have CTLFLAG_WR flag for a charp sysctl handler. For example this concurrent case:

• Thread a, sysctl compat.linuxkpi.iwlwifi_nvm_file=foo
• Thread b, strcmp(iwlwifi_nvm_file, "bar"). Possible to access freed memory.

sysctlcharplock currently only prevents concurrent read / write via sysctl handler. Maybe developers should be warned ? Or should we not support CTLFLAG_WR right now ?

I think asking devs is RW needed is not right. The correct question is whether users need RW control over some settings.
If users do need it, then perhaps some KPI to access the string is needed, instead of free to deref pointer thing.

Hi,

I think this is a lot of duplicated code from sysctl_handle_string(), which we may want to further clean-up/overhaul, for little added functionality.

Consequently, I would much prefer an approach where linux_sysctl_handle_charp() is reduced to simply allocating the backing storage for the new value in advance and just calling sysctl_handle_string(). This however requires the former to first copy the old value into that same buffer, as sysctl_handle_string() doesn't support separate input and output buffers, but I don't think performance is a real concern here. Alternatively, the cleanest solution is to extract the code of sysctl_handle_string() into a separate function that can take separate input and output buffers (and their (maximum) sizes), and have sysctl_handle_string() and linux_tunable_charp_init() call it appropriately (in this case, the latter doesn't have to copy the old string, but still has to allocate the new buffer in advance).

There should always be a reasonable limit to what the kernel wants to copy from userspace, else the malloc(M_WAITOK) can block indefinitely, resulting in an unkillable process factory at the user's discretion.

Going higher level, I wonder if we really need this "charp" part at all. Can't iwlwifi provide some buffer and just use something like module_param_string()?

If in the end folks prefer supporting "charp" for straightforward compatibility, then I think TUNABLE_CHARP() and SYSCTL_CHARP() should be available in FreeBSD native kernel code as well, and thus should go into the usual headers (sys/kernel.h and sys/sysctl.h respectively).

Thanks.

In D48941#1117878, @kib wrote:

In D48941#1117861, @zlei wrote:

It seems to be a bad idea to have CTLFLAG_WR flag for a charp sysctl handler. For example this concurrent case:

• Thread a, sysctl compat.linuxkpi.iwlwifi_nvm_file=foo
• Thread b, strcmp(iwlwifi_nvm_file, "bar"). Possible to access freed memory.

sysctlcharplock currently only prevents concurrent read / write via sysctl handler. Maybe developers should be warned ? Or should we not support CTLFLAG_WR right now ?

I think asking devs is RW needed is not right. The correct question is whether users need RW control over some settings.
If users do need it, then perhaps some KPI to access the string is needed, instead of free to deref pointer thing.

Well I'm interested in how Linux resolve this concurrent RW issue I found these in Linux source code include/linux/moduleparam.h

extern int param_set_charp(const char *val, const struct kernel_param *kp);
extern int param_get_charp(char *buffer, const struct kernel_param *kp);
extern void param_free_charp(void *arg);

and this https://github.com/torvalds/linux/blob/master/include/linux/moduleparam.h#L111 .

perm is 0 if the variable is not to appear in sysfs, or 0444
for world-readable, 0644 for root-writable, etc. Note that if it
is writable, you may need to use kernel_param_lock() around
accesses (esp. charp, which can be kfreed when it changes).

So to fully support writable ( CTLFLAG_WR ) charp sysctl knob, more work is required. Currently compat has only stub function

#define	kernel_param_lock(...) do {} while (0)

Meanwhile it appears there are limited usage of kernel_param_lock() in Linux source tree, see https://elixir.bootlin.com/linux/v6.14-rc2/C/ident/kernel_param_lock .

Given supporting read tunable ( CTLFLAG_RD | CTLFLAG_TUN ) is pretty much easy, and disabling writable in 3rd party modules requires little work, I think we can land this first. A full support writable need much more work and I think that deserves a separated review.

In D48941#1118024, @olce wrote:

Hi,

I think this is a lot of duplicated code from sysctl_handle_string(), which we may want to further clean-up/overhaul, for little added functionality.

Consequently, I would much prefer an approach where linux_sysctl_handle_charp() is reduced to simply allocating the backing storage for the new value in advance and just calling sysctl_handle_string(). This however requires the former to first copy the old value into that same buffer, as sysctl_handle_string() doesn't support separate input and output buffers, but I don't think performance is a real concern here. Alternatively, the cleanest solution is to extract the code of sysctl_handle_string() into a separate function that can take separate input and output buffers (and their (maximum) sizes), and have sysctl_handle_string() and linux_tunable_charp_init() call it appropriately (in this case, the latter doesn't have to copy the old string, but still has to allocate the new buffer in advance).

The *charp* version is special. It does not have a length info, and it is permitted to have the module assign it to a custom value, e.g. a const string "some const string". I wonder how much value to reuse sysctl_handle_string().

There should always be a reasonable limit to what the kernel wants to copy from userspace, else the malloc(M_WAITOK) can block indefinitely, resulting in an unkillable process factory at the user's discretion.

Indeed. I'm sure but it appears that the Linux's implementation limit the length of *charp* to 1024 only.

Going higher level, I wonder if we really need this "charp" part at all. Can't iwlwifi provide some buffer and just use something like module_param_string()?

Technically possible. iwlwifi want a file path to the location of firmware, and the maximum length of a path in FreeBSD is limited, so it can be converted to use module_param_string().

IMO FreeBSD's LinuxKPI emulate Linux so the ported driver requires only minimal modification. It is also easy to sync with the upstream.

If in the end folks prefer supporting "charp" for straightforward compatibility, then I think TUNABLE_CHARP() and SYSCTL_CHARP() should be available in FreeBSD native kernel code as well, and thus should go into the usual headers (sys/kernel.h and sys/sysctl.h respectively).

"charp" is not used by FreeBSD, but maybe someone want that, then it can be moved to sys/sysctl.h.

Thanks.

I'll not get involved in how the code is solved but I want people to remember that it is a compat layer and not everything will ever work 100% as upstream.

One thing to also consider is that modparams are passed kind-of with modprobe (our kldload) to my understanding but that is not how our tunable/sysctls work.
We have no ability to set these on a per-device (or cloned interface in the wifi case) which is something which has been bugging me as I have machine with multiple devices and they all get the same treatment and I cannot necessarily change the values at run-time.

How much is charp used in Linux? Hmm a lot more applicable (upstream) to drm than in wifi as otherwise I would have considered adjusting iwlwifi.

In D48941#1118313, @zlei wrote:

The *charp* version is special. It does not have a length info, and it is permitted to have the module assign it to a custom value, e.g. a const string "some const string".

Yes, but that doesn't technically preclude what I'm suggesting.

I wonder how much value to reuse sysctl_handle_string().

Well, not adding ~170 lines of mostly copy/paste code which is old and not straightforward (despite the recent simplifications you committed, which constitute a valuable progress) is pretty significant value to me.

Wrapping around sysctl_handle_string() should not take more than ~10 lines (and alleviates the need to introduce, e.g., sysctlcharplock at all).

There should always be a reasonable limit to what the kernel wants to copy from userspace, else the malloc(M_WAITOK) can block indefinitely, resulting in an unkillable process factory at the user's discretion.

Indeed. I'm sure but it appears that the Linux's implementation limit the length of *charp* to 1024 only.

Now it seems to me that there is in fact probably no general limitation mechanism at all in the sysctl machinery, I'll try to find time to investigate that at some point.

"charp" is not used by FreeBSD, but maybe someone want that, then it can be moved to sys/sysctl.h.

Yes. What I was trying to say is that I didn't really like the potential naming collision (i.e., API restricted to the Linux KPI is not prefixed), but that's a very minor point.