Commit 66d8ffe30 has simpler kevent handling for jaildesc, without any recursion. Jail kevents have also have recussion removed. Its lack of guarantees and incomplete problem-solving meant that applications would need a way to handle when notifications weren't 100% collected. As long as that's the case, might as well get rid of the complexity.

Commit 66d8ffe30 has simpler kevent handling for jaildesc, without any recursion. Jail kevents have also have recussion removed. Its lack of guarantees and incomplete problem-solving meant that applications would need a way to handle when notifications weren't 100% collected. As long as that's the case, might as well get rid of the complexity.

Very well. I suppose it doesn't hurt.

Plan B is in D52462.

Plan A is in D52461.

Yes, EPERM on open directories may be unexpected, but I don't see that is enough cause to change the long-standing tradition of referring to other manual pages when a function may return the set of errors produced by another function.

I've added a "Jail Descriptors" section to jail(2), and added the jail_attach_fd and jail_remove_jd system calls, and the new jail_get/jail_set flags.

The unrelated prison_add_allow talk deserves its own revision, so I made D52105 for that.

In D52039#1189578, @markj wrote:

I wonder why asprintf() uses M_NOWAIT when strdup() uses M_WAITOK. Looking at the relatively few callers, I suspect we can perhaps just switch to M_WAITOK...

prison_add_vfs then goes on to call prison_add_allow, which has its own asprintf call. So unless you fix that as well, you've bought very little. And prison_add_allow really ought to be fixed. It's just a matter of not relying on asprintf with its M_NOWAIT allocation. The context it's called in (and presumably would ever be called in) can handle waiting for allocation.

I've added D51940, which adds kqueue support to jails by JID, so it's separate from the descriptor work, except it contains fixes for the problems I've identified with the current kqueue code in this revision. In particular, it uses the NOTE_TRACK convention that processes use, and adds a flag that notes if attached processes were missed.

Yep, tried it out and it looks good now.

As I learn more about kqueue, I see I am trying to make it do things it's not suited for. In particular, the data field can't contain the kind of information I'm putting it in (process ID, jail ID). The non-queuing nature of kqueue (!) means that one event can be obliterated by the next event, leading to unreliable notification. There's not a lot of advantage of being notified that at least one child jail has been created, and here's the JID of the most recent one.

True, there's no real need for the PR_ALLOW_PRISON0 change, but since you went to the effort to make that macro anyway, it's a good place to showcase it.

It looks fine from the jail side of things.

In D51645#1179838, @jamie wrote:

In the meantime (and long-term), a knob makes sense.

In D51645#1179733, @kib wrote:

Would privileges actually work, I have no objections. But right now this change makes the very useful feature (at least for me), only available to root. I do often use 'jail -u <me> / something 127.0.0.1 /bin/sh', and have the jailed processes only bound to localhost, otherwise they are normal (can be debugged etc).

It would be pity to loose the ability. Can we have at least a knob to re-enable the current behavior?

I like this. Given that a non-root user isn't allowed to mess with a jail, it makes that it wouldn't be allowed to mess with processes in that jail, even if they happen to have the same uid. That sounds like preferable default behavior, even if it's a switch from current practice. The closer we get to namespaces like uid being conceptually a (jail, id) tuple, the better off we are.

In D43696#1179565, @crest_freebsd_rlwinm.de wrote:

In D43696#1179550, @jamie wrote:

This sounds deeply confused about what a file descriptor is (supposed to be). It is the capability to use the referenced resource. The permissions in the file description should govern what's allowed through the descriptor not the current process credentials. The check against the current process credential has to happen as the description the descriptor references is created.

In D43696#996669, @dvl wrote:

I read the description, but I'm still not sure how a jail descriptor would be used. How about some pseudo-code, to illustrate the concept please?

The big addition is kevent support. A jail descriptor can pass four notes:

In D51524#1176869, @markj wrote:

I could instead add an alternative function or even just an inline "is PR_VNET set" check.

There's a general trend in the "prison can do this thing" functions that they all take a ucred. One the one hand it seems reasonable that they would all take a struct prison instead, but is there a particular reason to break the trend for this case?

I had considered that the -l (exec clean) flag should be considered, but decided it really only makes sense for keeping the jail environment clean.

Yes, anything that makes these things more obviously deprecated is better.

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.