In D47332#1082956, @igoro wrote:

If we add CTLFLAG_PRISON to such sysctlS it will add some mess to the existing interface and behavior, and can add headache for future code. Having such flag added to these variables makes a feeling they can be changed but instead of a correct message that "Operation not permitted" we get something like this:

root@somejail:/ # sysctl security.jail.set_hostname_allowed=1
security.jail.set_hostname_allowed: 0 -> 0

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.

In D47107#1078238, @zlei wrote:

This change is focusing on VNET variables, but is open for -J ( CTLFLAG_PRISON ) if requested.

In D47027#1075329, @markj wrote:

But, the non-null-checked use of the pointer is harmless, as nothing actually dereferences it. Because pr_ip is an array, the value passed to vfs_setopt_part() is the value of the pointer plus the offset of the array within the structure being pointed to.

Sorry I'm late to the game. This actually seems like something should go in. The null dereference was introduced in 500f82d6c32ed, replacing earlier code that was unintuitive but notably didn't need to dereference that pointer. Unfortunately, the fix didn't include a null check, leading to a statement that has one non-null-checked use of the pointer, followed immediately by a properly checked use (because that one predated 500f82d6c32ed).

In D46899#1070761, @olce wrote:

In D46899#1070434, @jamie wrote:

Currently, it seems to suggest (being a jailsys parameter) that there's some sort of valid "new" or "deleted" state for MAC inside the jail.

Not sure what you mean here. SYSCTL_JAIL_PARAM_NODE() just declares the common MAC sub-node. mac_do(4) will then indeed use the new SYSCTL_JAIL_PARAM_SYS_SUBNODE() for the mac.do jail parameter "node", effectively intended to be a jailsys one.

It would make sense for the exec.clean parameter to apply to the config execution. Bit of a chicken and egg problem there, but there's still the "-l" flag.

I suppose this will make more sense once there's at least one MAC policy that has an associated jail parameter. Currently, it seems to suggest (being a jailsys parameter) that there's some sort of valid "new" or "deleted" state for MAC inside the jail.

This comes from discussion in Bug 277210.

In D45647#1041512, @zlei wrote:

I guess the change for sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vfsops.c go to upstream first. Will it ?

Is there any value in a virtual time, i.e. letting a jail have its own clock? Apart from a test framework, I can't think of any, but maybe someone else can. That was actually my first thought when I saw the title of this, rather than encapsulating ntpd.

Diff updated for libsys and other recent changes.

Yes, this is a better way.

I'll admit very little familiarity with the testing framework. If there's a standard to show a test as skipped that doesn't indicate a problem, that sounds best. But if it just claims that it's passed, it would seem better to just not run it. Though both is probably a good idea: still have it pass (like if the test was built at another time), but don't build it on a jail-less system.

Here's the latest diff to address concerns so far (except those that request proper documentation).

In D43696#996617, @bz wrote:

Also, can we please have a man page?

Sure, looks helpful for just such a situation.

Looks good to me!

c) Or something else what I have not spotted yet :)

c) Jamie wasn't thinking and of course you don't need it for read-only.

You'll want to add CTLFLAG_PRISON to the sysctl flags.

In D43476#992839, @igor.ostapenko_pm.me wrote:

@jamie, does it look as an acceptable feature to introduce security.jail.children.max RO sysctl which reflects the current prison's p_childmax? If it does then I will help to implement it.
P.S. Probably you know existing "legal" ways to retrieve current prison's children.max w/o additional code to introduce.

I'm not sure why this limit exists in the first place (it predates me). I suppose it's just for neatness' sake, with the idea no one would have more jails than that anyway. But as long as it's around, it might as well be known.

That's fine if there's a use for it, such as to quiet errors.

In D42672#973897, @rmacklem wrote:

This version of the patch acquires a shared lock on
allprison_lock (which looks sufficient to ensure the
jail does not go from alive to dying.
It also acquires pr_mtx to check for prison_isalive(),
although I am not 100% sure it is needed?

In D42672#973775, @rmacklem wrote:

In D42672#973753, @markj wrote:

Since vfs_exjail_delete() is called when there are no processes running in the jail, a caller of vfs_exjail_clone() cannot be running in that jail.

It's not clear to me that the first part is true. In particular, prison_deref() first calls prison_deref_kill(), which calls prison_cleanup() and thus vfs_exjail_delete(); later it kills processes within the jail.

Hmm, unless I'm missing something, this is at odds with the comment in vfs_exjail_delete(). Perhaps we need to check the prison state when setting mnt_exjail? i.e., refuse to set it if the jail is dying.

Well, the comment on vfs_exjail_delete() states that no processes are in the
prison, but maybe the comment is bogus?
Anyhow, I think this version might be ok, but hopefully you or jamie@ can
confirm this?

My understanding (which could be wrong) is that a prison cannot go from
active to dying when there is a p_uref held on it. If that is the case, then
prison_proc_hold()/prison_proc_free() should ensure that it remains alive
until after the cloning is done, I hope?

The command parameters (including "command" itself) are well established as being run during jail setup and teardown. I would expect a lot of existing configuration to have problems with the command being run when the jail has already started up. While it makes sense on the command line, I don't want to break the connection between file configuration and command line configuration (more than it's already broken).

Nothing new, though I just updated the diff against the latest sources.

The important part of this clearly good.

Looks good to me. But then the original that did the direct cr_uid check looked apparently good to me too, so take it for what it's worth.

Commited in eb5bfdd06565. I forgot to add the review to the commit message :-/

I've committed the "jails can include jails" and "use the recursive parser" bits separately. This new diff is just the part that handles the includes.

In D40188#919081, @crest_freebsd_rlwinm.de wrote:

Just a small nitpick: I would prefer a macro #define MAX_INCLUDE_DEPTH 32