Fix variable declaration order.

Clean up prison_attach_thread_single a bit so I don't need to different PROC_UNLOCK calls.

Move thread_single calls and related code inside a well-commented
wrapper function. Add atomic_load to the process flag read. Fix
tests so they can run in parallel, other small fixes.

Add a test for races between jail_attach_jd and chroot. Let all the tests run instead of stopping on the first failure.

I altered the patch to use an sx lock instead of thread single, and it passed the test as expected. But then I added a test for a race between jail_attach_jd and chroot, and it failed.

That's why it's EPERM to jail_attach with any directory fds open. But a concurrent chdir/chroot could work around that, e.g. if the jail_attach happens between path lookup and pwd_chroot().

I'll have to think a little on the chdir question. I considered it and chroot briefly, but only after I decided on single threading anyway, and I figured they weren't a problem because of the boundary requirement. Without that, it seems likely that a jail_attach in parallel with a chroot would have the same outcome.

I could do that, but I actually considered single threading to be the smaller hammer, because it only affects other threads in the process instead of any process attaching to any jail. But even if not single threading, I could have a jail_attach sx that's only used for multi-threaded processes.

Move the thread_single and thread_single_end calls from do_jail_attach to the system call level (kern_jail_set, sys_jail_attach, sys_jail_attach_jd). Return ERESTART instead of EAGAIN for lost races.

Don't change refcount_acquire to prison_proc_hold - the INVARIANTS test makes then not the same.

New diff with drflags pointer passed to do_jail_attach. I also removed the requirement that the jail be locked, which hasn't been the case since pr_ref and pr_uref went atomic.

Removed an unrelated fix to a different jail_attach problem.

Yes, the current setup is an ad-hoc mess, and could use some work. But maybe we *should* refactor the whole thing. Not to the point of ABI change with different parameters, but at least with enough specification inside to be able to know and report the state of subsystems. The current setup of a single bit works for the majority of systems, which can't be disabled. But the truly three-way systems could be standardized with separate "enabled" and "new" flags. There's usually a "real" state that uses something aside from a bit, such as the existence of the pr_addrs array, but these could exist along with the bitmask.

The key point is "there aren't any other jps_get implementations." So it's all yours :-)

Only one little thing to go, so I'll call it approval. Since the parameter is allow.mount.all, the associated flag should be PR_ALLOW_MOUNT_ALL, not PR_ALLOW_MOUNT_ANY.

No, I still hold that the extra level adds nothing.

There's no need for the two-layer name "unsafe.all". If you really want both "unsafe" and "all" in the name allow.mount.unsafe_all should do. Better yet would be to keep is simple with allow.mount.all to allow all filesystems, with the understanding that such a thing might be unsafe. As it stands, there's this "allow.mount.unsafe" hierarchy, which suggests that allowing all filesystem types is unsafe, which each of those filesystem types is implicitly labeled as safe.

PR_ALLOW_UNPRIV_PARENT_TAMPER is enough of a corner case (in restricting a parent jail) that I don't foresee anyone else calling prison_chain_allow. But perhaps that's just my own lack of imagination ;-)

This makes sense in if_vmove_reclaim, where the vnet comes from the held prison. But in if_vmove_loan, you're only holding the prison that will get the interface, not the one that currently has it. That wouldn't affect whether ifp currently belongs to a mid-shutdown vnet.

In D55066#1259412, @markj wrote:

In D55066#1259408, @bz wrote:

My historic understanding was that we would have all the pr_* options/check inside kern_jail.c and have accessor functions for them passing td or cred.

Yes, this would be better, but I followed the pattern of the existing allow.vmm knob. I suspect it should be defined in kern_jail.c, not in vmm.ko, so that prison_priv_check() can access it. If there is some consensus on this I'll make that change.

Simple from the jail perspective, not delving into the MAC part ;-)

Why does it matter that putenv(3) doesn't create a copy?

The original author should have done this in the first place ;-)

While I prefer the version I mentioned in the inline notes (it's a little less branchy), I'm also fine with the patch as originally given.

It only touches peripherally on jails, but sure, looks good. One could ask why we decided to have a separate and subtlely different "dist" and "distname" but I suppose that's water long since under the bridge.

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.