Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 60251 Build 57135: arc lint + arc unit
Event Timeline
This is the followup of https://reviews.freebsd.org/D47107.
I intentionally omitted the CTLFLAG_PRISON for these two sysctl during their introduction due to the docs say that such sysctl can be written to by processes in jail(2). As long as this couple is read-only I did not add the flag.
For now, after a quick code overview, it looks that this flag is not about writable, it's about "value per jail". I think that we could tune the docs and actually add this flag for a bunch of existing sysctl. A very quick counting on my side yields 18 of them like .jailed, .vnet, etc.
This very short patch is a discussion starter, I can extend it to cover more sysctl and the docs as well. What do you think?
Another point is how to deal with sysctl which are "per jail" and vnet-related as well. I guess, they could end up with both flags?
sysctl: Add missing CTLFLAG_PRISON to security.jail.children.*
@igoro The subsystem should be jail, so jail: Add missing CTLFLAG_PRISON to security.jail.children.* . Also it is important to explain why.
Also it is better to add commit meta message
Fixes: ab0841bdbe84 jail: expose children.max and children.cur via sysctl
I guess no. Actually a VNET instance can be shared between multiple jail prisons, e.g., legacy jails those share vnet0.
I like the idea of flagging sysctls that hold values that can be read per jail. If jail(2) says otherwise, we can change that to match.
I agree with your code review.
I like the idea. The only inconvenience I see is that this flagging has to be done by hand, and there is no real automatic way to prevent wrong flagging with CTLFLAG_PRISON (e.g., conditional on the use of curthread->td_cred->cr_prison), but these are very minor, and certainly not worse than the current situation.
This hasn't been done before simply because the CTLFLAG_PRISON was never actually checked if not trying to write to the corresponding sysctl. Now that D47107 has been committed, this change would make a useful difference IMO.