Prevent a jail_attach race between threads

[jamie]

Summary

Attaching to a jail changes its root directory and its process credentials. These operations both require unlocking the jail, and also need allprison_lock unlocked. That means that if two threads are trying to attach to different jails at the same time, it's possible for the process to end up with one jail's root directory but the other jail's credentials.

Solve this by forcing the process into single-threaded mode during the do_jail_attach operation. Since the problem is only intra-process, it's not necessary to try adding a new lock that would be higher in the order than the current vnode and process locks.

Test Plan

Add a new test, jail_thread, that tries to create the races by repeatedly setting two threads to attach to different jails. On my single test machine (4 cores of byhve on a 12-core Ryzen), this happens about a quarter of the time. I make 10000 tries to make sure something is tripped, which is still very quick since nothing like filesystem is involved.

[kib]

sys/kern/kern_jail.c

3194

Do not do this when holding any locks, esp. sleepable.

Also be aware that other threads get spurious EINTRs if sleeping in the slow syscalls.

3196

This causes spurious EAGAIN returned to the caller sometimes. Why not return ERESTART?

[jamie]

sys/kern/kern_jail.c

3194

I could drop allprison_lock before this, or move this test to the front of the system calls (especially considering the ERESTART stuff mentioned below).

Unless "do this" means the entire single-threaded section between here and and thread_single_end shouldn't be sleepable. If that's the case, I'll have to find a completely different strategy.

3196

Does ERESTART make it to the kernel/user boundary? I couldn't tell from a quick look through.

For jail_attach and jail_attach_jd that would work. jail_set(JAIL_CREATE | JAIL_ATTACH) is more complicated since restarting the system call would fail on attempting to re-create the jail. The solution may be to make this test in kern_jail_set before starting anything.

[kib]

sys/kern/kern_jail.c

3194

Single threading should be done at the beginning of the syscall, if possible. After single threading is established, there is no limitation on the synchronization operations that the executing thread can perform. Almost no limitations, the existing shenanigans are related to threading and debugging, which should be not relevant there.

3196

It makes it back to userspace, and the syscall instruction is re-issued.

[jamie]

Move the thread_single and thread_single_end calls from do_jail_attach to the system call level (kern_jail_set, sys_jail_attach, sys_jail_attach_jd). Return ERESTART instead of EAGAIN for lost races.

[jamie]

sys/kern/kern_jail.c

3194

I'm content for spurious EINTR to be the caller's problem. This situation is unlikely to happen in real code, and the main focus is to keep things out of an invalid state (including possible security concerns, though no specific issue has been noted).

[kib]

BTW, is the problem is in just two parallel jail attaches occurring? I.e. if other thread executes chdir() it is not a problem, am I correct?
If yes, then I think you do not need the single-threading there, it is too big hammer. You could have a single sx that is taken in exclusive mode under the same conditions as you currently do singlethreading. It is both simple and has less complicated side effects.

[jamie]

I could do that, but I actually considered single threading to be the smaller hammer, because it only affects other threads in the process instead of any process attaching to any jail. But even if not single threading, I could have a jail_attach sx that's only used for multi-threaded processes.

I'll try that out. It could move the test back to do_jail_attach, and wouldn't need to worry about any ERESTART or EINTR.

[jamie]

I'll have to think a little on the chdir question. I considered it and chroot briefly, but only after I decided on single threading anyway, and I figured they weren't a problem because of the boundary requirement. Without that, it seems likely that a jail_attach in parallel with a chroot would have the same outcome.

[kib]

How that would be different from e.g. having some directory opened, and doing fchdir() or fchroot() after attaching? I suspect that the end effect is same but achieved much simpler.
Is there any other ways to mix things up?

[jamie]

That's why it's EPERM to jail_attach with any directory fds open. But a concurrent chdir/chroot could work around that, e.g. if the jail_attach happens between path lookup and pwd_chroot().

[jamie]

I altered the patch to use an sx lock instead of thread single, and it passed the test as expected. But then I added a test for a race between jail_attach_jd and chroot, and it failed.

Three race possiblities here:
1: chroot completes before the chroot part of jail_attach, desired outcome of root and credential both matching target jail
2: chroot (both lookup and execution) happens after chroot part of jail attach, also good with chroot changing to a within-jail path or returning ENOENT.
3: chroot lookup before the chroot part of jail_attach, execution afterward, leaving a jail possibly rooted outside of its root path.

thread_single prevents this, so I think it's the way to go. I could use that sx for both jail_attach and chroot, though it might expand to chdir and fchdir, and make me wonder what else is out there.

[jamie]

Add a test for races between jail_attach_jd and chroot. Let all the tests run instead of stopping on the first failure.

[markj]

sys/kern/kern_jail.c

1082

I prefer that we annotate unlocked accesses like this using atomic(9): if (atomic_load_int(&p->p_flag) & P_HADTHREADS) != 0. It's a signal to readers and concurrency sanitizers that the unlocked access is "safe", in this case because 1) P_HADTHREADS is never cleared once set, and 2) if we're the only thread in the process, then P_HADTHREADS isn't going to be toggled concurrently.

Same comment elsewhere.

3075

You might point to the comment in kern_jail_set(). (Or use a helper function to wrap thread_single().)

tests/sys/kern/jail_thread.c

23

Could you please use unique jail names for each test? Otherwise the tests can't run in parallel.

38

style(9) wants a newline after the return type, ditto below.

190

Most tests omit the whitespace between ATF_TC/ATC_TC_HEAD/ATF_TC_BODY. I think that makes them more readable.

[jamie]

Move thread_single calls and related code inside a well-commented
wrapper function. Add atomic_load to the process flag read. Fix
tests so they can run in parallel, other small fixes.

[jamie]

sys/kern/kern_jail.c

3075

I went with the help function route, which also neatens it up nicely. Each call has the single-line comment, and the function has the larger one that kern_jail_set has (a little more than that, actually).

[kib]

sys/kern/kern_jail.c

3286

If you introduce a local error variable, then PROC_UNLOCK() can be done only once:

error = 0;
if ((atomic_load_int(&p->p_flag) & P_HADTHREADS) != 0) {
        PROC_LOCK(p);
        if (thread_single(p, SINGLE_BOUNDARY))
                error = ERESTART;
        PROC_UNLOCK(p);
}
return (error);

[jamie]

Clean up prison_attach_thread_single a bit so I don't need to different PROC_UNLOCK calls.

[kib]

sys/kern/kern_jail.c

3279

int error;' should go after struct proc *p;` (style suggest ordering by alignment descending)

[jamie]

Fix variable declaration order.