LinuxKPI: Rework LINUXKPI_PARAM_charp()

[zlei]

Summary

This was introduced by change [1] but never worked. SYSCTL_STRING
requires a compile-time constant pointer to a string, but
LINUXKPI_PARAM_charp pass in a pointer type char ** .

[1] c1661d59e68e LinuxKPI: add LINUXKPI_PARAM_charp()

Fixes: c1661d59e68e LinuxKPI: add LINUXKPI_PARAM_charp()
MFC after: 1 week

Test Plan

Test preloaded module,

# echo 'compat.linuxkpi.iwlwifi_nvm_file="/tmp/foo"' >> /boot/loader.conf
# echo 'if_iwlwifi_load="YES"' >> /boot/loader.conf
# shutdown -r now
...
# sysctl compat.linuxkpi.iwlwifi_nvm_file
compat.linuxkpi.iwlwifi_nvm_file: /tmp/foo

Test dynamically loaded module,

# kldunload if_iwlwifi
# kenv -u compat.linuxkpi.iwlwifi_nvm_file
# kldload if_iwlwifi
# sysctl compat.linuxkpi.iwlwifi_nvm_file
compat.linuxkpi.iwlwifi_nvm_file:

# kldunload if_iwlwifi
# kenv compat.linuxkpi.iwlwifi_nvm_file=/tmp/bar
# kldload if_iwlwifi
# sysctl compat.linuxkpi.iwlwifi_nvm_file
compat.linuxkpi.iwlwifi_nvm_file: /tmp/bar

Test with slight modified iwlwifi driver, to verify that CTLFLAG_WR works as intended.

diff --git a/sys/contrib/dev/iwlwifi/iwl-drv.c b/sys/contrib/dev/iwlwifi/iwl-drv.c
index b99204d87283..2af0bcbb9bdb 100644
--- a/sys/contrib/dev/iwlwifi/iwl-drv.c
+++ b/sys/contrib/dev/iwlwifi/iwl-drv.c
@@ -2025,7 +2025,7 @@ MODULE_PARM_DESC(amsdu_size,
 module_param_named(fw_restart, iwlwifi_mod_params.fw_restart, bool, 0444);
 MODULE_PARM_DESC(fw_restart, "restart firmware in case of error (default true)");
 
-module_param_named(nvm_file, iwlwifi_mod_params.nvm_file, charp, 0444);
+module_param_named(nvm_file, iwlwifi_mod_params.nvm_file, charp, 0644);
 MODULE_PARM_DESC(nvm_file, "NVM file name");
 
 module_param_named(uapsd_disable, iwlwifi_mod_params.uapsd_disable, uint, 0644);

# sysctl compat.linuxkpi.iwlwifi_nvm_file="/tmp/nvm_file"
compat.linuxkpi.iwlwifi_nvm_file: /tmp/bar -> /tmp/nvm_file

[zlei]

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.

[kib]

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

129

Is KMEM for having malloc working?

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

2777

This probably should be sx_sunlock(). What is the point of sysctlcharplock? What does it protect?

2850

Why reallocating and not reusing the initial alloc? Is it to save memory? I suspect it is not too much economy.

[zlei]

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

129

Is KMEM for having malloc working?

function linux_tunable_charp_init requires malloc(..., M_KMALLOC), and M_KMALLOC is defined in sys/compat/linuxkpi/common/src/linux_compat.c:

MALLOC_DEFINE(M_KMALLOC, "lkpikmalloc", "Linux kmalloc compat");

while MALLOC_DEFINE in sys/sys/malloc.h:

#ifdef _KERNEL
#define MALLOC_DEFINE(type, shortdesc, longdesc)                        \
        struct malloc_type type[1] = {                                  \
                {                                                       \
                        .ks_next = NULL,                                \
                        .ks_version = M_VERSION,                        \
                        .ks_shortdesc = shortdesc,                      \
                }                                                       \
        };                                                              \
        SYSINIT(type##_init, SI_SUB_KMEM, SI_ORDER_THIRD, malloc_init,  \
            type);                                                      \
        SYSUNINIT(type##_uninit, SI_SUB_KMEM, SI_ORDER_ANY,             \
            malloc_uninit, type)

So YES, SI_SUB_KMEM is required.

146

Is this arbitrary size large enough ?

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

2777

This probably should be sx_sunlock(). What is the point of sysctlcharplock? What does it protect?

Ahh, yes, you're right. A copy-paste bug.

2850

The initial alloc may be arbitrary large, I guess it is better to have a max length parameter in LINUXKPI_PARAM_charp()

---#define	LINUXKPI_PARAM_charp(name, var, perm)
+++#define	LINUXKPI_PARAM_charp(name, var, size, perm)

LINUXKPI_PARAM_charp() actually emulate the linux version, I'm not sure if a new parameter size is proper.

[zlei]

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

2775

Emm, missing the null terminator, but sysctl(8) tolerates this.

[zlei]

Fixed off-by-one error. Fixed sx_sunlock().

[wulf]

There is prior work to fix that D34252

[zlei]

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 ?

[wulf]

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

[zlei]

1. Avoided reallocating in function linux_tunable_charp_init().
2. Fixed retrying a snapshot
3. Fixed infinite loop ( Not sure why previous runtime test not cover that, maybe compiler is smart ? )
4. Removed max length limit.

[zlei]

@kib Does the latest revision look good to you ?

[kib]

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

2786

Why not simply srtdup() under the slock?

2904

This is not a usable name. The first six characters of the sx name are displayed for wmesg of the sleeping thread by the userspace tools like ps and top. The name should be short and pref. not contain spaces.

[zlei]

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

2786

I think you're right, sx(9) may be held during unbounded sleep, so it is safe to malloc(M_WAITOK) while holding sx(9).

2904

This is not a usable name. The first six characters of the sx name are displayed for wmesg of the sleeping thread by the userspace tools like ps and top.

Is that actually eight characters ?

% grep -r 'WMESGLEN' sys/sys
sys/sys/user.h:#define	WMESGLEN	8		/* size of returned wchan message */
sys/sys/user.h:	char	ki_wmesg[WMESGLEN+1];	/* wchan message */

# ps -alx
 UID   PID  PPID C PRI NI   VSZ   RSS MWCHAN   STAT TT        TIME COMMAND
...
   0     3     0 2 -16  0     0    80 crypto_w DL    -     0:00.00 [crypto]
...

The name should be short and pref. not contain spaces.

What about this one,

---	sx_init(&sysctlcharplock, "linux sysctl charp handler");
+++	sx_init(&sysctlcharplock, "lkpi-sysctl-charp-handler");

The short version "lkpi-sys" has not been used in sys/compat/linuxkpi yet.

[zlei]

1. Changed description of sysctlcharplock to "lkpi-sysctl-charp-handler".
2. Avoid retrying snapshot.

[zlei]

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 ?

[kib]

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.

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

2762

slock/sunlock can be moved out of if() branches over the whole if.

[olce]

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.

[zlei]

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.

[zlei]

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.

[bz]

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.

[olce]

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.