Combine duplicated code in prison_deref and prison_deref_kill

[jamie]

Summary

prison_deref and prison_deref kill do almost the same thing when they remove a jail's last reference. Put that common code into a new function. There's also an operation that prison_deref_kill does in two places that can also go into a helper function.

The "almost the same thing" is partly because mac_prison_destroy is only called in prison_deref, but it should be called in prison_deref_kill as well. As part of this, the condition for that call changes from "prison is locked" to "prison is invalid" and the assertion is changed to match.

[jamie]

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.

So I'll be changing this code a bit, probably moving LIST_REMOVE(pr, pr_sibling) out of prison_deref_remove and again making it the only delayed operation. That means the call path from both prison_deref and prison_deref_kill will be with a locked prison, and the assertion in mac_prison_destroy can change back from "is invalid" to "is locked".

It also means that I can go back to setting the state to invalid after the mac_prison_destroy call, and restore the comment that says "ideally we call this prior to any final state invalidation to be safe." But is that the right thing to do? I'm uncomfortable with the idea of a prison that has pr_ref == 0 and yet has pr_state == something valid. I think it still might be best to call it invalid, so if the mac handlers do anything that shouldn't be done to an invalid prison, they stand a greater chance of (properly) panicking.

[kevans]

In D60003#1376600, @jamie wrote:

It also means that I can go back to setting the state to invalid after the mac_prison_destroy call, and restore the comment that says "ideally we call this prior to any final state invalidation to be safe." But is that the right thing to do? I'm uncomfortable with the idea of a prison that has pr_ref == 0 and yet has pr_state == something valid. I think it still might be best to call it invalid, so if the mac handlers do anything that shouldn't be done to an invalid prison, they stand a greater chance of (properly) panicking.

I don't mind the state being INVALID, it's really more important that we're calling it on a jail that made it to ALIVE, which I think surfaces a different bug: mac_prison_create() is called just before pr_state transitions away, but all of the error paths before it will still go through normal deref teardown. The mac_prison_destroy() call should probably be predicated on the state having been not-INVALID before we invalidate it just prior to avoid surfacing an unpaired destroy().

[jamie]

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.

[jamie]

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.

Comment in mac_prison_destroy why the KASSERT tests what it does.

[kevans]

sys/kern/kern_jail.c

2306

To clarify: up here, for instance, we wouldn't yet have called mac_prison_created. We'll goto done_deref and have a pr_ref from allocation, but the created prison's state isn't set to ALIVE until line 2324 just below -- so we want to avoid calliing mac_prison_destroy unless the below state transition away from INVALID happened as our heuristic for "would the MAC layer have been aware that this prison was created?"

[jamie]

sys/kern/kern_jail.c

2306

What is mac_prison_destroy paired with? mac_prison_created seems reasonable, but mac_prison_init makes more sense to me. And that's called when the prison record is created in its first invalid state. If you avoid calling mac_prison_destroy because the jail was never fully alive, then it seems there would be a dangling label.

It could be that max_prison_destroy would want to do different things depending on whether a prison made it as far as having called mac_prison_created before it was destroyed. But one way or the other, it would still want to make that call.

BTW, the mac_prison_init call seems slightly misplaced. There's one possibility for an early error exit in creating jails: cpuset_create_root immediately before mac_prison_init, which should probably be immediately after.

[jamie]

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

mac_prison_destroy is still called from prison_deref_remove, meaning it's now called from prison_deref_kill as well as from the main body of prison_deref. But it's called the same way it was before, with the prison locked and the state not yet changed to PRISON_STATE_INVALID. Likewise, the assertion at the top of mac_prison_destroy has been changed back to ensuring that the prison is locked.