linux-kernel - Re: [PATCH v3 0/6] Static calls

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]

Message-ID: <5cbd249a-3b2b-6b3b-fb52-67571617403f@zytor.com>
Date:   Sun, 13 Jan 2019 18:31:19 -0800
From:   "H. Peter Anvin" <hpa@...or.com>
To:     Jiri Kosina <jikos@...nel.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Nadav Amit <namit@...are.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        David Laight <David.Laight@...lab.com>,
        Borislav Petkov <bp@...en8.de>,
        Julia Cartwright <julia@...com>, Jessica Yu <jeyu@...nel.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Edward Cree <ecree@...arflare.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>
Subject: Re: [PATCH v3 0/6] Static calls

On 1/11/19 11:39 AM, Jiri Kosina wrote:
> On Fri, 11 Jan 2019, hpa@...or.com wrote:
> 
>> I still don't see why can't simply spin in the #BP handler until the 
>> patch is complete.
> 
> I think this brings us to the already discussed possible deadlock, when 
> one CPU#0 is in the middle of text_poke_bp(), CPU#1 is spinning inside 
> spin_lock_irq*(&lock) and CPU#2 hits the breakpont while holding that very 
> 'lock'.
> 
> Then we're stuck forever, because CPU#1 will never handle the pending 
> sync_core() IPI (it's not NMI).
> 
> Or have I misunderstood what you meant?
> 

OK, I was thinking about this quite a while ago, and even started hacking on
it, but apparently I managed to forget some key details.

Specifically, you do *not* want to use the acknowledgment of the IPI as the
blocking condition, so don't use a waiting IPI.

Instead, you want a CPU bitmap (or percpu variable) that the IPI handler
clears.  When you are spinning in the IPI handler, you *also* need to clear
that bit.  Doing so is safe even in the case of batched updates, because you
are guaranteed to execute an IRET before you get to patched code.

So the synchronization part of the patching routine becomes:

static cpumask_t text_poke_cpumask;

static void text_poke_sync(void)
{
	smp_wmb();
	text_poke_cpumask = cpu_online_mask;
	smp_wmb();	/* Optional on x86 */
	cpumask_clear_cpu(&text_poke_cpumask, smp_processor_id());
	on_each_cpu_mask(&text_poke_cpumask, text_poke_sync_cpu, NULL, false);
	while (!cpumask_empty(&text_poke_cpumask)) {
		cpu_relax();
		smp_rmb();
	}
}

static void text_poke_sync_cpu(void *dummy)
{
	(void)dummy;

	smp_rmb();
	cpumask_clear_cpu(&poke_bitmask, smp_processor_id());
	/*
	 * We are guaranteed to return with an IRET, either from the
	 * IPI or the #BP handler; this provides serialization.
	 */
}

The spin routine then needs add a call to do something like this. By
(optionally) not comparing to a specific breakpoint address we allow for
batching, but we may end up spinning on a breakpoint that is not actually a
patching breakpoint until the patching is done.

int poke_int3_handler(struct pt_regs *regs)
{
	/* In the current handler, but shouldn't be needed... */
	smp_rmb();

	if (likely(!atomic_read(bp_patching_in_progress)))
		return 0;

	if (user_mode(regs) unlikely(!user_mode(regs) &&
	    atomic_read(&bp_patching_in_progress)) {
		text_poke_sync();
		regs->ip--;
		return 1;	/* May end up retaking the trap */
	} else {
		return 0;
	}
}

Unless I'm totally mistaken, the worst thing that will happen with this code
is that it may end up taking a harmless spurious IPI at a later time.

	-hpa