I don't see you on the committer list - do you want me to commit this for you?

The const strings seem unlike to cause any trouble, but I wonder what code is out there along the lines of:

In D59748#1380508, @bz wrote:

I have an old (non-stack) stack open (I believe from 2018) of which you would have been a reviewer:
D15556 Initial vps (virtual process space) framework for jails.
D15570 Virtualization of basic variables and locks for jail+vps.
D15865 Provide process space virtualisation functionality for jails.
D15906 Implement "global" process (and zombie) lists loops iterating over all vps instances and all processes in each.

Just in case it helps taking this one level further; a rebase will certainly not be easy.

In D59748#1380440, @zlei wrote:

Some thinking about this:

1. Actually prison0 is almost identical to other unprivileged prisons. After we have virtual init, then is it possible to fast reboot prison0 ( let's forget kill 1 right now ) ?

I could be added with some new flag to specify it. But I don't know about the utility: usually when you're rebooting the whole system, you want to reset hardware state, or change a kernel, or something else more than this virtual reboot provides.

In D59748#1380088, @netchild wrote:

This is interesting. I asked Claude Code to help a bit with the review...

Ah, my first encounter with Claude. So how much of this was Claude Code, and how much was you?

Decouple the question of mac_prison_destroy's expected state, which is independent of the rest of the patch.

Look good. I was hesitant because it's an sx lock, but I don't see any mutexes held when it's locked. I suspect that hesitance was why I didn't do it tthe same as sysv_sem and sysv_msg in the first place, but that was a while ago and has totally escaped my memory.

Always call prison_deref_remove with both allprison and the prison lock held. That's the documented requirement for changing pr_state; otherwise some thread with a locked prison might find it disappear out from under them. This reverse to the old behavior of prison_deref_kill delaying the removal from the sibling list, but not delaying any of the other operations.

The test of (INVALID && pr_ref == 0) guarantees the "was valid, no longer is" status. When a prison is created, it's INVALID and initialized with pr_ref = 1. The only other time the state is INVALID is when the prison dies, i.e. reaches pr_ref == 0. It's certainly non-obvious, so I can add a comment for it.

Just on typing the description, I've found a problem: the reason mac_prison_destroy couldn't count on the prison being locked is that I call it from prison_deref_kill with a delay. The purpose of the delay is so I'm not mangling a prison's sibling list as I traverse it (and by that time the prison is no longer locked). But prison_deref_remove sets pr_state = PRISON_STATE_INVALID, which is only allowed when the mutex is held.

Yes, I'm referring to a prison I don't hold a reference to, without allprison_lock held. My bad. This look good, with the common path not adding any extra steps.

I wasn't familiar with __diagused. Yes, this is all cleaner.

Ah yes, they're in libjail as well.

Now that I think about it, jail(8) and jls(8) code is also rife with these strings.

It would make sense to broaden their use, with bare string literals also being used in vfs_copyopt and vfs_setopt calls.

What's the significance of the parent cpuset being marked CPU_SET_ROOT? This question didn't block my approval, because I really just don't understand the implications. Before, root cpusets had a 1:1 correspondence with jails, and now they don't.

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.