I appreciate the single vfs_lookup_isroot function - it took me an embarrassingly long time to discover a second copy of that code had appeared in vfs_cache.c a mere five years ago.

In D50371#1150131, @olce wrote:

@jamie : Was the possibility of having a sub-jail with a root that is not under its parent jail something that was explicitly meant to be allowed from the start? Aren't all the other jail-managed resources not allowed to grow when going down the jail tree (for those where that makes sense)? I agree there is currently no mechanism to actually enforce that some directory stays below another one, but maybe we can search for some practical ones if we consider this to be really relevant. Providing such a guarantee could ease general reasoning about whether some paths are contained in others. But that doesn't seem to make a big difference here: With such a property, we could allow simply passing any descriptor from some sub-jail to a parent jail, but it would still be too weak by itself to allow a parent jail to unconditionally send a descriptor to its children, or have two siblings exchanging one. If you have some pointers or remembrances about restrictions on the root of sub-jails, I would be interested.

In D50371#1149684, @jamie wrote:

I would widen this a little, and allow passing dirfds up and down the jail parent/child chain. That fits with the current ".."-handling strategy of preventing traversal up from the current jail or any ancestor.

In D50371#1149495, @kib wrote:

IMO the right way is much simpler, and was formulated in the PR: only allow to pass dirfd across the same jail, or from any jail to prison0. I think your addition of allowing to pass dirfd between different jails but having same rootvp is neat.

In D49843#1138751, @zec wrote:

If this patch hits the tree, which I'm strongly opposed to, at minimum the commit message should clearly state that this is an open-ended redefinition of the jail contract, for a cause no one is able to articulate clearly.

No opinion on slew vs skew, the rest looks good.

This looks good to me, but I'm hoping someone from networking will chime in.

It looks very basic, and is good by me. But I'm inviting Mariusz to make sure he didn't have a particular reason not to include PRIV_SETTIMEOFDAY when he added PRIV_CLOCK_SETTIME.

OK, I understand the problem with the maximum sizes. I think we need to pursue something in libjail that makes this seamless, but that can be later work.

I'm not sure what the point of the soft/hard size limits is. I imagine I could find it in the code, but could you explain it to me? Something more than the one comment about it, but less detail than the code itself :-).

I don't disagree that the way the character set is limited is efficient and doesn't add much to the code base. It's just the entire direction that I don't like. There are many places where sloppy text interpretation can cause problems. Environment variables come to mind - they too are passed between processes in the kernel. But user-level string injection has never been considered a kernel problem, and I don't see why it should start now. That's just not the kernel's job.

Everything looks good, except that allowed chars part. That still looks like a kernel-level solution to a user-level non-problem.

In D47668#1119805, @igoro wrote:

In D47668#1115164, @jamie wrote:

Here's a possibility:

Saying meta.foo="" on the command line or jail.conf clears the contents of meta.foo, or adds it with empty contents. The jail_set(2) interface passes an empty string for the value. This already works.

Just saying meta.foo (no '=') on the command line or jail.conf removes the key if it exists. The jail_set(2) interface passes a null string for the value, which is typically an error.

Thank you for confirming this way, it seems more usable instead of meta.nofoo or nometa.foo. The latest patch version adds key removal using NULL value as a trigger.

In D47668#1115164, I wrote:

There's no way to remove a key, only to add or set one. I can set it to an empty string, but they key is still there. I suppose that makes sense that setting a key to an empty value creates a key with an empty value, but I'd like a way to remove a key, short of reading the whole thing and writing back everything except the unwanted key. But I don't know a good way to specify that only working with a string value.

A few observations from actually running this:

No further comments, except sorry for taking so long to mention that.

In D47992#1097186, @markj wrote:

The new PR_VNET_ROOT flag is unneeded: it's equivalent to PR_VNET, just check
that instead.

There are currently a wide variety of strings in jail parameters, and for that matter many more elsewhere in the kernel. So far, we have gotten by with counting on administrators putting reasonable values in them.

Accepted with the caveat I mentioned in D48074.

I'd prefer it to take the slot number, and do the "slot - 1" translation in the macro. That came to mind looking at D48075 which passes in slot - 1.

In D47668#1095796, @igoro wrote:

• The allowed chars for each buffer are very limited by default, it covers Base64, k=v\n format, and some extra bytes. It can be changed via security.jail.meta_allowedchars sysctl. For convenience (as it seems to me for now), setting it to an empty string allows everything.

Longer term, I'd like to push some of vnet removal into the period between prison_cleanup() and the final jail destruction; interface removal is definitely part of that desire. But that's a mess of dependencies that hasn't found a solution yet, so I can see how we need this.

I'm surprised the wrong behavior existed for this long without being noticed. I guess that shows how little-used kill(-1) is in usual operation.

In D47668#1089529, @igoro wrote:

Yeah, it seems that on the Jail Call of 26-Nov we came to a conclusion that for now we would keep it very simple like two buffers per jail managed from the user-land side, while keeping a wide spectrum of opportunities to extend it in the future having a more specific production need in mind. Thus, we can postpone thinking about extra complexity on the kernel side.

In D47668#1087001, @igoro wrote:

• The current state of the patch allows reading meta by any user within a jail. Do we want to disable it by default and add something like allow.read_meta or allow.metadata parameter to control it per jail?

In D47668#1087018, @crest_freebsd_rlwinm.de wrote:

A flat list of key=value pairs (instead of a single value) would allow multiple users of this feature per jail e.g. multiple helper commands preparing just one aspect of a jail e.g. dynamic devfs ruleset loading, network setup/teardown, storage provisioning.

Not really part of this specific problem, but jail(8) should auto-recognize list parameters because their SYSCTL_JAIL specification has a type with ",a" appended. Ideally, it would build a list of such parameters it sees like that. For non-kernel parameters like vnet.interface and zfs.dataset, putting them in the array is probably as good as its going to get.

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.