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

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

Details

Reviewers

• hselasky
• ian
jhb
julian
kib

Commits

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

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 - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 11842
Build 12180: arc lint + arc unit

Event Timeline

avg created this revision.Sep 29 2017, 2:45 PM

Herald added a subscriber: imp. · View Herald TranscriptSep 29 2017, 2:45 PM

Harbormaster completed remote builds in B11814: Diff 33567.Sep 29 2017, 2:45 PM

• hselasky added inline comments.Sep 29 2017, 2:56 PM

sys/kern/kern_sysctl.c
494	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?
527	It might make sense to have: sysctl_disable_oid() and use that to disable oid before calling sysctl_register_oid_impl() --HPS

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

julian added a comment.Sep 30 2017, 6:44 PM

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	That's a very good suggestion, I will implement it.
527	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?