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.