linux-kernel - Re: [PATCH v0.9.1 3/6] sched/umcg: implement UMCG syscalls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Nov 2021 22:29:41 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Peter Oskolkov <posk@...k.io>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-api@...r.kernel.org, Paul Turner <pjt@...gle.com>,
        Ben Segall <bsegall@...gle.com>,
        Peter Oskolkov <posk@...gle.com>,
        Andrei Vagin <avagin@...gle.com>, Jann Horn <jannh@...gle.com>,
        Thierry Delisle <tdelisle@...terloo.ca>
Subject: Re: [PATCH v0.9.1 3/6] sched/umcg: implement UMCG syscalls

On Mon, Nov 29, 2021 at 10:08:41PM +0100, Peter Zijlstra wrote:
> I'm not sure I'm following. For this to be true A and C must be running
> on a different server right?
> 
> So we have something like:
> 
> 	S0 running A			S1 running B
> 
> Therefore:
> 
> 	S0::state == RUNNABLE		S1::state == RUNNABLE
> 	A::server_tid == S0.tid		B::server_tid == S1.tid
> 	A::state == RUNNING		B::state == RUNNING
> 
> Now, you want A to switch to C, therefore C had better be with S0, eg we
> have:
> 
> 	C::server_tid == S0.tid
> 	C::state == RUNNABLE
> 
> So then A does:
> 
> 	A::next_tid = C.tid;
> 	sys_umcg_wait();
> 
> Which will:
> 
> 	pin(A);
> 	pin(S0);
> 
> 	cmpxchg(A::state, RUNNING, RUNNABLE);
> 
> 	next_tid = A::next_tid; // C
> 
> 	enqueue(S0::runnable, A);
> 
> At which point B steals S0's runnable queue, and tries to make A go.
> 
> 					runnable = xchg(S0::runnable_list_ptr, NULL); // == A
> 					A::server_tid = S1.tid;
> 					B::next_tid = A.tid;
> 					sys_umcg_wait();
> 
> 	wake(C)
> 	  cmpxchg(C::state, RUNNABLE, RUNNING); <-- *fault*
> 
> 
> Something like that, right?

And note that there's an XXX in the code about exactly this case; it has
a question whether we want to add pin(next) to umcg_pin_pages().

That would not in fact help here, because sys_umcg_wait() is faultable
and the only reason it'll return -EFAULT is because, as stated below, C
is garbage. But it does make a difference for when we do something like:

	self->next_tid = someone;
	sys_something_we_expect_to_block();
	// handle not blocking

Because in that case userspace must have taken 'someone' from the
runnable queue and made it 'next', but then we'll not wake next but the
server, which then needs to figure out something went sideways.

So I'm tempted to add that optional 3rd pin, simply to reduce the
failure cases.

> What currently happens is that S0 goes back to S0 and S1 ends up in A.
> That is, if, for any reason we fail to wake next_tid, we'll wake
> server_tid.
> 
> So then S0 wakes up and gets to re-evaluate life. If it has another
> worker it can go run that, otherwise it can try and steal a worker
> somewhere or just idle out.
> 
> Now arguably, the only reason A->C can fault is because C is garbage, at
> which point your program is malformed and it doesn't matter what
> happens one way or the other.