kern: cpuset: resolve race between cpuset_lookup/cpuset_rel

[kevans]

Summary

The race plays out like so between threads A and B:

1. A ref's cpuset 10
2. B does a lookup of cpuset 10, grabs the cpuset lock and searches cpuset_ids
3. A rel's cpuset 10 and observes the last ref, waits on the cpuset lock while B is still searching and not yet ref'd
4. B ref's cpuset 10 and drops the cpuset lock
5. A proceeds to free the cpuset out from underneath B

Resolve the race by only releasing the last reference under the cpuset lock. Thread A now picks up the spinlock and observes that the cpuset has been revived, returning immediately for B to deal with later.

Reported by: [e-mail]

Test Plan

Ran the syz repro with -procs 4 -repeat 0; previously reproduced within a second or two, now runs with no issue.

Observed with vmstat -z | grep cpuset that requests skyrocket and no indication of a leak.

[kevans]

Observed with vmstat -z | grep cpuset that requests skyrocket and no indication of a leak.

I say that, but I seem to have ended up with exactly one leaked cpuset with 358 references to it as a result. I suspect that that's due to uncleanly terminating syz-execprog and losing references in there, but I'm investigating a bit further.

[kevans]

I found an incidental unr leak while investigating (which is what cued me in to the 'problem' I noted), but that's easy/obvious enough that I'm probably going to go ahead and just commit it:

diff --git a/sys/kern/kern_cpuset.c b/sys/kern/kern_cpuset.c
index c1739ef81f0..2c9e5dd7236 100644
--- a/sys/kern/kern_cpuset.c
+++ b/sys/kern/kern_cpuset.c
@@ -246,9 +254,14 @@ cpuset_rel_defer(struct setlist *head, struct cpuset *set)
 static void
 cpuset_rel_complete(struct cpuset *set)
 {
+       cpusetid_t id;
+
+       id = set->cs_id;
        LIST_REMOVE(set, cs_link);
        cpuset_rel(set->cs_parent);
        uma_zfree(cpuset_zone, set);
+       if (id != CPUSET_INVALID)
+               free_unr(cpuset_unr, id);
 }

 /*

It turns out the original problem was not a leak, but just how it works. The reproducer's changing the cpuset for proc0 (because pfind(0) happily finds proc0), so of course we end up with one more cpuset with a lot of references after that succeeds. I think it's perhaps not spectacular that userland can change the swapper's cpuset, but that's definitely an unrelated problem.

[markj]

I think the unr leak fix is right.

WRT modifying process 0, I'm not sure whether it's intentional or not. There are some other interfaces which appear to have this problem, basically anything that uses p_cansched().

sys/kern/kern_cpuset.c

213 ↗

(On Diff #80405)

refcount_release() returns a bool, so I think if (!refcount_release(...)) { ...; return; } is better.