sysctl-s in a module should be accessible only when the module is initialized
ClosedPublic

Authored by avg on Sep 29 2017, 2:45 PM.

Details

Summary

A sysctl can have a custom handler that may access data that is initialized
via SYSINIT(9) or via a module event handler (also invoked via SYSINIT).
Thus, it is not safe to allow access to the module's sysctl-s until
the initialization is performed. Likewise, we should not allow access
to teh sysctl-s after the module is uninitialized.
The latter is easy to achieve by properly ordering linker_file_unregister_sysctls
and linker_file_sysuninit.
The former is not as easy for two reasons:

  • the initialization may depend on tunables which get set when sysctl-s are registered, so we need to set the tunables before running sysinit-s
  • the initialization may try to dynamically add more sysctl-s under statically defined sysctl nodes

So, this change splits the sysctl setup into two phases. In the first phase
the sysctl-s are registered as before but they are disabled and hidden from
consumers. In the second phase, done after sysinit-s, normal access to the
sysctl-s is enabled.

The change should affect only dynamic module loading and unloading after
the system boot-up. Nothing changes for sysctl-s compiled into the kernel
and sysctl-s in preloaded modules.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
avg created this revision.Sep 29 2017, 2:45 PM
hselasky added inline comments.Sep 29 2017, 2:56 PM
sys/kern/kern_sysctl.c
494 ↗(On Diff #33567)

Just a question about code factoring. Why is not CTLFLAG_DORMANT set by the wrapper before calling this function? Would save passing in "enable" argument to this function?

538 ↗(On Diff #33567)

It might make sense to have:

sysctl_disable_oid() and use that to disable oid before calling sysctl_register_oid_impl()

--HPS

kib accepted this revision.Sep 30 2017, 8:15 AM

I am fine with this approach. After you answer the Hans' questions, I do not see anything wrong.

This revision is now accepted and ready to land.Sep 30 2017, 8:15 AM
This comment was removed by julian.
julian accepted this revision.Sep 30 2017, 6:45 PM
avg added inline comments.Oct 2 2017, 2:16 PM
sys/kern/kern_sysctl.c
494 ↗(On Diff #33567)

That's a very good suggestion, I will implement it.

538 ↗(On Diff #33567)

Yes, that might make sense, thank you for the suggestion.
But I am somewhat reluctant to introduce a small internal function that has only a single consumer at this time.
I'll postpone this suggestion until the time when there is more than one use for sysctl_disable_oid.
Is that okay with you?

avg updated this revision to Diff 33632.Oct 2 2017, 2:23 PM

implement sysctl_register_disabled_oid as a thin wrapper around sysctl_register_oid,
so that the latter does not need to be modified at all

This revision now requires review to proceed.Oct 2 2017, 2:23 PM
hselasky added inline comments.Oct 2 2017, 2:31 PM
sys/kern/kern_sysctl.c
538 ↗(On Diff #33567)

Sure.

avg updated this revision to Diff 33675.Oct 4 2017, 7:32 AM

enable sysctl-s only if the module file doesn't fail to load

julian accepted this revision.Oct 5 2017, 4:05 AM
This revision is now accepted and ready to land.Oct 5 2017, 4:05 AM
This revision was automatically updated to reflect the committed changes.