This adds avoidable overhead. Combined with prison_proc_hold the lock is taken and unlocked twice.

Instead you can add a read mostly-sleepable lock to protect against fork. It would scale perfectly in the common case and only be a burden when someone is racing killing the jail.

pseudo-code wise it would be like this:

int
prison_fork_enter(struct prison *pr)
{
        int error;

        error = 0;
        rms_rlock(pr->pr_forklock);
        if (__predict_false(pr->pr_state == PRISON_STATE_DYING))
                error = EAGAIN;
        return (error);
}

fork:

if (!prison_fork_enter(cred->cr_prison)) {
        PROC_LOCK(p1);
        kern_psignal(p1, SIGKILL);
        PROC_UNLOCK(p1);
        goto fail0;
}

do_fork(td, fr, newproc, td2, vm2, fp_procdesc);
prison_fork_exit(cred->cr_prison);

then when you switch the state, you rms_wlock() around it.

As a result you have an invariant that by the time you get the lock for writing, any pending forks in the jail wait and anyone who completed managed to finish and can get a signal when you kill it.

This will also open a way to move pr_ref to a per-cpu scheme and avoid taking the prison mutex altogether.

I have to stress rms_rlock and runlock don't do any atomics. which is the crux here.

I would suggest changing this to
"Allow making changes to a dying jail. This is deprecated and is equivalent to the
.Va allow.dying
parameter, which is also deprecated."

That will make it match the verbiage for -l, -n, -s, and -u/-U, and is a little easier to read.

I'm not familiar with read mostly-sleepable locks; I'll read up on them, to make sure I at least begin to understand.

True, the rms lock would remove one mtx overhead from do_fork. It would add a certain amount of complexity to kern_jail_set and prison_deref though (the places that set pr_state), because they will have to juggle the sleepable lock with the prison mutex.

Still, fork is so much a more common code path that the jail lifecycle, that it makes sense to keep that path clean.

This will also open a way to move pr_ref to a per-cpu scheme and avoid taking the prison mutex altogether.

So something like a combination of refcount(9) (which is atomic-based and not per-cpu) and counter(9) (which has a caveat that a fetched value is "not guaranteed to reflect the real cumulative value for any moment"). Ideally both increment and "decrement and test for zero" would be atomic-less in the common case.

Is there such a system currently in the kernel? It seems most things either use refcount(9) or mtx-protected non-atomic counters like jail does.

The kernel does not have a self-contained routine which does the trick, but I would argue it's not needed here.

General idea is to take the per-cpu lock for reading, verify per-cpu operation is still enabled and then modify the per-cpu state in whatever manner applicable. If it's not allowed, you fallback to mutex-protected case. Then you would have both per-cpu and a global counter, all of which can be modified in arbitrary manners in per-cpu mode. But if the mode gets disabled, you sum up the per-cpu state with the global counter and store it there. From that point on the only legal modification is to the global counter which provides an exact value.

I think moving refcounts around is not necessary for the purpose of this review though.

pseudo-code wise it would be like this:

int
prison_fork_enter(struct prison *pr)
{
        int error;

        error = 0;
        rms_rlock(pr->pr_forklock);
        if (__predict_false(pr->pr_state == PRISON_STATE_DYING))
                error = EAGAIN;
        return (error);
}

fork:

if (!prison_fork_enter(cred->cr_prison)) {
        PROC_LOCK(p1);
        kern_psignal(p1, SIGKILL);
        PROC_UNLOCK(p1);
        goto fail0;
}

do_fork(td, fr, newproc, td2, vm2, fp_procdesc);
prison_fork_exit(cred->cr_prison);

then when you switch the state, you rms_wlock() around it.

This has a deadlock problem. allproc_lock has higher precedence than allprison_lock - I think for a number of reasons that predate my time here, but for certain that's built in to the racct system, which has many places that lock wrap allprison_lock around allproc_lock. And allprison_lock needs higher precedence than this pr_forklock, since I would need it inside e.g. prison_find to avoid invalid/new prisons. A sample deadlock is:

A: fork / do_fork
B: jail_set / prison_racct_modify
C: jail_remove / prison_deref

1: A rlock pr_forklock
2: B slock allproc_lock
3: C xlock allprison_lock
4: A xlock allproc_lock, wait on 2B
5: B xlock allprison_lock, wait on 3C
6: C wlock pr_forklock, wait on 1A

So I could still use this lock around pr_state, which I would check inside do_fork as I do now, which would still save a pt_mtx lock. But the race case would have to be creating and killing the new process rather than avoiding it with EAGAIN.

I still like the rm lock idea for it, not just in fork, but because I think I may be able to avoid pr_mtx in prison_find and other such allprison loops (after accounting for pr_ref which I discovered I need to move to lockless for other reasons).

But does it still need to be a sleepable lock? Or should it be even? You mentioned the lack of atomics as a selling point, but a quick read of regular _rm_rlock suggests its common case is also no-atomic (though I don't know just how common that common case is).

It could be that by "higher precedence" I mean "lower precedence" but hopefully you get the idea.

The deadlock at hand should not be present. Worst case, if code flow really induces it, you can play tricks like setting the flag at some point, dropping the locks, then rms_wlock/rms_wunlock to synchronize against fork.

You could also just add PD_KILL at 0x20 and leave the other flags on the same bits?

I am pondering if "dying jails" should be allocated from the end of the ID space;

imagine jid=2 is dying immediately, jid=3...7 are running, and jid=8 is not started yet; then you probably pick jid=8 (I assume from the function name) and when jid=8 tries to start jid 8 is already used and the confusion continues as this change kind-of even makes it worse rather than better.

As I am expecting that to become an allocator problem ... and lastid .. unless we split the ID space in half or do some other nasty thing...

The reason I did it this way was to keep a bit of separation between the action flags and the current-state flags. In fact, if I'm changing those numbers, it might even be better to start PD_LOCKED at 0x10 or even 0x100.

If by "jid=8 is not started yet" you mean that nothing at all has been done with jid=8, then yes it would be picked. If you mean it's in the process of creation and not ready yet, then jid=9 would be picked, so no problem.

get_next_prid is based on the code that used to be inside kern_jail_set, moved into its function because in one iteration of this code I called it in two different places. Even though it's now only called in one, I'm keeping it in its own function because I should have been better with my code organization all along.

Currently, lastid sets a limit on the total number of jails, current or dying. With the potential renumbering of dying jails, lastid still sets a limit on the total number of jails, current and dying. So I don't expect any more of an allocator problem than we have now.

Generally, I see two scenarios:
1: well-behaved users who take whatever jid is given to them. get_next_prid will only ever be called for new jails, and at least for the first million jail creations will take the common case of not even looking through the list for a free jid.
2: users who want to specify things by jid, and may re-start jails that are in the dying state. get_next_prid will only ever be called to renumber the dying jails to an unused ID, and once the counter gets past the range of static jids that are being used, it too will usually just pick the next jid without having to traverse the list.
True, there could be a mixed case. But even then, it's little worse than the current case where adding a new jail will either check that a jid is available or look for a new one, only as it may have to do both for a dying jail replacement.

JID partitioning would either cut down the number of available active jails, limit the number of dying jails (which probably wouldn't be an issue), or convert dying jails to jids greater than JAIL_MAX.

I mean if my config has 50 jails and I am starting to rely on jid because the guarantee is now basically coming in and while I am starting all 50 jails one of the earlier jails already dies then the 1..50 guarantee is not there either and I still cannot use "JID" for anything unless I dynamically keep it at the end of start and with that I am no better or worse of using "name", right?

So what does this change really buy me? The answer is probably nothing? The only thing is that I never started to use names and should have a long time ago?

Or am I getting something wrong here?

This isn't about relying on the jid staying under a certain number when allocated dynamically; that works exactly the same as it has before.

This is about you having 50 jails with static-JID definitions like:
1 { ip4.addr 10.0.0.1; }
2 { ip4.addr 10.0.0.2; }

Currently, jail 1 dies with a TCP keepalive or other such thing that keeps it in dying state, and you can "create" it again with the same jid using "-d" or "allow.dying", which really just brings it back to life.

This doesn't change much from the user perspective: you can still re-create jail 1 when it's dying (with or without the now-unused allow.dying), but you'll get a new jail while the old one gets shoved off to jid 51 or whatever. Either way, the user sees "jail -c 1" working as expected.

The difference, and the reason for the patch, is that now the new jail 1 is always in a consistent newly-created state, without potential problems from something in the old jail state and jail.conf.

Part of this is to ease potential security problems (I don't have any examples cleaner is better). Part is with a guarantee that dead jails stay dead, more aggressive action can be taken to clean up dying jails. Notably, the potential advantages are the same for other scenarios of making dying jails totally invisible to userspace, or of not allowing static jids.

To my understanding the problem you are solving is "everything is running and something gets ""restarted""".

Your above explanation has the implicit understanding that no jail dies while all jails are initially started. That is not guaranteed. If I re-created your jail 1 the "now dying one" may be on jid 42 which is < 50 and suddenly jail 42..50 are on 43..51.

My problem is that "during startup your change now can screw my expectations of I start <n> jails they'll get jid 1..<n>" as those jids are all unused, because the "dying jail" consumes an extra JID on "restart" given creation of <n> jails is not atomic and a "restart" can happen before all are running.

So one problem solved another one introduced; hence my suggestion of moving dead jails to the end of the jail ID space rather than to the next free ID initially to reduce the likelihood of collision. If we do something like that then the startup problem also goes away and I'd be happy I think.

This *is* deprecated

OK, it took a while, but I think get it now :-).

Yes, that would happen if you started jails 1-41, and 1 died, and then you restarted it (explicitly setting it to 1), and then you started what you thought would be 42-50 which would turn out to be 43-51.

The dying jails don't get renumbered when they're dying. That only happens when a newly created jail wants to use the jid that a dying one currently occupies. So this problem scenario only happens when jails are implicitly numbered at first, and then explicitly numbered to their old jids on re-creation, and that re-creation can happen before all the jails are first created. Yes, in that scenario, the jids may not be as expected, and counting down the renumbered dying jails from the top would solve the issue.

We previously have a bug [1] that prevent jail die immediately, actually the jail stuck at dying state for quite a long time. The bug has been fixed now.

I guess maybe it is still useful for users to report such unusual state ? Or without it do we have simple mean to check / observe that unusual state ?

1. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=264981

It's still useful - there's bound to be another such case. That's why the flag is still supported for jail_get(2).