Kernel modules can add sysctl entries. If the user wishes to change the default value of those sysctls,= using sysctl.cont, they must load the kld before rc.d/sysctl runs. In practice this means loading it at boot time via /boot/loader.conf even though loading later in the boot process via rc.conf's kld_list variable is more efficient. Adding 'REQUIRE: kld' to rc.d/sysctl would make the ordering of these two scripts deterministic and allows sysctl.conf to manage sysctl variables for kernel modules loaded via kld_list.
note that kld depends on kldxref, which (optionally) *uses* sysctl to obtain `kern.module_path`. But, that's probably not something anyone should be setting in sysctl.conf. if one wants to modify that path, they can do so in rc.conf via `kldxref_module_path`.
Created attachment 242919 [details] Make sysctl require kld
I don't the use of kern.module_path is a problem - this variable is exported by the kernel linker itself and cannot be in a loadable module.
(In reply to dfr from comment #2) please note that the project has been trying to discourage submitting patches on bugzilla: https://docs.freebsd.org/en/articles/contributing/ GitHub and Phabricator are the preferred venues
(In reply to dfr from comment #3) I think the concern is that someone could set kern.module_path via sysctl.conf in order to control the search path used by kld_list?
As Mina already noted, setting kldxref_module_path is a better way of controlling the search path so I don't think this is a problem. IMO, the violation of POLA where a user adds a module to kld_list but cannot reliably manage sysctl variables is a worse issue. I know I wasted too much time debugging that on one of my systems yesterday.
I tried to fix this some time ago, but ran into objections: https://reviews.freebsd.org/D25601 Applying your patch to an installed system and running rcorder etc/rc.d/* yields: rcorder: Circular dependency on file `zpool'. rcorder: Circular dependency on provision `hostid': hostid -> sysctl -> kld -> kldxref -> mountcritlocal -> zpool -> hostid. rcorder: `zpool' was seen in circular dependencies for 1 times. rcorder: Circular dependency on provision `hostid': hostid -> sysctl -> kld -> kldxref -> mountcritlocal -> hostid_save -> hostid. rcorder: `hostid_save' was seen in circular dependencies for 1 times. rcorder: `mountcritlocal' was seen in circular dependencies for 2 times. rcorder: `kldxref' was seen in circular dependencies for 2 times. rcorder: `kld' was seen in circular dependencies for 2 times. rcorder: `sysctl' was seen in circular dependencies for 2 times. rcorder: `hostid' was seen in circular dependencies for 2 times. rcorder: `zpool' was seen in circular dependencies for 1 times. rcorder: Circular dependency on file `zfskeys'.
Hmm - clearly my simplistic solution is not going to work well. I liked Alan Jude's suggestion in the linked review which adds /etc/sysctl.conf.d for module-specific values.
I'm really confused about @imp's comment here > I don't think this is a good idea.... > > Setting sysctl values multiple times may be unwise. > > Most modules should be loaded in the loader, and most config of modules should be done via tunables, not sysctls. "most modules should be loaded in loader" feels counter to what I've read in rc.conf and probably also loader.conf
I just created https://reviews.freebsd.org/D40886 which implements Allan Jude's suggestion from https://reviews.freebsd.org/D25601
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=09267cc15284795fef958fb9ed786bb2382d6763 commit 09267cc15284795fef958fb9ed786bb2382d6763 Author: Doug Rabson <dfr@FreeBSD.org> AuthorDate: 2023-06-21 12:26:17 +0000 Commit: Doug Rabson <dfr@FreeBSD.org> CommitDate: 2023-07-14 09:49:47 +0000 /etc/rc.subr: add support for kld sysctl variables For kernel modules loaded by scripts in /etc/rc.d and /usr/local/etc/rc.d, if there is a file in /etc/sysctl.conf.d named <kld name>.conf, then this will be loaded using the sysctl(8) utility. For instance, sysctl variable changes for the pf kernel module would be placed in the file /etc/sysctl.conf.d/pf.conf. PR: 272129 Reviewed by: imp freebsd_igalic.co MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D40886 etc/mtree/BSD.root.dist | 2 ++ libexec/rc/rc.subr | 3 +++ share/man/man5/sysctl.conf.5 | 10 ++++++++++ 3 files changed, 15 insertions(+)