From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:28
Ahmed and Sebastian wanted additional lockdep_assert*() macros and ran into
header hell. I figured using per-cpu variables would cure that, and also
ran into header hell, still tracktable though.
By moving the IRQ state into per-cpu variables we remove the dependency on
task_struct.
Patches go on top of anything recent I think, an actual git tree with them
in is (for now) here:
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git locking/irqstate
Which 0day blessed with 0 build fails.
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:31
In order to break a header dependency between lockdep and task_struct,
I need per-cpu stuff from lockdep.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/sparc/include/asm/percpu_64.h | 2 ++
arch/sparc/include/asm/trap_block.h | 2 ++
2 files changed, 4 insertions(+)
From: David Miller <davem@davemloft.net> Date: 2020-06-23 21:35:55
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue, 23 Jun 2020 10:36:48 +0200
In order to break a header dependency between lockdep and task_struct,
I need per-cpu stuff from lockdep.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:35
Currently all IRQ-tracking state is in task_struct, this means that
task_struct needs to be defined before we use it.
Especially for lockdep_assert_irq*() this can lead to header-hell.
Move the hardirq state into per-cpu variables to avoid the task_struct
dependency.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/irqflags.h | 19 ++++++++++++-------
include/linux/lockdep.h | 34 ++++++++++++++++++----------------
include/linux/sched.h | 2 --
kernel/fork.c | 4 +---
kernel/locking/lockdep.c | 30 +++++++++++++++---------------
kernel/softirq.c | 6 ++++++
6 files changed, 52 insertions(+), 43 deletions(-)
@@ -14,6 +14,7 @@#include<linux/typecheck.h>#include<asm/irqflags.h>+#include<asm/percpu.h>/* Currently lockdep_softirqs_on/off is used only by lockdep */#ifdef CONFIG_PROVE_LOCKING
@@ -20,6 +20,7 @@ extern int lock_stat;#define MAX_LOCKDEP_SUBCLASSES 8UL#include<linux/types.h>+#include<asm/percpu.h>enumlockdep_wait_type{LD_WAIT_INV=0,/* not checked, catch all */
@@ -703,28 +704,29 @@ do { \lock_release(&(lock)->dep_map,_THIS_IP_);\}while(0)-#define lockdep_assert_irqs_enabled() do { \-WARN_ONCE(debug_locks&&!current->lockdep_recursion&&\-!current->hardirqs_enabled,\-"IRQs not enabled as expected\n");\-}while(0)+DECLARE_PER_CPU(int,hardirqs_enabled);+DECLARE_PER_CPU(int,hardirq_context);-#define lockdep_assert_irqs_disabled() do { \-WARN_ONCE(debug_locks&&!current->lockdep_recursion&&\-current->hardirqs_enabled,\-"IRQs not disabled as expected\n");\-}while(0)+#define lockdep_assert_irqs_enabled() \+do{\+WARN_ON_ONCE(debug_locks&&!this_cpu_read(hardirqs_enabled));\+}while(0)-#define lockdep_assert_in_irq() do { \-WARN_ONCE(debug_locks&&!current->lockdep_recursion&&\-!current->hardirq_context,\-"Not in hardirq as expected\n");\-}while(0)+#define lockdep_assert_irqs_disabled() \+do{\+WARN_ON_ONCE(debug_locks&&this_cpu_read(hardirqs_enabled));\+}while(0)++#define lockdep_assert_in_irq() \+do{\+WARN_ON_ONCE(debug_locks&&!this_cpu_read(hardirq_context));\+}while(0)#else# define might_lock(lock) do { } while (0)# define might_lock_read(lock) do { } while (0)# define might_lock_nested(lock, subclass) do { } while (0)+# define lockdep_assert_irqs_enabled() do { } while (0)# define lockdep_assert_irqs_disabled() do { } while (0)# define lockdep_assert_in_irq() do { } while (0)
@@ -734,7 +736,7 @@ do { \# define lockdep_assert_RT_in_threaded_ctx() do { \WARN_ONCE(debug_locks&&!current->lockdep_recursion&&\-current->hardirq_context&&\+lockdep_hardirq_context(current)&&\!(current->hardirq_threaded||current->irq_config),\"Not in threaded context on PREEMPT_RT as expected\n");\}while(0)---a/include/linux/sched.h+++b/include/linux/sched.h
@@ -3751,7 +3751,7 @@ void noinstr lockdep_hardirqs_on(unsigneskip_checks:/* we'll do an OFF -> ON transition: */-curr->hardirqs_enabled=1;+this_cpu_write(hardirqs_enabled,1);curr->hardirq_enable_ip=ip;curr->hardirq_enable_event=++curr->irq_events;debug_atomic_inc(hardirqs_on_events);
From: Ahmed S. Darwish <hidden> Date: 2020-06-23 15:01:09
On Tue, Jun 23, 2020 at 10:36:52AM +0200, Peter Zijlstra wrote:
...
-#define lockdep_assert_irqs_disabled() do { \
- WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- current->hardirqs_enabled, \
- "IRQs not disabled as expected\n"); \
- } while (0)
+#define lockdep_assert_irqs_enabled() \
+do { \
+ WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \
+} while (0)
Can we add a small comment on top of lockdep_off(), stating that lockdep
IRQ tracking will still be kept after a lockdep_off call?
thanks,
--
Ahmed S. Darwish
Linutronix GmbH
From: Ahmed S. Darwish <hidden> Date: 2020-06-23 16:13:43
On Tue, Jun 23, 2020 at 05:24:50PM +0200, Peter Zijlstra wrote:
On Tue, Jun 23, 2020 at 05:00:31PM +0200, Ahmed S. Darwish wrote:
quoted
On Tue, Jun 23, 2020 at 10:36:52AM +0200, Peter Zijlstra wrote:
...
quoted
-#define lockdep_assert_irqs_disabled() do { \
- WARN_ONCE(debug_locks && !current->lockdep_recursion && \
- current->hardirqs_enabled, \
- "IRQs not disabled as expected\n"); \
- } while (0)
+#define lockdep_assert_irqs_enabled() \
+do { \
+ WARN_ON_ONCE(debug_locks && !this_cpu_read(hardirqs_enabled)); \
+} while (0)
Can we add a small comment on top of lockdep_off(), stating that lockdep
IRQ tracking will still be kept after a lockdep_off call?
That would only legitimize lockdep_off(). The only comment I want to put
on that is: "if you use this, you're doing it wrong'.
Well, freshly merged code is using it. For example, KCSAN:
=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:
void kcsan_report(...)
{
...
/*
* With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
* we do not turn off lockdep here; this could happen due to recursion
* into lockdep via KCSAN if we detect a race in utilities used by
* lockdep.
*/
lockdep_off();
...
}
thanks,
--
Ahmed S. Darwish
Linutronix GmbH
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 16:38:45
On Tue, Jun 23, 2020 at 06:13:21PM +0200, Ahmed S. Darwish wrote:
Well, freshly merged code is using it. For example, KCSAN:
=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:
void kcsan_report(...)
{
...
/*
* With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
* we do not turn off lockdep here; this could happen due to recursion
* into lockdep via KCSAN if we detect a race in utilities used by
* lockdep.
*/
lockdep_off();
...
}
Marco, do you remember what exactly happened there? Because I'm about to
wreck that. That is, I'm going to make TRACE_IRQFLAGS ignore
lockdep_off().
From: Marco Elver <elver@google.com> Date: 2020-06-23 18:00:08
On Tue, Jun 23, 2020 at 06:37PM +0200, Peter Zijlstra wrote:
On Tue, Jun 23, 2020 at 06:13:21PM +0200, Ahmed S. Darwish wrote:
quoted
Well, freshly merged code is using it. For example, KCSAN:
=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:
void kcsan_report(...)
{
...
/*
* With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
* we do not turn off lockdep here; this could happen due to recursion
* into lockdep via KCSAN if we detect a race in utilities used by
* lockdep.
*/
lockdep_off();
...
}
Marco, do you remember what exactly happened there? Because I'm about to
wreck that. That is, I'm going to make TRACE_IRQFLAGS ignore
lockdep_off().
Yeah, I was trying to squash any kind of recursion:
lockdep -> other libs ->
-> KCSAN
-> print report
-> dump stack, printk and friends
-> lockdep -> other libs
-> KCSAN ...
Some history:
* Initial patch to fix:
https://lore.kernel.org/lkml/20200115162512.70807-1-elver@google.com/
* KCSAN+lockdep+ftrace:
https://lore.kernel.org/lkml/20200214211035.209972-1-elver@google.com/
lockdep now has KCSAN_SANITIZE := n, but we still need to ensure that
there are no paths out of lockdep, or the IRQ flags tracing code, that
might lead through other libs, through KCSAN, libs used to generate a
report, and back to lockdep.
I never quite figured out the exact trace that led to corruption, but
avoiding any kind of potential for recursion was the only thing that
would avoid the check_flags() warnings.
Thanks,
-- Marco
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 18:13:53
On Tue, Jun 23, 2020 at 07:59:57PM +0200, Marco Elver wrote:
On Tue, Jun 23, 2020 at 06:37PM +0200, Peter Zijlstra wrote:
quoted
On Tue, Jun 23, 2020 at 06:13:21PM +0200, Ahmed S. Darwish wrote:
quoted
Well, freshly merged code is using it. For example, KCSAN:
=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:
void kcsan_report(...)
{
...
/*
* With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
* we do not turn off lockdep here; this could happen due to recursion
* into lockdep via KCSAN if we detect a race in utilities used by
* lockdep.
*/
lockdep_off();
...
}
Marco, do you remember what exactly happened there? Because I'm about to
wreck that. That is, I'm going to make TRACE_IRQFLAGS ignore
lockdep_off().
Yeah, I was trying to squash any kind of recursion:
lockdep -> other libs ->
-> KCSAN
-> print report
-> dump stack, printk and friends
-> lockdep -> other libs
-> KCSAN ...
Some history:
* Initial patch to fix:
https://lore.kernel.org/lkml/20200115162512.70807-1-elver@google.com/
That patch is weird; just :=n on lockdep.c should've cured that, the
rest is massive overkill.
lockdep now has KCSAN_SANITIZE := n, but we still need to ensure that
there are no paths out of lockdep, or the IRQ flags tracing code, that
might lead through other libs, through KCSAN, libs used to generate a
report, and back to lockdep.
I never quite figured out the exact trace that led to corruption, but
avoiding any kind of potential for recursion was the only thing that
would avoid the check_flags() warnings.
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
From: Marco Elver <elver@google.com> Date: 2020-06-23 18:39:52
On Tue, 23 Jun 2020 at 20:13, Peter Zijlstra [off-list ref] wrote:
On Tue, Jun 23, 2020 at 07:59:57PM +0200, Marco Elver wrote:
quoted
On Tue, Jun 23, 2020 at 06:37PM +0200, Peter Zijlstra wrote:
quoted
On Tue, Jun 23, 2020 at 06:13:21PM +0200, Ahmed S. Darwish wrote:
quoted
Well, freshly merged code is using it. For example, KCSAN:
=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:
void kcsan_report(...)
{
...
/*
* With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
* we do not turn off lockdep here; this could happen due to recursion
* into lockdep via KCSAN if we detect a race in utilities used by
* lockdep.
*/
lockdep_off();
...
}
Marco, do you remember what exactly happened there? Because I'm about to
wreck that. That is, I'm going to make TRACE_IRQFLAGS ignore
lockdep_off().
Yeah, I was trying to squash any kind of recursion:
lockdep -> other libs ->
-> KCSAN
-> print report
-> dump stack, printk and friends
-> lockdep -> other libs
-> KCSAN ...
Some history:
* Initial patch to fix:
https://lore.kernel.org/lkml/20200115162512.70807-1-elver@google.com/
That patch is weird; just :=n on lockdep.c should've cured that, the
rest is massive overkill.
lockdep now has KCSAN_SANITIZE := n, but we still need to ensure that
there are no paths out of lockdep, or the IRQ flags tracing code, that
might lead through other libs, through KCSAN, libs used to generate a
report, and back to lockdep.
I never quite figured out the exact trace that led to corruption, but
avoiding any kind of potential for recursion was the only thing that
would avoid the check_flags() warnings.
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
Thanks!
This was happening with Qian Cai's (Cc'd) test cases. If the kernel or
this patch changed things around so this doesn't happen anymore
regardless, then I don't see a problem.
Thanks,
-- Marco
From: Marco Elver <elver@google.com> Date: 2020-06-23 19:13:50
On Tue, Jun 23, 2020 at 08:39PM +0200, Marco Elver wrote:
On Tue, 23 Jun 2020 at 20:13, Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Jun 23, 2020 at 07:59:57PM +0200, Marco Elver wrote:
quoted
On Tue, Jun 23, 2020 at 06:37PM +0200, Peter Zijlstra wrote:
quoted
On Tue, Jun 23, 2020 at 06:13:21PM +0200, Ahmed S. Darwish wrote:
quoted
Well, freshly merged code is using it. For example, KCSAN:
=> f1bc96210c6a ("kcsan: Make KCSAN compatible with lockdep")
=> kernel/kcsan/report.c:
void kcsan_report(...)
{
...
/*
* With TRACE_IRQFLAGS, lockdep's IRQ trace state becomes corrupted if
* we do not turn off lockdep here; this could happen due to recursion
* into lockdep via KCSAN if we detect a race in utilities used by
* lockdep.
*/
lockdep_off();
...
}
Marco, do you remember what exactly happened there? Because I'm about to
wreck that. That is, I'm going to make TRACE_IRQFLAGS ignore
lockdep_off().
Yeah, I was trying to squash any kind of recursion:
lockdep -> other libs ->
-> KCSAN
-> print report
-> dump stack, printk and friends
-> lockdep -> other libs
-> KCSAN ...
Some history:
* Initial patch to fix:
https://lore.kernel.org/lkml/20200115162512.70807-1-elver@google.com/
That patch is weird; just :=n on lockdep.c should've cured that, the
rest is massive overkill.
lockdep now has KCSAN_SANITIZE := n, but we still need to ensure that
there are no paths out of lockdep, or the IRQ flags tracing code, that
might lead through other libs, through KCSAN, libs used to generate a
report, and back to lockdep.
I never quite figured out the exact trace that led to corruption, but
avoiding any kind of potential for recursion was the only thing that
would avoid the check_flags() warnings.
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
Thanks!
This was happening with Qian Cai's (Cc'd) test cases. If the kernel or
this patch changed things around so this doesn't happen anymore
regardless, then I don't see a problem.
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 19:42:32
On Tue, Jun 23, 2020 at 09:13:35PM +0200, Marco Elver wrote:
I see the below report when I boot with your branch + KCSAN and
PROVE_LOCKING. config attached. Trying to make sense of what's
happening.
Ah, I was still playing with tip/master + PROVE_LOCKING + KCSAN and
slowly removing parts of that annotation patch to see what would come
unstuck.
I think I just hit a genuine but unavoidable lockdep report on
report_lock.
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 20:24:41
On Tue, Jun 23, 2020 at 08:12:32PM +0200, Peter Zijlstra wrote:
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
OK, so the below patch doesn't seem to have any nasty recursion issues
here. The only 'problem' is that lockdep now sees report_lock can cause
deadlocks.
It is completely right about it too, but I don't suspect there's much we
can do about it, it's pretty much the standard printk() with scheduler
locks held report.
---
@@ -397,8 +397,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)}if(!kcsan_interrupt_watcher)-/* Use raw to avoid lockdep recursion via IRQ flags tracing. */-raw_local_irq_save(irq_flags);+local_irq_save(irq_flags);watchpoint=insert_watchpoint((unsignedlong)ptr,size,is_write);if(watchpoint==NULL){
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 21:09:21
On Tue, Jun 23, 2020 at 10:24:04PM +0200, Peter Zijlstra wrote:
On Tue, Jun 23, 2020 at 08:12:32PM +0200, Peter Zijlstra wrote:
quoted
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
OK, so the below patch doesn't seem to have any nasty recursion issues
here. The only 'problem' is that lockdep now sees report_lock can cause
deadlocks.
It is completely right about it too, but I don't suspect there's much we
can do about it, it's pretty much the standard printk() with scheduler
locks held report.
Just for giggles I added the below and that works fine too. Right until
the report_lock deadlock splat of course, thereafter lockdep is
disabled.
@@ -459,6 +459,8 @@ static void set_other_info_task_blocking(unsigned long *flags,*/inttimeout=max(kcsan_udelay_task,kcsan_udelay_interrupt);+lockdep_assert_held(&report_lock);+other_info->task=current;do{if(is_running){
@@ -495,6 +497,8 @@ static void set_other_info_task_blocking(unsigned long *flags,other_info->task==current);if(is_running)set_current_state(TASK_RUNNING);++lockdep_assert_held(&report_lock);}/* Populate @other_info; requires that the provided @other_info not in use. */
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-24 09:02:24
On Tue, Jun 23, 2020 at 10:24:04PM +0200, Peter Zijlstra wrote:
On Tue, Jun 23, 2020 at 08:12:32PM +0200, Peter Zijlstra wrote:
quoted
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
OK, so the below patch doesn't seem to have any nasty recursion issues
here. The only 'problem' is that lockdep now sees report_lock can cause
deadlocks.
It is completely right about it too, but I don't suspect there's much we
can do about it, it's pretty much the standard printk() with scheduler
locks held report.
So I've been getting tons and tons of this:
[ 60.471348] ==================================================================
[ 60.479427] BUG: KCSAN: data-race in __rcu_read_lock / __rcu_read_unlock
[ 60.486909]
[ 60.488572] write (marked) to 0xffff88840fff1cf0 of 4 bytes by interrupt on cpu 1:
[ 60.497026] __rcu_read_lock+0x37/0x60
[ 60.501214] cpuacct_account_field+0x1b/0x170
[ 60.506081] task_group_account_field+0x32/0x160
[ 60.511238] account_system_time+0xe6/0x110
[ 60.515912] update_process_times+0x1d/0xd0
[ 60.520585] tick_sched_timer+0xfc/0x180
[ 60.524967] __hrtimer_run_queues+0x271/0x440
[ 60.529832] hrtimer_interrupt+0x222/0x670
[ 60.534409] __sysvec_apic_timer_interrupt+0xb3/0x1a0
[ 60.540052] asm_call_on_stack+0x12/0x20
[ 60.544434] sysvec_apic_timer_interrupt+0xba/0x130
[ 60.549882] asm_sysvec_apic_timer_interrupt+0x12/0x20
[ 60.555621] delay_tsc+0x7d/0xe0
[ 60.559226] kcsan_setup_watchpoint+0x292/0x4e0
[ 60.564284] __rcu_read_unlock+0x73/0x2c0
[ 60.568763] __unlock_page_memcg+0xda/0xf0
[ 60.573338] unlock_page_memcg+0x32/0x40
[ 60.577721] page_remove_rmap+0x5c/0x200
[ 60.582104] unmap_page_range+0x83c/0xc10
[ 60.586582] unmap_single_vma+0xb0/0x150
[ 60.590963] unmap_vmas+0x81/0xe0
[ 60.594663] exit_mmap+0x135/0x2b0
[ 60.598464] __mmput+0x21/0x150
[ 60.601970] mmput+0x2a/0x30
[ 60.605176] exit_mm+0x2fc/0x350
[ 60.608780] do_exit+0x372/0xff0
[ 60.612385] do_group_exit+0x139/0x140
[ 60.616571] __do_sys_exit_group+0xb/0x10
[ 60.621048] __se_sys_exit_group+0xa/0x10
[ 60.625524] __x64_sys_exit_group+0x1b/0x20
[ 60.630189] do_syscall_64+0x6c/0xe0
[ 60.634182] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 60.639820]
[ 60.641485] read to 0xffff88840fff1cf0 of 4 bytes by task 2430 on cpu 1:
[ 60.648969] __rcu_read_unlock+0x73/0x2c0
[ 60.653446] __unlock_page_memcg+0xda/0xf0
[ 60.658019] unlock_page_memcg+0x32/0x40
[ 60.662400] page_remove_rmap+0x5c/0x200
[ 60.666782] unmap_page_range+0x83c/0xc10
[ 60.671259] unmap_single_vma+0xb0/0x150
[ 60.675641] unmap_vmas+0x81/0xe0
[ 60.679341] exit_mmap+0x135/0x2b0
[ 60.683141] __mmput+0x21/0x150
[ 60.686647] mmput+0x2a/0x30
[ 60.689853] exit_mm+0x2fc/0x350
[ 60.693458] do_exit+0x372/0xff0
[ 60.697062] do_group_exit+0x139/0x140
[ 60.701248] __do_sys_exit_group+0xb/0x10
[ 60.705724] __se_sys_exit_group+0xa/0x10
[ 60.710201] __x64_sys_exit_group+0x1b/0x20
[ 60.714872] do_syscall_64+0x6c/0xe0
[ 60.718864] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 60.724503]
[ 60.726156] Reported by Kernel Concurrency Sanitizer on:
[ 60.732089] CPU: 1 PID: 2430 Comm: sshd Not tainted 5.8.0-rc2-00186-gb4ee11fe08b3-dirty #303
[ 60.741510] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
[ 60.752957] ==================================================================
And I figured a quick way to get rid of that would be something like the
below, seeing how volatile gets auto annotated... but that doesn't seem
to actually work.
What am I missing?
From: Marco Elver <elver@google.com> Date: 2020-06-24 10:18:13
On Wed, 24 Jun 2020 at 11:01, Peter Zijlstra [off-list ref] wrote:
On Tue, Jun 23, 2020 at 10:24:04PM +0200, Peter Zijlstra wrote:
quoted
On Tue, Jun 23, 2020 at 08:12:32PM +0200, Peter Zijlstra wrote:
quoted
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
OK, so the below patch doesn't seem to have any nasty recursion issues
here. The only 'problem' is that lockdep now sees report_lock can cause
deadlocks.
It is completely right about it too, but I don't suspect there's much we
can do about it, it's pretty much the standard printk() with scheduler
locks held report.
So I've been getting tons and tons of this:
[ 60.471348] ==================================================================
[ 60.479427] BUG: KCSAN: data-race in __rcu_read_lock / __rcu_read_unlock
[ 60.486909]
[ 60.488572] write (marked) to 0xffff88840fff1cf0 of 4 bytes by interrupt on cpu 1:
[ 60.497026] __rcu_read_lock+0x37/0x60
[ 60.501214] cpuacct_account_field+0x1b/0x170
[ 60.506081] task_group_account_field+0x32/0x160
[ 60.511238] account_system_time+0xe6/0x110
[ 60.515912] update_process_times+0x1d/0xd0
[ 60.520585] tick_sched_timer+0xfc/0x180
[ 60.524967] __hrtimer_run_queues+0x271/0x440
[ 60.529832] hrtimer_interrupt+0x222/0x670
[ 60.534409] __sysvec_apic_timer_interrupt+0xb3/0x1a0
[ 60.540052] asm_call_on_stack+0x12/0x20
[ 60.544434] sysvec_apic_timer_interrupt+0xba/0x130
[ 60.549882] asm_sysvec_apic_timer_interrupt+0x12/0x20
[ 60.555621] delay_tsc+0x7d/0xe0
[ 60.559226] kcsan_setup_watchpoint+0x292/0x4e0
[ 60.564284] __rcu_read_unlock+0x73/0x2c0
[ 60.568763] __unlock_page_memcg+0xda/0xf0
[ 60.573338] unlock_page_memcg+0x32/0x40
[ 60.577721] page_remove_rmap+0x5c/0x200
[ 60.582104] unmap_page_range+0x83c/0xc10
[ 60.586582] unmap_single_vma+0xb0/0x150
[ 60.590963] unmap_vmas+0x81/0xe0
[ 60.594663] exit_mmap+0x135/0x2b0
[ 60.598464] __mmput+0x21/0x150
[ 60.601970] mmput+0x2a/0x30
[ 60.605176] exit_mm+0x2fc/0x350
[ 60.608780] do_exit+0x372/0xff0
[ 60.612385] do_group_exit+0x139/0x140
[ 60.616571] __do_sys_exit_group+0xb/0x10
[ 60.621048] __se_sys_exit_group+0xa/0x10
[ 60.625524] __x64_sys_exit_group+0x1b/0x20
[ 60.630189] do_syscall_64+0x6c/0xe0
[ 60.634182] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 60.639820]
[ 60.641485] read to 0xffff88840fff1cf0 of 4 bytes by task 2430 on cpu 1:
[ 60.648969] __rcu_read_unlock+0x73/0x2c0
[ 60.653446] __unlock_page_memcg+0xda/0xf0
[ 60.658019] unlock_page_memcg+0x32/0x40
[ 60.662400] page_remove_rmap+0x5c/0x200
[ 60.666782] unmap_page_range+0x83c/0xc10
[ 60.671259] unmap_single_vma+0xb0/0x150
[ 60.675641] unmap_vmas+0x81/0xe0
[ 60.679341] exit_mmap+0x135/0x2b0
[ 60.683141] __mmput+0x21/0x150
[ 60.686647] mmput+0x2a/0x30
[ 60.689853] exit_mm+0x2fc/0x350
[ 60.693458] do_exit+0x372/0xff0
[ 60.697062] do_group_exit+0x139/0x140
[ 60.701248] __do_sys_exit_group+0xb/0x10
[ 60.705724] __se_sys_exit_group+0xa/0x10
[ 60.710201] __x64_sys_exit_group+0x1b/0x20
[ 60.714872] do_syscall_64+0x6c/0xe0
[ 60.718864] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 60.724503]
[ 60.726156] Reported by Kernel Concurrency Sanitizer on:
[ 60.732089] CPU: 1 PID: 2430 Comm: sshd Not tainted 5.8.0-rc2-00186-gb4ee11fe08b3-dirty #303
[ 60.741510] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
[ 60.752957] ==================================================================
And I figured a quick way to get rid of that would be something like the
below, seeing how volatile gets auto annotated... but that doesn't seem
to actually work.
What am I missing?
There's one more in include/linux/rcupdate.h. I suggested this at some point:
https://lore.kernel.org/lkml/20200220213317.GA35033@google.com/
To avoid volatiles as I don't think they are needed here.
[ Still testing your other patches for KCSAN, will send another reply there. ]
Thanks,
-- Marco
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-24 12:32:36
On Wed, Jun 24, 2020 at 12:17:56PM +0200, Marco Elver wrote:
On Wed, 24 Jun 2020 at 11:01, Peter Zijlstra [off-list ref] wrote:
quoted
And I figured a quick way to get rid of that would be something like the
below, seeing how volatile gets auto annotated... but that doesn't seem
to actually work.
What am I missing?
Urgghh.. local_t is very expensive for this. The current code is
actually fine, even on load-store architectures. Using local_t will only
result in it being more expensive for no gain.
I'll go put data_race() around it.
From: Marco Elver <elver@google.com> Date: 2020-06-24 11:33:00
On Tue, Jun 23, 2020 at 10:24PM +0200, Peter Zijlstra wrote:
On Tue, Jun 23, 2020 at 08:12:32PM +0200, Peter Zijlstra wrote:
quoted
Fair enough; I'll rip it all up and boot a KCSAN kernel, see what if
anything happens.
OK, so the below patch doesn't seem to have any nasty recursion issues
here. The only 'problem' is that lockdep now sees report_lock can cause
deadlocks.
Thanks, using non-raw now makes sense.
It is completely right about it too, but I don't suspect there's much we
can do about it, it's pretty much the standard printk() with scheduler
locks held report.
Right, I think we just have to tolerate the potential risk of deadlock
until there is a way to make all the code that prints in print_report()
scheduler-safe (that includes stack_trace_print()).
Based on your suggested change to core.c, how about the below patch?
Anything we've missed? If you think it's reasonable, please carry it
with the IRQ state tracking changes.
As far as I can tell there are no more warnings together with the other
patch you sent to add '& LOCKDEP_RECURSION_MASK'.
Thanks,
-- Marco
------ >8 ------
From: Marco Elver <elver@google.com>
Date: Wed, 24 Jun 2020 11:23:22 +0200
Subject: [PATCH] kcsan: Make KCSAN compatible with new IRQ state tracking
The new IRQ state tracking code does not honor lockdep_off(), and as
such we should again permit tracing by using non-raw functions in
core.c. Update the lockdep_off() comment in report.c, to reflect the
fact there is still a potential risk of deadlock due to using printk()
from scheduler code.
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Marco Elver <elver@google.com>
---
kernel/kcsan/core.c | 5 ++---
kernel/kcsan/report.c | 9 +++++----
2 files changed, 7 insertions(+), 7 deletions(-)
@@ -397,8 +397,7 @@ kcsan_setup_watchpoint(const volatile void *ptr, size_t size, int type)}if(!kcsan_interrupt_watcher)-/* Use raw to avoid lockdep recursion via IRQ flags tracing. */-raw_local_irq_save(irq_flags);+local_irq_save(irq_flags);watchpoint=insert_watchpoint((unsignedlong)ptr,size,is_write);if(watchpoint==NULL){
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-24 15:19:13
On Wed, Jun 24, 2020 at 01:32:46PM +0200, Marco Elver wrote:
From: Marco Elver <elver@google.com>
Date: Wed, 24 Jun 2020 11:23:22 +0200
Subject: [PATCH] kcsan: Make KCSAN compatible with new IRQ state tracking
The new IRQ state tracking code does not honor lockdep_off(), and as
such we should again permit tracing by using non-raw functions in
core.c. Update the lockdep_off() comment in report.c, to reflect the
fact there is still a potential risk of deadlock due to using printk()
from scheduler code.
Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Marco Elver <elver@google.com>
Thanks!
I've put this in front of the series at hand. I'll wait a little while
longer for arch people to give feedback on their header patches before I
stuff the lot into tip/locking/core.
I think it would be nice to keep the "IRQs not disabled as expected"
message. It makes the lockdep splat much more readable.
This is similarly the case for the v3 lockdep preemption macros:
https://lkml.kernel.org/r/20200630054452.3675847-5-a.darwish@linutronix.de
I did not add a message though to get in-sync with the IRQ macros above.
Thanks,
--
Ahmed S. Darwish
Linutronix GmbH
I think it would be nice to keep the "IRQs not disabled as expected"
message. It makes the lockdep splat much more readable.
This is similarly the case for the v3 lockdep preemption macros:
https://lkml.kernel.org/r/20200630054452.3675847-5-a.darwish@linutronix.de
I did not add a message though to get in-sync with the IRQ macros above.
Hurmph.. the file:line output of a splat is usually all I look at, also
__WARN_printf() generates such atrocious crap code that try and not use
it.
I suppose I should do a __WARN_str() or something, but then people are
unlikely to want to use that, too much variation etc. :/
Cursed if you do, cursed if you don't.
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:38
In order to use <asm/percpu.h> in lockdep.h, we need to make sure
asm/percpu.h does not itself depend on lockdep.
The below seems to make that so and builds powerpc64-defconfig +
PROVE_LOCKING.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/powerpc/include/asm/dtl.h | 52 +++++++++++++++++++++++++++++++++
arch/powerpc/include/asm/lppaca.h | 44 ---------------------------
arch/powerpc/include/asm/paca.h | 2 -
arch/powerpc/kernel/time.c | 2 +
arch/powerpc/kvm/book3s_hv.c | 1
arch/powerpc/platforms/pseries/dtl.c | 1
arch/powerpc/platforms/pseries/lpar.c | 1
arch/powerpc/platforms/pseries/setup.c | 1
arch/powerpc/platforms/pseries/svm.c | 1
9 files changed, 60 insertions(+), 45 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:42
There is no reason not to always, accurately, track IRQ state.
This change also makes IRQ state tracking ignore lockdep_off().
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/locking/lockdep.c | 44 +++++++++++++++++++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 3 deletions(-)
@@ -3720,6 +3749,7 @@ void noinstr lockdep_hardirqs_on(unsigneDEBUG_LOCKS_WARN_ON(current->hardirq_chain_key!=current->curr_chain_key);+skip_checks:/* we'll do an OFF -> ON transition: */curr->hardirqs_enabled=1;curr->hardirq_enable_ip=ip;
@@ -109,9 +109,9 @@ do { \# define trace_hardirqs_off_finish() do { } while (0)# define trace_hardirqs_on() do { } while (0)# define trace_hardirqs_off() do { } while (0)-# define lockdep_hardirq_context(p) 0+# define lockdep_hardirq_context() 0# define lockdep_softirq_context(p) 0-# define lockdep_hardirqs_enabled(p) 0+# define lockdep_hardirqs_enabled() 0# define lockdep_softirqs_enabled(p) 0# define lockdep_hardirq_enter() do { } while (0)# define lockdep_hardirq_threaded() do { } while (0)---a/include/linux/lockdep.h+++b/include/linux/lockdep.h
@@ -736,7 +736,7 @@ do { \# define lockdep_assert_RT_in_threaded_ctx() do { \WARN_ONCE(debug_locks&&!current->lockdep_recursion&&\-lockdep_hardirq_context(current)&&\+lockdep_hardirq_context()&&\!(current->hardirq_threaded||current->irq_config),\"Not in threaded context on PREEMPT_RT as expected\n");\}while(0)---a/kernel/locking/lockdep.c+++b/kernel/locking/lockdep.c
@@ -2062,9 +2062,9 @@ print_bad_irq_dependency(struct task_strpr_warn("-----------------------------------------------------\n");pr_warn("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n",curr->comm,task_pid_nr(curr),-lockdep_hardirq_context(curr),hardirq_count()>>HARDIRQ_SHIFT,+lockdep_hardirq_context(),hardirq_count()>>HARDIRQ_SHIFT,curr->softirq_context,softirq_count()>>SOFTIRQ_SHIFT,-lockdep_hardirqs_enabled(curr),+lockdep_hardirqs_enabled(),curr->softirqs_enabled);print_lock(next);
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:47
While the nmi_enter() users did
trace_hardirqs_{off_prepare,on_finish}() there was no matching
lockdep_hardirqs_*() calls to complete the picture.
Introduce idtentry_{enter,exit}_nmi() to enable proper IRQ state
tracking across the NMIs.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/entry/common.c | 42 ++++++++++++++++++++++++++++++++++++----
arch/x86/include/asm/idtentry.h | 3 ++
arch/x86/kernel/nmi.c | 9 +++-----
arch/x86/kernel/traps.c | 17 +++++-----------
include/linux/hardirq.h | 28 ++++++++++++++++++--------
5 files changed, 70 insertions(+), 29 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:57
In order to use <asm/percpu.h> in irqflags.h, we need to make sure
asm/percpu.h does not itself depend on irqflags.h
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/s390/include/asm/smp.h | 1 +
arch/s390/include/asm/thread_info.h | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-23 08:39:59
In order to use <asm/percpu.h> in irqflags.h, we need to make sure
asm/percpu.h does not itself depend on irqflags.h.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm/include/asm/percpu.h | 2 ++
1 file changed, 2 insertions(+)
From: Will Deacon <will@kernel.org> Date: 2020-06-23 09:03:05
On Tue, Jun 23, 2020 at 10:36:51AM +0200, Peter Zijlstra wrote:
quoted hunk
In order to use <asm/percpu.h> in irqflags.h, we need to make sure
asm/percpu.h does not itself depend on irqflags.h.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm/include/asm/percpu.h | 2 ++
1 file changed, 2 insertions(+)
From: Peter Zijlstra <peterz@infradead.org> Date: 2020-06-24 17:55:07
On Tue, Jun 23, 2020 at 10:02:57AM +0100, Will Deacon wrote:
On Tue, Jun 23, 2020 at 10:36:51AM +0200, Peter Zijlstra wrote:
quoted
In order to use <asm/percpu.h> in irqflags.h, we need to make sure
asm/percpu.h does not itself depend on irqflags.h.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm/include/asm/percpu.h | 2 ++
1 file changed, 2 insertions(+)
If you define this unconditionally, then we can probably get rid of the
copy in asm/thread_info.h, rather than duplicate the same #define.
The below delta seems to build arm-allnoconfig, arm-defconfig and
arm-allmodconfig.
Although please don't ask me how asm/thread_info.h includes asm/percpu.h
Does that work for you?
From: Will Deacon <will@kernel.org> Date: 2020-06-25 07:31:53
On Wed, Jun 24, 2020 at 07:53:20PM +0200, Peter Zijlstra wrote:
On Tue, Jun 23, 2020 at 10:02:57AM +0100, Will Deacon wrote:
quoted
On Tue, Jun 23, 2020 at 10:36:51AM +0200, Peter Zijlstra wrote:
quoted
In order to use <asm/percpu.h> in irqflags.h, we need to make sure
asm/percpu.h does not itself depend on irqflags.h.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm/include/asm/percpu.h | 2 ++
1 file changed, 2 insertions(+)
If you define this unconditionally, then we can probably get rid of the
copy in asm/thread_info.h, rather than duplicate the same #define.
The below delta seems to build arm-allnoconfig, arm-defconfig and
arm-allmodconfig.
Although please don't ask me how asm/thread_info.h includes asm/percpu.h
Does that work for you?
Yes, thanks! I can't believe you removed the helpful comment.
-/*
- * how to get the current stack pointer in C
- */