x86 entry stuff, maybe for 4.4

Paolo pointed out that enter_from_user_mode could be called while irqflags were traced as though IRQs were on.

In principle, this could confuse lockdep. It doesn't cause any problems that I've seen in any configuration, but if I build with CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:

	if (irqs_disabled()) {
		spin_lock(&something);
		spin_unlock(&something);
	}

to the top of enter_from_user_mode, then lockdep will complain without this fix. It seems that lockdep's irqflags sanity checks are too weak to detect this bug without forcing the issue.

This patch adds one byte to normal kernels, and it's IMO a bit ugly. I haven't spotted a better way to do this yet, though. The issue is that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but we're also supposed to do it before calling C code.

An alternative approach would be to call trace_hardirqs_off in enter_from_user_mode. That would be less code and would not bloat normal kernels at all, but it would be harder to see how the code worked.

Signed-off-by Andy Lutomirski <[email protected]>

Paolo pointed out that enter_from_user_mode could be called while irqflags were traced as though IRQs were on.

In principle, this could confuse lockdep. It doesn't cause any problems that I've seen in any configuration, but if I build with CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:

	if (irqs_disabled()) {
		spin_lock(&something);
		spin_unlock(&something);
	}

to the top of enter_from_user_mode, then lockdep will complain without this fix. It seems that lockdep's irqflags sanity checks are too weak to detect this bug without forcing the issue.

This patch adds one byte to normal kernels, and it's IMO a bit ugly. I haven't spotted a better way to do this yet, though. The issue is that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but we're also supposed to do it before calling C code.

Quoted 16 lines · Andy Lutomirski

Paolo pointed out that enter_from_user_mode could be called while irqflags were traced as though IRQs were on.

In principle, this could confuse lockdep. It doesn't cause any problems that I've seen in any configuration, but if I build with CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:

      if (irqs_disabled()) {
              spin_lock(&something);
              spin_unlock(&something);
      }

to the top of enter_from_user_mode, then lockdep will complain without this fix. It seems that lockdep's irqflags sanity checks are too weak to detect this bug without forcing the issue.

Quoted 4 lines

This patch adds one byte to normal kernels, and it's IMO a bit ugly. I haven't spotted a better way to do this yet, though. The issue is that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but we're also supposed to do it before calling C code.

I would not mind to have that explanation in the code itself so that people don't scratch heads why the duplicated TRACE_IRQS_OFF call.

This is much less error-prone than the old code.

Signed-off-by Andy Lutomirski <[email protected]>

Unfortunately, these only work if HAVE_JUMP_LABEL. In principle, we could do some serious surgery on the core jump label infrastructure to keep the patch infrastructure available on x86 on all builds, but that's probably not worth it.

Quoted 4 lines · Andy Lutomirski

Unfortunately, these only work if HAVE_JUMP_LABEL. In principle, we could do some serious surgery on the core jump label infrastructure to keep the patch infrastructure available on x86 on all builds, but that's probably not worth it.

No, but we can be smarter about it. There is nothing which that asm entry code needs from linux/jump_label.h and asm/jump_label.h execpt STATIC_KEY_INIT_NOP. Something like the patch below should do the trick.

Thanks,

tglx

Index: tip/arch/x86/entry/calling.h
===================================================================

arch/x86/entry/calling.h For 32-bit we have the following convent · line 239

Quoted 5 lines · Thomas Gleixner

arch/x86/entry/calling.h +5 -0

+/* ASM jump label support */
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
+#include <asm/jump_label.h>
+
+       .macro STATIC_CALL_IF_ENABLED fun, key

I think we still need the "def" parameter. But maybe the "static call" is a good idea. (By giving 'def' a default value or omitting it entirely, it's too easy not to think about it, and that's a huge historical source of breakage and it's a good part of the reason for the new improved C API. At least these days I *think* we'll diagnose the problem on bootup instead of waiting for random breakage later.)

arch/x86/entry/calling.h For 32-bit we have the following convent · line 241

Quoted 2 lines

arch/x86/entry/calling.h +2 -0

+       1:
+       jmp.d32         2f

Old binutils will choke on this. But maybe no one has jump labels and old binutils.

arch/x86/entry/calling.h For 32-bit we have the following convent · line 252

Quoted 11 lines

arch/x86/entry/calling.h +11 -0

+       call            fun
+       .pushsection __jump_table, "aw"
+       _ASM_ALIGN
+       _ASM_PTR        1b, 2f, \key
+       .popsection
+       2:
+       .endm
+#else
+       .macro STATIC_CALL_IF_ENABLED fun, key
+       call            fun
+       .endm

This could result in errors down the road, since this macro is really "static call if enabled or if the kernel is built without jump labels". enter_from_user_mode is special because it's already a no-op if context tracking is off, but I'm not sure that this oddity should leak into more generic code.

Quoted 7 lines

+#endif
Index: tip/arch/x86/entry/entry_64.S
===================================================================

arch/x86/entry/entry_64.S +0 -0

@@ -510,7 +510,7 @@ END(irq_entries_start)
         */

arch/x86/entry/entry_64.S END(irq_entries_start) · line 514

Quoted 5 lines

arch/x86/entry/entry_64.S +1 -1

        SWAPGS
 #ifdef CONFIG_CONTEXT_TRACKING
-       call enter_from_user_mode
+       STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
 #endif

This doesn't get rid of the ifdeffery, which I thought was a nice feature of my patch.

Quoted 13 lines · Andy Lutomirski

arch/x86/entry/calling.h For 32-bit we have the following convent · line 239

Quoted 5 lines · Thomas Gleixner

arch/x86/entry/calling.h +5 -0

+/* ASM jump label support */
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
+#include <asm/jump_label.h>
+
+       .macro STATIC_CALL_IF_ENABLED fun, key

I think we still need the "def" parameter. But maybe the "static call" is a good idea. (By giving 'def' a default value or omitting it entirely, it's too easy not to think about it, and that's a huge historical source of breakage and it's a good part of the reason for the new improved C API. At least these days I *think* we'll diagnose the problem on bootup instead of waiting for random breakage later.)

Ok. That's easy to add :)

Quoted 5 lines

arch/x86/entry/calling.h For 32-bit we have the following convent · line 241

Quoted 2 lines

arch/x86/entry/calling.h +2 -0

+       1:
+       jmp.d32         2f

Old binutils will choke on this. But maybe no one has jump labels and old binutils.

Good question.

Quoted 12 lines

arch/x86/entry/calling.h For 32-bit we have the following convent · line 252

Quoted 11 lines

arch/x86/entry/calling.h +11 -0

+       call            fun
+       .pushsection __jump_table, "aw"
+       _ASM_ALIGN
+       _ASM_PTR        1b, 2f, \key
+       .popsection
+       2:
+       .endm
+#else
+       .macro STATIC_CALL_IF_ENABLED fun, key
+       call            fun
+       .endm

Quoted 5 lines

This could result in errors down the road, since this macro is really "static call if enabled or if the kernel is built without jump labels". enter_from_user_mode is special because it's already a no-op if context tracking is off, but I'm not sure that this oddity should leak into more generic code.

Hmm. Maybe we need a better name for this macro.

Quoted 7 lines

Quoted 7 lines

+#endif
Index: tip/arch/x86/entry/entry_64.S
===================================================================

arch/x86/entry/entry_64.S +0 -0

@@ -510,7 +510,7 @@ END(irq_entries_start)
         */

Quoted 8 lines

arch/x86/entry/entry_64.S END(irq_entries_start) · line 514

Quoted 5 lines

arch/x86/entry/entry_64.S +1 -1

        SWAPGS
 #ifdef CONFIG_CONTEXT_TRACKING
-       call enter_from_user_mode
+       STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
 #endif

This doesn't get rid of the ifdeffery, which I thought was a nice feature of my patch.

Right. I was just too lazy to add another macro :)

My main point was to avoid the extra stuff for !HAVE_JUMP_LABEL and hide this in a header file.

On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
True. But I hid it in a header file, too, but it was just a different header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.

Unfortunately, these only work if HAVE_JUMP_LABEL ....

Quoted 3 lines · Andy Lutomirski

On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
True. But I hid it in a header file, too, but it was just a different header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.

It probably does not really make a difference :)

I just got triggered by you saying:

Quoted 1 line

Unfortunately, these only work if HAVE_JUMP_LABEL ....

Quoted 1 line

Quoted 1 line

Unfortunately, these only work if HAVE_JUMP_LABEL ....

Yeah, I don't really like that either. I think that the real fix would be to compile in the runtime jump label bits unconditionally. Admittedly it would bloat the kernel image a little bit for !CONFIG_JUMP_LABEL or if build with an older gcc, and that's even less appropriate for 4.4 at this point. Peter?

Quoted 3 lines · Andy Lutomirski

On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
True. But I hid it in a header file, too, but it was just a different header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.

It probably does not really make a difference :)

I just got triggered by you saying:

Quoted 1 line

Unfortunately, these only work if HAVE_JUMP_LABEL ....

On CONFIG_CONTEXT_TRACKING kernels that have context tracking disabled at runtime (which includes most distro kernels), we still have the overhead of a call to enter_from_user_mode in interrupt and exception entries.

If jump labels are available, this uses the jump label infrastructure to skip the call.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/calling.h  | 15 +++++++++++++++
 arch/x86/entry/entry_64.S |  8 ++------
 2 files changed, 17 insertions(+), 6 deletions(-)

arch/x86/entry/calling.h +15 -0

index 3c71dd947c7b..271e30c585bc 100644
@@ -1,3 +1,5 @@
+#include <linux/jump_label.h>
+
 /*
 
  x86 function call convention, 64-bit:

@@ -232,3 +234,16 @@ For 32-bit we have the following conventions - kernel is built with
 
 #endif /* CONFIG_X86_64 */
 
+/*
+ * This does 'call enter_from_user_mode' unless we can avoid it based on
+ * kernel config or using the static jump infrastructure.
+ */
+.macro CALL_ENTER_FROM_USER_MODE
+#ifdef CONFIG_CONTEXT_TRACKING
+#ifdef HAVE_JUMP_LABEL
+	STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
+#endif
+	call enter_from_user_mode
+.Lafter_call_\@:
+#endif
+.endm

arch/x86/entry/entry_64.S +1 -3

index f585df24ab3d..9b49b56efa29 100644
@@ -517,9 +517,7 @@ END(irq_entries_start)
 	 */
 	TRACE_IRQS_OFF
 
-#ifdef CONFIG_CONTEXT_TRACKING
-	call enter_from_user_mode
-#endif
+	CALL_ENTER_FROM_USER_MODE
 
 1:
 	/*