Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 69027 Build 65910: arc lint + arc unit
Event Timeline
I guess that what protects the prison's label is the jail's mutex. That explains why you use the MAC_POLICY_CHECK_NOSLEEP() variant almost exclusively. This needs at least some documentation, perhaps just in the form of assertions that the prison is locked in the different entry points.
Which leads me to mac_prison_internalize_label() and mac_prison_internalize_label(). These are using MAC_POLICY_INTERNALIZE() and MAC_POLICY_EXTERNALIZE(), which both can sleep. For vnodes, this isn't a problem, as the vnode lock is sleepable, but that's not the case for prison locks. I'll watch out for this in the following revisions.
| sys/kern/kern_jail.c | ||
|---|---|---|
| 283–284 | Do you anticipate some specific prison0 treatment that necessitates to have mac_prison_create_init() in addition to mac_prison_init()? Saying this because mac_prison_init() is called for every jail, regardless of whether it is prison0. And then there's the ordering: Shouldn't mac_prison_create_init() be called first? Alternatives: Call one or the other, but not both. Or just remove mac_prison_create_init() entirely? If keeping mac_prison_create_init(), I would rename it so that it is immediately clear it applies only for prison0, the current name feels too vague. | |
| sys/security/mac/mac_prison.c | ||
| 49–50 | Preferably call mac_prison_label_free() here instead. | |
| sys/kern/kern_jail.c | ||
|---|---|---|
| 283–284 | You want mac_prison_init first to actually allocate the label. This is modelled after the same for mac_cred: crget() will mac_cred_init(), then we may follow-up with special handling for specific ucreds (swapper, init). For prison0, I was thinking it's immediately useful for much the same reason that we explicitly pass redundant struct label * around to various entry points: it rermoves the need for a policy to understand that prison0 is the root of the prison tree. As I write this, however, it occurs to me that trying to propagate a label in mac_prison_init means they'll need to check that pr_parent != NULL anyways, which does render the later call a little redundant. | |
| sys/sys/jail.h | ||
| 201 | Hmm, I was planning to leave it intact since this is main and the rest of the changes will require a __FreeBSD_version bump anyways. How strongly do you feel about it? | |
| sys/sys/jail.h | ||
|---|---|---|
| 201 | Eh, it's fine either way. | |
Highlights:
- Remove mac_prison_create_init(): one would probably want to special-case prison0 earlier in mac_prison_init(), and they don't actually need to know anything about prison0 specifically: pr_parent == NULL is a good indicator of the root prison
- Don't leak labels, call mac_prison_destroy()
- Drop a bunch of locking assertions in label operations
The most controversial change might be that mac_prison_init() now locks the
prison on success. This leaves us with a spurious locking of prison0 early on,
but it's guararnteed to be unlocked and uncontended so that seems more or less
fine. This change was driven by wanting to be sure that the pr_label assignment
was actually protected, though it doesn't really matter *today*: it's done
early enough in the jail lifetime that nothing else can grab a reference to it
yet.
For the non-prison0 creation path, the very next step is to grab the prison lock
anyways, so it works out well enough for us to do an M_WAITOK to init the label
before we lock the prison and assign it. Future work might want to check that
pr_label == NULL after we lock it, in case we decide to support something like
a jail MAC policy being loaded late and mac_prison_init() all existing jails
at that time. Other object types don't do this today, for whatever reason, so
I haven't considered it yet.
Thanks!
The most controversial change might be that mac_prison_init() now locks the
prison on success. This leaves us with a spurious locking of prison0 early on,
but it's guararnteed to be unlocked and uncontended so that seems more or less
fine. This change was driven by wanting to be sure that the pr_label assignment
was actually protected, though it doesn't really matter *today*: it's done
early enough in the jail lifetime that nothing else can grab a reference to it
yet.
This mac_prison_init() locking change is slightly disturbing, but to me only because callers (with M_NOWAIT) now have to be careful to treat the lock state as different on error. Transient locking of prison0 is not a problem, and pr_label assignment generally should be done under lock (even if not necessary right now, as you point out).
It seems mac_prison_init() could as well return the prison always unlocked, at the (tiny) expense of an immediate relock in kern_jail_set(), and loss of atomicity between this and other changes to the jail, but this does not matter since we are at prison creation time (the jail can't be found by someone else yet). If mac_prison_init() is finally used for late module loading, the module is probably only concerned with changing the label and does not need this to be atomic with respect to other changes (which is a constraint which anyway there's no general provision in MAC for). But I'm not insisting: It seems easy to change that once in tree, as this contract anyway does not concern module code.
Future work might want to check that
pr_label == NULL after we lock it, in case we decide to support something like
a jail MAC policy being loaded late and mac_prison_init() all existing jails
at that time. Other object types don't do this today, for whatever reason, so
I haven't considered it yet.
E.g., in mac_do(4), there is a need to attach info to jails at module load, which is done by iterating upon all jails and ensure proper initialization of each. Perhaps we could indeed have such kind of generic mechanism for "official" fields (in the case of mac_do(4), OSD is used). In the meantime, perhaps we should enforce that MPC_LOADTIME_FLAG_NOTLATE is present when MPC_OBJECT_PRISON must be set.