Page MenuHomeFreeBSD

Require allprison_lock and prison mutex when to free last prison reference
ClosedPublic

Authored by jamie on Jan 29 2021, 7:41 PM.

Details

Summary

Change the locking around pr_ref and pr_uref, requiring both the
prison lock and allprison_lock when they go to/from zero. Adding a
non-first or removing a non-last reference remain lock-free. Among
other things, this means that a shared lock on allprison_lock is
sufficient for prison_isvalid() and prison_isalive() to be useful,
which removes a number of cases of lock/check/unlock, moving locks
later to when they're needed for other things.

The now-common code to make sure that both the prison lock and
(exclusive) allprison_lock are both held, starting in a variety of
locking states, has been put into a function prison_lock_xlock(),
that uses the same flags as prison_deref().

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

jamie requested review of this revision.Jan 29 2021, 7:41 PM
jamie created this revision.

I haven't fully checked all cases but in general LGTM.

sys/kern/kern_jail.c
2565 ↗(On Diff #83121)

Looks like a long line in Phabricator.

2635 ↗(On Diff #83121)

If you unlock unconditionally leaving an assert is a good way to document the expectation for the caller.

2877 ↗(On Diff #83121)

Is this save in all circumstances? Or may some callers have to "restart" to validate state?
I see we had this before, so at least no change of status-quo.

sys/kern/uipc_mqueue.c
1570 ↗(On Diff #83121)

I assume this is save as we never remove OSDs for the base system jail so there is always a non-NULL pr_parent?

sys/kern/kern_jail.c
2565 ↗(On Diff #83121)

A lot of things do - it just depends on the browser window width. It's 79 columns in emacs, so I'll go with that :-).

2635 ↗(On Diff #83121)

Yeah, good point. I can put that back.

2877 ↗(On Diff #83121)

So far, yes. It's called mostly in prison_deref, which has long unlocked and relocked the prison mutex and shared allprison_lock as needed. Other callers are:

  • The end of kern_jail_set for new prisons, where the current prison either has pr_ref==0 so no else should touch it anyway, or just got one from do_jail_attach which we know won't be going away. And it's the very last thing the function does, so nothing else depends on allprison_lock.
  • do_jail_attach, which is either called by jail_attach using curthread's prison which is known will be sticking around, or kern_jail_set, which either has a ref held (existing prisons) or has pr_ref==0 (new prisons), so again dropping either the mutex or allprison_lock won't do anything bad.
  • prison_complete, which doesn't have any locks held at the time.

Generally speaking, callers do need to know that they don't really need these locks to stay unbroken, but it's static to kern_jail.c, so callers are generally expected to understand the jail locking ins and outs.

Oki doki. Continue as you want. "Glanced-at-by: bz"

Update for c050ea803eaa and add suggested mtx_assert.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 21 2021, 6:57 PM
This revision was automatically updated to reflect the committed changes.