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

Groups > linux.kernel > #1264640 > unrolled thread

[PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking

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

  [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking Andy Lutomirski <luto@kernel.org> - 2015-11-07 00:20 +0100
    Re: [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context  tracking Thomas Gleixner <tglx@linutronix.de> - 2015-11-07 11:10 +0100
    Re: [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context  tracking Borislav Petkov <bp@alien8.de> - 2015-11-07 12:20 +0100
      Re: [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking Andy Lutomirski <luto@amacapital.net> - 2015-11-09 05:30 +0100

#1264640 — [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking

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 <luto@kernel.org>
---
 arch/x86/entry/entry_64.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 53616ca03244..f585df24ab3d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -509,6 +509,14 @@ END(irq_entries_start)
 	 * tracking that we're in kernel mode.
 	 */
 	SWAPGS
+
+	/*
+	 * IRQs are off.  NB: this trace call is duplicated.  That's
+	 * okay -- it's idempotent and it's irrelevant for performance as
+	 * it's a no-op unless CONFIG_DEBUG_LOCKDEP=y.
+	 */
+	TRACE_IRQS_OFF
+
 #ifdef CONFIG_CONTEXT_TRACKING
 	call enter_from_user_mode
 #endif
@@ -1049,12 +1057,13 @@ ENTRY(error_entry)
 	SWAPGS
 
 .Lerror_entry_from_usermode_after_swapgs:
+	TRACE_IRQS_OFF
 #ifdef CONFIG_CONTEXT_TRACKING
 	call enter_from_user_mode
 #endif
+	ret
 
 .Lerror_entry_done:
-
 	TRACE_IRQS_OFF
 	ret
 
-- 
2.4.3

--
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]

#1264794 — Re: [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking

On Fri, 6 Nov 2015, Andy Lutomirski wrote:
> 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 <luto@kernel.org>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
--
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]

#1264811 — Re: [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking

On Fri, Nov 06, 2015 at 03:12:43PM -0800, Andy Lutomirski wrote:
> 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.

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
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]

#1265333

On Sat, Nov 7, 2015 at 3:18 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Nov 06, 2015 at 03:12:43PM -0800, Andy Lutomirski wrote:
>> 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.
>
> 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.
>

Done for v2.

--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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web