Details

Reviewers

Commits

rG2bfecbef9a57: MFC jail: Add pr_state to struct prison
rG1158508a8086: jail: Add pr_state to struct prison

Summary

This is the first of two related changes, the second change making dying jails unresurrectable.

Currently, the state of a prison is deduced from its pr_ref and pr_uref counters, with three or four possible states:
pr_ref == 0: new (internal to kern_jail_set), or dead but not yet unlinked.
pr_ref > 0 && pr_uref > 0: alive, visible to kernel and user space
pr_ref > 0 && pr_uref == 0: dying, vidible to kernel but hidden from user space

The difference between the very beginning and end of a jail life cycle is subtle: both are linked into allprisons and pr_children, and are skipped when searching those lists. New prisons are referenced within kern_jail_set and given references to make them visible when the system call succeeds. Dead prisons are only linked into the lists when the last reference is dropped in a context that isn't known to support sleep locks.

This patch makes the jail state explicit, though it mostly follows the above model. The state may be one of the three already-defined constants in jail.h: PRISON_STATE_INVALID (new), PRISON_STATE_ALIVE, PRISON_STATE_DYING. The difference between ALIVE and DYING states is exactly as before: a prison is ALIVE when its pr_uref > 0. Fully dead jails are still deduced from pr_ref == 0, but new jails are not. With an explicit INVALID state, new jails can get a reference added at creation time, avoiding accidental removal coming from a ref being added and removed before the prison is fully created. I don't think there's a code path that does this, though it could possible happen in OSD modules.

Really though, this isn't intended as a standalone change, but part of a set. The next patch, removing the ability to "resurrect" a dying jail, relies on breaking the linkage between pr_uref and aliveness.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

jamie requested review of this revision.Jan 1 2021, 1:09 AM

jamie created this revision.

Updated for 2a4b22514635

jamie mentioned this in D28150: jail: Don't allow resurrection of dead jails.Jan 14 2021, 5:32 AM

jamie added a reviewer: bz.Jan 14 2021, 5:41 AM

In theory I like this change. Can you please upload it with full context either using arc or if doing diff file upload using -U10000?

Add full context diff for kern_jail.c

It doesn't really make sense for this to be a separate revision, considering it's part of D28150 which overwrites much of the change here. So I'm rolling it all into a single revision instead.

A new version, built on top of D28419, and updated with other recent jail changes. With the addition of prison_isvalid() and prison_isalive(), the changes are significantly reduced.

One notable change is that prison_free no longer removed the final reference, leaving that for prison_complete/prison_deref.

In general looks ok to me; didn't entirely review it. "Glanced-at-by: bz"

kern/kern_jail.c
1870 ↗	(On Diff #83135)	You check for shared lock as well but then acquire an exclusive lock? Do you need the exclusive lock or is that just so that you don't have to upgrade later? .. or would an slock suffice here?

jamie added inline comments.Jan 30 2021, 5:33 PM

kern/kern_jail.c
1870 ↗	(On Diff #83135)	Right, an slock is all that's needed for osd_jail_call(), and often what's set at this point - in fact allprison_lock is never xlocked in that path, but it seemed right to check that bit just on the safe side. Later on, prison_deref() is expected to destroy the prison, calling prison_lock_xlock(). I would have called it here, except that the prison mutex itself needs to be unlocked for osd_jail_call().