Page MenuHomeFreeBSD

jail: fix leaked persistent prisons on failed jail_set(2) create
Needs ReviewPublic

Authored by maxim on Sun, Oct 18, 5:09 AM.
Tags
None
Referenced Files
F176595348: D60833.id.diff
Tue, Oct 20, 2:35 AM
F176577756: D60833.diff
Mon, Oct 19, 11:43 PM
Unknown Object (File)
Mon, Oct 19, 3:41 AM
Unknown Object (File)
Mon, Oct 19, 1:03 AM
Unknown Object (File)
Sun, Oct 18, 11:03 PM
Unknown Object (File)
Sun, Oct 18, 5:33 PM
Unknown Object (File)
Sun, Oct 18, 5:27 PM
Unknown Object (File)
Sun, Oct 18, 1:10 PM
Subscribers

Details

Reviewers
jamie
markj
Summary

If creating a persistent jail fails after kern_jail_set() has taken the
persist references (a failing OSD PR_METHOD_CREATE/SET method, or a MAC
label rejection), the half-built prison stays in allprison forever.

With INVARIANTS a jid lookup panics the kernel.

Two flaws:

  • prison_deref() ignores PD_KILL for every prison that is not ALIVE. That also skips a prison still being created (INVALID), which is exactly what jail_set(2) relies on PD_KILL to clean up. Ignore only DYING prisons. The defect was introduced in 78f70d4ff9dd.
  • PD_KILL was set only after the MAC label step, so a label failure never asked for the kill at all. Set it right after the parameters are validated, before the persist references are taken.

To reproduce the first bug:

jail -c persist name=x meta="$(jot -s '' -b a 5000)"; jls -d

To reproduce the second bug someone needs to write a MAC policy, I failed
to make a one-liner.

Fixes: 78f70d4ff9dd ("jail: fix a regression that creates zombies when removing dying jails")
MFC after: 1 month
Found with: Claude Code Sonnet 5.5

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 78329
Build 75212: arc lint + arc unit

Event Timeline

maxim requested review of this revision.Sun, Oct 18, 5:09 AM
sys/kern/kern_jail.c
2081

There's no point marking a prison to de-persist until it's marked persistent.

2240

So this is the earliest you would want to add PD_KILL. Another option would be to move the MAC label setting to before setting pr_flags, but there may be a reason it should be after.

I suppose it wouldn't actually change anything to keep it where you put it, since there aren't any error conditions between the two points. But this is the logical place.

Another option would be to not set the flags, or perhaps only PR_PERSIST, after the MAC block. But I don't know if those flags would be important.

3696

I'm uneasy about this. I think the code paths are clean, with allprison_lock being held exclusive between the setting of PR_PERSIST/PD_KILL and the jail visibility (setting PRISON_STATE_ALIVE).

If they are in fact clean, this should be fine. The prison_lock_xlock call won't drop allprison_lock, and the fact that prison_deref_kill sets PRISON_STATE_DYING should remain invisible until it's changed back to PRISON_STATE_INVALID.

3703

So just in case, I would want a "KASSERT(prison_isvalid(pr) || sx_xlocked(&allprison_lock))" here.