Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1191322 > unrolled thread

Re: Dealing with the NMI mess

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Dealing with the NMI mess Peter Zijlstra <peterz@infradead.org> - 2015-07-23 23:30 +0200
    Re: Dealing with the NMI mess Linus Torvalds <torvalds@linux-foundation.org> - 2015-07-23 23:40 +0200
      Re: Dealing with the NMI mess Andy Lutomirski <luto@amacapital.net> - 2015-07-23 23:50 +0200
        Re: Dealing with the NMI mess Linus Torvalds <torvalds@linux-foundation.org> - 2015-07-24 00:00 +0200
          Re: Dealing with the NMI mess Andy Lutomirski <luto@amacapital.net> - 2015-07-24 00:10 +0200
            Re: Dealing with the NMI mess Linus Torvalds <torvalds@linux-foundation.org> - 2015-07-24 00:10 +0200
            Re: Dealing with the NMI mess Peter Zijlstra <peterz@infradead.org> - 2015-07-24 12:30 +0200
          Re: Dealing with the NMI mess Peter Zijlstra <peterz@infradead.org> - 2015-07-24 13:10 +0200

#1191322 — Re: Dealing with the NMI mess

On Thu, Jul 23, 2015 at 01:38:33PM -0700, Linus Torvalds wrote:

> And the "take them and disable them" is really simple. No "am I in an
> NMI contect" thing (because that leads to the whole question about
> "what is NMI context"). That's not the real rule anyway.
> 
> No, make it very simple and straightforward. Make the test be "uhhuh,
> I got a #DB in kernel mode, and interrupts were disabled - I know I'm
> going to return with "ret", so I'm just going to have to disable this
> breakpoint".
> 
> Nothing clever. Nothing subtle. Nothing that needs "this range of
> instructions is magical". No.  Just a very simple rule: if the context
> we return to is kernel mode and interrupts are disabled, we're using
> 'ret', so we cannot suppress debug faults.
> 
> Did I miss something? There were a lot of emails flying around, but I
> *thought* I saw them all..

So the NMI could trigger userspace debug register faults, and simply
disabling them would make the whole debug register thing entirely
unreliable.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191335

On Thu, Jul 23, 2015 at 2:20 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> So the NMI could trigger userspace debug register faults, and simply
> disabling them would make the whole debug register thing entirely
> unreliable.

We could easily set something to re-enable them for when we actually
return to user space. I'd be ok with just setting the
_TIF_USER_WORK_MASK.

But even that should not be a requirement for the basic stability and
core integrity of the kernel. Not like the current horrid mess with
NMI nesting and ESP fixing etc.

And realistically, nobody will ever even notice. So the whole "ok, we
can use _TIF_USER_WORK_MASK to re-enable dr7" is a tiny tiny detail
that is more like cleaning up things, not a core issue.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191340

On Thu, Jul 23, 2015 at 2:35 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jul 23, 2015 at 2:20 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> So the NMI could trigger userspace debug register faults, and simply
>> disabling them would make the whole debug register thing entirely
>> unreliable.
>
> We could easily set something to re-enable them for when we actually
> return to user space. I'd be ok with just setting the
> _TIF_USER_WORK_MASK.
>
> But even that should not be a requirement for the basic stability and
> core integrity of the kernel. Not like the current horrid mess with
> NMI nesting and ESP fixing etc.
>
> And realistically, nobody will ever even notice. So the whole "ok, we
> can use _TIF_USER_WORK_MASK to re-enable dr7" is a tiny tiny detail
> that is more like cleaning up things, not a core issue.
>

Or we just re-enable them on the way out of NMI (i.e. the very last
thing we do in the NMI handler).  I don't want to break regular
userspace gdb when perf is running.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191346

On Thu, Jul 23, 2015 at 2:45 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Or we just re-enable them on the way out of NMI (i.e. the very last
> thing we do in the NMI handler).  I don't want to break regular
> userspace gdb when perf is running.

I'd really prefer it if we don't touch NMI code in those kinds of
ways. The NMI code is fragile as hell. All the problems we have with
it is exactly due to "where is the boundary" issues.

That's why I *don't* want NMI code to do magic crap. Anything that
says "disable this during this magic window" is broken. The problems
we've had are exactly about atomicity of the entry/exit conditions,
and there is no really good way to get them right.

I'd be much happier with a _TIF_USER_WORK_MASK approach exactly
because it's so *obvious* that it's not a boundary condition.

I dislike the "disable and re-enable dr7 in the NMI handler" exactly
because it smells like "we can only handle faults in _this_ region".
It may be true, but it's also what I want us to get away from. I'd
much rather have the "big picture" be that we can take faults anywhere
at all (*), and that none of the core code really cares. Then we "fix
up" user space.

                   Linus

(*) And yes, sysenter and not having a stack at all is very special,
and I think we will *always* have to have that magical special case of
the first few instructions there. But that's a separate hardware
limitation we can't get around.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191357

On Thu, Jul 23, 2015 at 2:54 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Jul 23, 2015 at 2:45 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Or we just re-enable them on the way out of NMI (i.e. the very last
>> thing we do in the NMI handler).  I don't want to break regular
>> userspace gdb when perf is running.
>
> I'd really prefer it if we don't touch NMI code in those kinds of
> ways. The NMI code is fragile as hell. All the problems we have with
> it is exactly due to "where is the boundary" issues.
>
> That's why I *don't* want NMI code to do magic crap. Anything that
> says "disable this during this magic window" is broken. The problems
> we've had are exactly about atomicity of the entry/exit conditions,
> and there is no really good way to get them right.
>
> I'd be much happier with a _TIF_USER_WORK_MASK approach exactly
> because it's so *obvious* that it's not a boundary condition.
>
> I dislike the "disable and re-enable dr7 in the NMI handler" exactly
> because it smells like "we can only handle faults in _this_ region".
> It may be true, but it's also what I want us to get away from. I'd
> much rather have the "big picture" be that we can take faults anywhere
> at all (*), and that none of the core code really cares. Then we "fix
> up" user space.

OK, new proposal:

In do_debug, if we trip an instruction breakpoint while
!user_mode(regs) && ((regs->flags & X86_EFLAGS_IF) == 0), then disarm
*that breakpoint*.

Why?  It only looks at hardware state (dr6 and dr7), and it can't
break gdb, because gdb can't set a breakpoint that will cause this
problem.

All the other variants of this either need cached state or break gdb
watchpoints on stack variables with perf running.

--Andy

>
>                    Linus
>
> (*) And yes, sysenter and not having a stack at all is very special,
> and I think we will *always* have to have that magical special case of
> the first few instructions there. But that's a separate hardware
> limitation we can't get around.



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191358

On Thu, Jul 23, 2015 at 2:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> OK, new proposal:
>
> In do_debug, if we trip an instruction breakpoint while
> !user_mode(regs) && ((regs->flags & X86_EFLAGS_IF) == 0), then disarm
> *that breakpoint*.

Ack.  The more targeted we can make this while still guaranteeing
forward progress, the better. So that sounds really good.

              Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191690

On Thu, Jul 23, 2015 at 02:59:46PM -0700, Andy Lutomirski wrote:
> OK, new proposal:
> 
> In do_debug, if we trip an instruction breakpoint while
> !user_mode(regs) && ((regs->flags & X86_EFLAGS_IF) == 0), then disarm
> *that breakpoint*.

Doesn't !IF already imply that it must be kernel space? AFAIK user space
cannot clear IF.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191722

On Thu, Jul 23, 2015 at 02:54:54PM -0700, Linus Torvalds wrote:
> On Thu, Jul 23, 2015 at 2:45 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> > Or we just re-enable them on the way out of NMI (i.e. the very last
> > thing we do in the NMI handler).  I don't want to break regular
> > userspace gdb when perf is running.
> 
> I'd really prefer it if we don't touch NMI code in those kinds of
> ways. The NMI code is fragile as hell. All the problems we have with
> it is exactly due to "where is the boundary" issues.
> 
> That's why I *don't* want NMI code to do magic crap. Anything that
> says "disable this during this magic window" is broken. The problems
> we've had are exactly about atomicity of the entry/exit conditions,
> and there is no really good way to get them right.
> 
> I'd be much happier with a _TIF_USER_WORK_MASK approach exactly
> because it's so *obvious* that it's not a boundary condition.
> 
> I dislike the "disable and re-enable dr7 in the NMI handler" exactly
> because it smells like "we can only handle faults in _this_ region".
> It may be true, but it's also what I want us to get away from. I'd
> much rather have the "big picture" be that we can take faults anywhere
> at all (*), and that none of the core code really cares. Then we "fix
> up" user space.

A wee bit something like so?

We need the intermediate self-IPI because NMI/MCE etc do not deal with
TIF flags.

I further cleared all of DR7 in an attempt at reducing the amount of
state tracked. And it doesn't distinguish between kernel/user
watchpoints because the kernel can touch both from !IF.

---
 arch/x86/kernel/traps.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 8e65d8a9b8db..e8308e9c2b1e 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -570,6 +570,33 @@ struct bad_iret_stack *fixup_bad_iret(struct bad_iret_stack *s)
 NOKPROBE_SYMBOL(fixup_bad_iret);
 #endif
 
+struct do_debug_state {
+	unsigned long dr7;
+	struct irq_work irq_work;
+	struct callback_head task_work;
+};
+
+static void __debug_irq_trampoline(struct irq_work *work)
+{
+	struct do_debug_state *dds =
+		container_of(work, struct do_debug_state, irq_work);
+
+	task_work_add(current, &dds->task_work, true);
+}
+
+static void __debug_restore_dr7(struct callback_head *work)
+{
+	struct do_debug_state *dds =
+		container_of(work, struct do_debug_state, task_work);
+
+	set_debugreg(dds->dr7, 7);
+}
+
+static DEFINE_PER_CPU(struct do_debug_state, do_debug_state) = {
+	.irq_work = { .func = __debug_irq_trampoline, },
+	.task_work = { .func = __debug_restore_dr7, },
+};
+
 /*
  * Our handling of the processor debug registers is non-trivial.
  * We do not clear them on entry and exit from the kernel. Therefore
@@ -603,6 +630,16 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 
 	ist_enter(regs);
 
+	if (arch_irqs_disabled_flags(regs->flags)) {
+		struct do_debug_state *dds = this_cpu_ptr(&do_debug_state);
+
+		get_debugreg(dds->dr7, 7);
+		set_debugreg(0, 7);
+		irq_work_queue(&dds->irq_work);
+
+		goto exit;
+	}
+
 	get_debugreg(dr6, 6);
 
 	/* Filter out all the reserved bits which are preset to 1 */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web