Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1543269 > unrolled thread
| Started by | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| First post | 2016-12-16 10:00 +0100 |
| Last post | 2016-12-16 12:20 +0100 |
| Articles | 20 on this page of 29 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH 1/4] mm: add new mmgrab() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:00 +0100
Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:10 +0100
Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:50 +0100
crash during oom reaper (was: Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging) Michal Hocko <mhocko@kernel.org> - 2016-12-16 11:20 +0100
Re: crash during oom reaper (was: Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging) "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 11:50 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 13:00 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 13:20 +0100
Re: crash during oom reaper "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 13:50 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 14:00 +0100
Re: crash during oom reaper "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 14:10 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 14:20 +0100
Re: crash during oom reaper Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-12-18 14:50 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-18 17:10 +0100
Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 14:20 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 15:10 +0100
Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 15:30 +0100
Re: crash during oom reaper Michal Hocko <mhocko@kernel.org> - 2016-12-16 15:50 +0100
Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 16:00 +0100
Re: crash during oom reaper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 15:10 +0100
Re: [PATCH 3/4] mm: use mmget_not_zero() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:30 +0100
Re: [PATCH 2/4] mm: add new mmget() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:30 +0100
[PATCH 3/4] mm: use mmget_not_zero() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:40 +0100
[PATCH 2/4] mm: add new mmget() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 10:50 +0100
Re: [PATCH 1/4] mm: add new mmgrab() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 10:50 +0100
Re: [PATCH 1/4] mm: add new mmgrab() helper Peter Zijlstra <peterz@infradead.org> - 2016-12-16 11:00 +0100
Re: [PATCH 1/4] mm: add new mmgrab() helper "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-12-16 11:20 +0100
Re: [PATCH 1/4] mm: add new mmgrab() helper Michal Hocko <mhocko@kernel.org> - 2016-12-16 11:50 +0100
Re: [PATCH 1/4] mm: add new mmgrab() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 12:00 +0100
Re: [PATCH 1/4] mm: add new mmgrab() helper Vegard Nossum <vegard.nossum@oracle.com> - 2016-12-16 12:20 +0100
Page 1 of 2 [1] 2 Next page →
| From | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| Date | 2016-12-16 10:00 +0100 |
| Subject | [PATCH 1/4] mm: add new mmgrab() helper |
| Message-ID | <sOWwW-19z-41@gated-at.bofh.it> |
Apart from adding the helper function itself, the rest of the kernel is
converted mechanically using:
git grep -l 'atomic_inc.*mm_count' | xargs sed -i 's/atomic_inc(&\(.*\)->mm_count);/mmgrab\(\1\);/'
git grep -l 'atomic_inc.*mm_count' | xargs sed -i 's/atomic_inc(&\(.*\)\.mm_count);/mmgrab\(\&\1\);/'
This is needed for a later patch that hooks into the helper, but might be
a worthwhile cleanup on its own.
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
arch/alpha/kernel/smp.c | 2 +-
arch/arc/kernel/smp.c | 2 +-
arch/arm/kernel/smp.c | 2 +-
arch/arm64/kernel/smp.c | 2 +-
arch/blackfin/mach-common/smp.c | 2 +-
arch/hexagon/kernel/smp.c | 2 +-
arch/ia64/kernel/setup.c | 2 +-
arch/m32r/kernel/setup.c | 2 +-
arch/metag/kernel/smp.c | 2 +-
arch/mips/kernel/traps.c | 2 +-
arch/mn10300/kernel/smp.c | 2 +-
arch/parisc/kernel/smp.c | 2 +-
arch/powerpc/kernel/smp.c | 2 +-
arch/s390/kernel/processor.c | 2 +-
arch/score/kernel/traps.c | 2 +-
arch/sh/kernel/smp.c | 2 +-
arch/sparc/kernel/leon_smp.c | 2 +-
arch/sparc/kernel/smp_64.c | 2 +-
arch/sparc/kernel/sun4d_smp.c | 2 +-
arch/sparc/kernel/sun4m_smp.c | 2 +-
arch/sparc/kernel/traps_32.c | 2 +-
arch/sparc/kernel/traps_64.c | 2 +-
arch/tile/kernel/smpboot.c | 2 +-
arch/x86/kernel/cpu/common.c | 4 ++--
arch/xtensa/kernel/smp.c | 2 +-
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +-
drivers/gpu/drm/i915/i915_gem_userptr.c | 2 +-
drivers/infiniband/hw/hfi1/file_ops.c | 2 +-
fs/proc/base.c | 4 ++--
fs/userfaultfd.c | 2 +-
include/linux/sched.h | 5 +++++
kernel/exit.c | 2 +-
kernel/futex.c | 2 +-
kernel/sched/core.c | 4 ++--
mm/khugepaged.c | 2 +-
mm/ksm.c | 2 +-
mm/mmu_context.c | 2 +-
mm/mmu_notifier.c | 2 +-
mm/oom_kill.c | 4 ++--
virt/kvm/kvm_main.c | 2 +-
40 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
index 46bf263c3153..acb4b146a607 100644
--- a/arch/alpha/kernel/smp.c
+++ b/arch/alpha/kernel/smp.c
@@ -144,7 +144,7 @@ smp_callin(void)
alpha_mv.smp_callin();
/* All kernel threads share the same mm context. */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
/* inform the notifiers about the new cpu */
diff --git a/arch/arc/kernel/smp.c b/arch/arc/kernel/smp.c
index 88674d972c9d..9cbc7aba3ede 100644
--- a/arch/arc/kernel/smp.c
+++ b/arch/arc/kernel/smp.c
@@ -125,7 +125,7 @@ void start_kernel_secondary(void)
setup_processor();
atomic_inc(&mm->mm_users);
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
current->active_mm = mm;
cpumask_set_cpu(cpu, mm_cpumask(mm));
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 7dd14e8395e6..c6514ce0fcbc 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -371,7 +371,7 @@ asmlinkage void secondary_start_kernel(void)
* reference and switch to it.
*/
cpu = smp_processor_id();
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
current->active_mm = mm;
cpumask_set_cpu(cpu, mm_cpumask(mm));
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 8507703dabe4..61969ea29654 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -214,7 +214,7 @@ asmlinkage void secondary_start_kernel(void)
* All kernel threads share the same mm context; grab a
* reference and switch to it.
*/
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
current->active_mm = mm;
set_my_cpu_offset(per_cpu_offset(smp_processor_id()));
diff --git a/arch/blackfin/mach-common/smp.c b/arch/blackfin/mach-common/smp.c
index 23c4ef5f8bdc..bc5617ef7128 100644
--- a/arch/blackfin/mach-common/smp.c
+++ b/arch/blackfin/mach-common/smp.c
@@ -308,7 +308,7 @@ void secondary_start_kernel(void)
/* Attach the new idle task to the global mm. */
atomic_inc(&mm->mm_users);
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
current->active_mm = mm;
preempt_disable();
diff --git a/arch/hexagon/kernel/smp.c b/arch/hexagon/kernel/smp.c
index 983bae7d2665..c02a6455839e 100644
--- a/arch/hexagon/kernel/smp.c
+++ b/arch/hexagon/kernel/smp.c
@@ -162,7 +162,7 @@ void start_secondary(void)
);
/* Set the memory struct */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
cpu = smp_processor_id();
diff --git a/arch/ia64/kernel/setup.c b/arch/ia64/kernel/setup.c
index 7ec7acc844c2..ecbff47b01f1 100644
--- a/arch/ia64/kernel/setup.c
+++ b/arch/ia64/kernel/setup.c
@@ -992,7 +992,7 @@ cpu_init (void)
*/
ia64_setreg(_IA64_REG_CR_DCR, ( IA64_DCR_DP | IA64_DCR_DK | IA64_DCR_DX | IA64_DCR_DR
| IA64_DCR_DA | IA64_DCR_DD | IA64_DCR_LC));
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
BUG_ON(current->mm);
diff --git a/arch/m32r/kernel/setup.c b/arch/m32r/kernel/setup.c
index 136c69f1fb8a..b18bc0bd6544 100644
--- a/arch/m32r/kernel/setup.c
+++ b/arch/m32r/kernel/setup.c
@@ -403,7 +403,7 @@ void __init cpu_init (void)
printk(KERN_INFO "Initializing CPU#%d\n", cpu_id);
/* Set up and load the per-CPU TSS and LDT */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
if (current->mm)
BUG();
diff --git a/arch/metag/kernel/smp.c b/arch/metag/kernel/smp.c
index bad13232de51..af9cff547a19 100644
--- a/arch/metag/kernel/smp.c
+++ b/arch/metag/kernel/smp.c
@@ -345,7 +345,7 @@ asmlinkage void secondary_start_kernel(void)
* reference and switch to it.
*/
atomic_inc(&mm->mm_users);
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
current->active_mm = mm;
cpumask_set_cpu(cpu, mm_cpumask(mm));
enter_lazy_tlb(mm, current);
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 3905003dfe2b..e50b0e0ca44c 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -2177,7 +2177,7 @@ void per_cpu_trap_init(bool is_boot_cpu)
if (!cpu_data[cpu].asid_cache)
cpu_data[cpu].asid_cache = asid_first_version(cpu);
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
BUG_ON(current->mm);
enter_lazy_tlb(&init_mm, current);
diff --git a/arch/mn10300/kernel/smp.c b/arch/mn10300/kernel/smp.c
index 426173c4b0b9..e65b5cc2fa67 100644
--- a/arch/mn10300/kernel/smp.c
+++ b/arch/mn10300/kernel/smp.c
@@ -589,7 +589,7 @@ static void __init smp_cpu_init(void)
}
printk(KERN_INFO "Initializing CPU#%d\n", cpu_id);
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
BUG_ON(current->mm);
diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 75dab2871346..67b452b41ff6 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -279,7 +279,7 @@ smp_cpu_init(int cpunum)
set_cpu_online(cpunum, true);
/* Initialise the idle task for this CPU */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
BUG_ON(current->mm);
enter_lazy_tlb(&init_mm, current);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 9c6f3fd58059..42b82364c782 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -707,7 +707,7 @@ void start_secondary(void *unused)
unsigned int cpu = smp_processor_id();
int i, base;
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
smp_store_cpu_info(cpu);
diff --git a/arch/s390/kernel/processor.c b/arch/s390/kernel/processor.c
index 81d0808085e6..ec9bc100895c 100644
--- a/arch/s390/kernel/processor.c
+++ b/arch/s390/kernel/processor.c
@@ -73,7 +73,7 @@ void cpu_init(void)
get_cpu_id(id);
if (machine_has_cpu_mhz)
update_cpu_mhz(NULL);
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
BUG_ON(current->mm);
enter_lazy_tlb(&init_mm, current);
diff --git a/arch/score/kernel/traps.c b/arch/score/kernel/traps.c
index 5cea1e750cec..6f6e5a39d147 100644
--- a/arch/score/kernel/traps.c
+++ b/arch/score/kernel/traps.c
@@ -336,7 +336,7 @@ void __init trap_init(void)
set_except_vector(18, handle_dbe);
flush_icache_range(DEBUG_VECTOR_BASE_ADDR, IRQ_VECTOR_BASE_ADDR);
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
cpu_cache_init();
}
diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
index 38e7860845db..ee379c699c08 100644
--- a/arch/sh/kernel/smp.c
+++ b/arch/sh/kernel/smp.c
@@ -178,7 +178,7 @@ asmlinkage void start_secondary(void)
struct mm_struct *mm = &init_mm;
enable_mmu();
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
atomic_inc(&mm->mm_users);
current->active_mm = mm;
#ifdef CONFIG_MMU
diff --git a/arch/sparc/kernel/leon_smp.c b/arch/sparc/kernel/leon_smp.c
index 71e16f2241c2..b99d33797e1d 100644
--- a/arch/sparc/kernel/leon_smp.c
+++ b/arch/sparc/kernel/leon_smp.c
@@ -93,7 +93,7 @@ void leon_cpu_pre_online(void *arg)
: "memory" /* paranoid */);
/* Attach to the address space of init_task. */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
index 8182f7caf5b1..c1d2bed22961 100644
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -122,7 +122,7 @@ void smp_callin(void)
current_thread_info()->new_child = 0;
/* Attach to the address space of init_task. */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
/* inform the notifiers about the new cpu */
diff --git a/arch/sparc/kernel/sun4d_smp.c b/arch/sparc/kernel/sun4d_smp.c
index 9d98e5002a09..7b55c50eabe5 100644
--- a/arch/sparc/kernel/sun4d_smp.c
+++ b/arch/sparc/kernel/sun4d_smp.c
@@ -93,7 +93,7 @@ void sun4d_cpu_pre_online(void *arg)
show_leds(cpuid);
/* Attach to the address space of init_task. */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
local_ops->cache_all();
diff --git a/arch/sparc/kernel/sun4m_smp.c b/arch/sparc/kernel/sun4m_smp.c
index 278c40abce82..633c4cf6fdb0 100644
--- a/arch/sparc/kernel/sun4m_smp.c
+++ b/arch/sparc/kernel/sun4m_smp.c
@@ -59,7 +59,7 @@ void sun4m_cpu_pre_online(void *arg)
: "memory" /* paranoid */);
/* Attach to the address space of init_task. */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
while (!cpumask_test_cpu(cpuid, &smp_commenced_mask))
diff --git a/arch/sparc/kernel/traps_32.c b/arch/sparc/kernel/traps_32.c
index 4f21df7d4f13..ecddac5a4c96 100644
--- a/arch/sparc/kernel/traps_32.c
+++ b/arch/sparc/kernel/traps_32.c
@@ -448,7 +448,7 @@ void trap_init(void)
thread_info_offsets_are_bolixed_pete();
/* Attach to the address space of init_task. */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
/* NOTE: Other cpus have this done as they are started
diff --git a/arch/sparc/kernel/traps_64.c b/arch/sparc/kernel/traps_64.c
index 4094a51b1970..0dbbe40012ef 100644
--- a/arch/sparc/kernel/traps_64.c
+++ b/arch/sparc/kernel/traps_64.c
@@ -2764,6 +2764,6 @@ void __init trap_init(void)
/* Attach to the address space of init_task. On SMP we
* do this in smp.c:smp_callin for other cpus.
*/
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
}
diff --git a/arch/tile/kernel/smpboot.c b/arch/tile/kernel/smpboot.c
index 6c0abaacec33..53ce940a5016 100644
--- a/arch/tile/kernel/smpboot.c
+++ b/arch/tile/kernel/smpboot.c
@@ -160,7 +160,7 @@ static void start_secondary(void)
__this_cpu_write(current_asid, min_asid);
/* Set up this thread as another owner of the init_mm */
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
current->active_mm = &init_mm;
if (current->mm)
BUG();
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cc9e980c68ec..b580da4582e1 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1555,7 +1555,7 @@ void cpu_init(void)
for (i = 0; i <= IO_BITMAP_LONGS; i++)
t->io_bitmap[i] = ~0UL;
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
me->active_mm = &init_mm;
BUG_ON(me->mm);
enter_lazy_tlb(&init_mm, me);
@@ -1606,7 +1606,7 @@ void cpu_init(void)
/*
* Set up and load the per-CPU TSS and LDT
*/
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
curr->active_mm = &init_mm;
BUG_ON(curr->mm);
enter_lazy_tlb(&init_mm, curr);
diff --git a/arch/xtensa/kernel/smp.c b/arch/xtensa/kernel/smp.c
index fc4ad21a5ed4..9bf5cea3bae4 100644
--- a/arch/xtensa/kernel/smp.c
+++ b/arch/xtensa/kernel/smp.c
@@ -136,7 +136,7 @@ void secondary_start_kernel(void)
/* All kernel threads share the same mm context. */
atomic_inc(&mm->mm_users);
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
current->active_mm = mm;
cpumask_set_cpu(cpu, mm_cpumask(mm));
enter_lazy_tlb(mm, current);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index ef7c8de7060e..ca5f2aa7232d 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -262,7 +262,7 @@ static void kfd_process_notifier_release(struct mmu_notifier *mn,
* and because the mmu_notifier_unregister function also drop
* mm_count we need to take an extra count here.
*/
- atomic_inc(&p->mm->mm_count);
+ mmgrab(p->mm);
mmu_notifier_unregister_no_release(&p->mmu_notifier, p->mm);
mmu_notifier_call_srcu(&p->rcu, &kfd_process_destroy_delayed);
}
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index c6f780f5abc9..f21ca404af79 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -341,7 +341,7 @@ i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
mm->i915 = to_i915(obj->base.dev);
mm->mm = current->mm;
- atomic_inc(¤t->mm->mm_count);
+ mmgrab(current->mm);
mm->mn = NULL;
diff --git a/drivers/infiniband/hw/hfi1/file_ops.c b/drivers/infiniband/hw/hfi1/file_ops.c
index bd786b7bd30b..2e1a6643a910 100644
--- a/drivers/infiniband/hw/hfi1/file_ops.c
+++ b/drivers/infiniband/hw/hfi1/file_ops.c
@@ -185,7 +185,7 @@ static int hfi1_file_open(struct inode *inode, struct file *fp)
if (fd) {
fd->rec_cpu_num = -1; /* no cpu affinity by default */
fd->mm = current->mm;
- atomic_inc(&fd->mm->mm_count);
+ mmgrab(fd->mm);
fp->private_data = fd;
} else {
fp->private_data = NULL;
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ca651ac00660..0b8ccacae8b3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -795,7 +795,7 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
if (!IS_ERR_OR_NULL(mm)) {
/* ensure this mm_struct can't be freed */
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
/* but do not pin its memory */
mmput(mm);
}
@@ -1093,7 +1093,7 @@ static int __set_oom_adj(struct file *file, int oom_adj, bool legacy)
if (p) {
if (atomic_read(&p->mm->mm_users) > 1) {
mm = p->mm;
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
}
task_unlock(p);
}
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 85959d8324df..ffa9c7cbc5fa 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1304,7 +1304,7 @@ static struct file *userfaultfd_file_create(int flags)
ctx->released = false;
ctx->mm = current->mm;
/* prevent the mm struct to be freed */
- atomic_inc(&ctx->mm->mm_count);
+ mmgrab(ctx->mm);
file = anon_inode_getfile("[userfaultfd]", &userfaultfd_fops, ctx,
O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
diff --git a/include/linux/sched.h b/include/linux/sched.h
index e9c009dc3a4a..31ae1f49eebb 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2872,6 +2872,11 @@ static inline unsigned long sigsp(unsigned long sp, struct ksignal *ksig)
*/
extern struct mm_struct * mm_alloc(void);
+static inline void mmgrab(struct mm_struct *mm)
+{
+ atomic_inc(&mm->mm_count);
+}
+
/* mmdrop drops the mm and the page tables */
extern void __mmdrop(struct mm_struct *);
static inline void mmdrop(struct mm_struct *mm)
diff --git a/kernel/exit.c b/kernel/exit.c
index 3076f3089919..b12753840050 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -500,7 +500,7 @@ static void exit_mm(struct task_struct *tsk)
__set_task_state(tsk, TASK_RUNNING);
down_read(&mm->mmap_sem);
}
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
BUG_ON(mm != tsk->active_mm);
/* more a memory barrier than a real lock */
task_lock(tsk);
diff --git a/kernel/futex.c b/kernel/futex.c
index 2c4be467fecd..cbe6056c17c1 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -338,7 +338,7 @@ static inline bool should_fail_futex(bool fshared)
static inline void futex_get_mm(union futex_key *key)
{
- atomic_inc(&key->private.mm->mm_count);
+ mmgrab(key->private.mm);
/*
* Ensure futex_get_mm() implies a full barrier such that
* get_futex_key() implies a full barrier. This is relied upon
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 154fd689fe02..ee1fb0070544 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2877,7 +2877,7 @@ context_switch(struct rq *rq, struct task_struct *prev,
if (!mm) {
next->active_mm = oldmm;
- atomic_inc(&oldmm->mm_count);
+ mmgrab(oldmm);
enter_lazy_tlb(oldmm, next);
} else
switch_mm_irqs_off(oldmm, mm, next);
@@ -7667,7 +7667,7 @@ void __init sched_init(void)
/*
* The boot idle thread does lazy MMU switching as well:
*/
- atomic_inc(&init_mm.mm_count);
+ mmgrab(&init_mm);
enter_lazy_tlb(&init_mm, current);
/*
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 87e1a7ca3846..1343271a18f1 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -420,7 +420,7 @@ int __khugepaged_enter(struct mm_struct *mm)
list_add_tail(&mm_slot->mm_node, &khugepaged_scan.mm_head);
spin_unlock(&khugepaged_mm_lock);
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
if (wakeup)
wake_up_interruptible(&khugepaged_wait);
diff --git a/mm/ksm.c b/mm/ksm.c
index 9ae6011a41f8..5a49aad9d87b 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1813,7 +1813,7 @@ int __ksm_enter(struct mm_struct *mm)
spin_unlock(&ksm_mmlist_lock);
set_bit(MMF_VM_MERGEABLE, &mm->flags);
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
if (needs_wakeup)
wake_up_interruptible(&ksm_thread_wait);
diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index 6f4d27c5bb32..daf67bb02b4a 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -25,7 +25,7 @@ void use_mm(struct mm_struct *mm)
task_lock(tsk);
active_mm = tsk->active_mm;
if (active_mm != mm) {
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
tsk->active_mm = mm;
}
tsk->mm = mm;
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
index f4259e496f83..32bc9f2ff7eb 100644
--- a/mm/mmu_notifier.c
+++ b/mm/mmu_notifier.c
@@ -275,7 +275,7 @@ static int do_mmu_notifier_register(struct mmu_notifier *mn,
mm->mmu_notifier_mm = mmu_notifier_mm;
mmu_notifier_mm = NULL;
}
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
/*
* Serialize the update against mmu_notifier_unregister. A
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ec9f11d4f094..ead093c6f2a6 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -660,7 +660,7 @@ static void mark_oom_victim(struct task_struct *tsk)
/* oom_mm is bound to the signal struct life time. */
if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
- atomic_inc(&tsk->signal->oom_mm->mm_count);
+ mmgrab(tsk->signal->oom_mm);
/*
* Make sure that the task is woken up from uninterruptible sleep
@@ -877,7 +877,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
/* Get a reference to safely compare mm after task_unlock(victim) */
mm = victim->mm;
- atomic_inc(&mm->mm_count);
+ mmgrab(mm);
/*
* We should send SIGKILL before setting TIF_MEMDIE in order to prevent
* the OOM victim from depleting the memory reserves from the user
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7f9ee2929cfe..43914b981691 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -613,7 +613,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
return ERR_PTR(-ENOMEM);
spin_lock_init(&kvm->mmu_lock);
- atomic_inc(¤t->mm->mm_count);
+ mmgrab(current->mm);
kvm->mm = current->mm;
kvm_eventfd_init(kvm);
mutex_init(&kvm->lock);
--
2.11.0.1.gaa10c3f
[toc] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 10:10 +0100 |
| Subject | Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging |
| Message-ID | <sOWGD-1t7-57@gated-at.bofh.it> |
| In reply to | #1543269 |
On Fri 16-12-16 09:22:02, Vegard Nossum wrote: > Reference counting bugs are hard to debug by their nature since the actual > manifestation of one can occur very far from where the error is introduced > (e.g. a missing get() only manifest as a use-after-free when the reference > count prematurely drops to 0, which could be arbitrarily long after where > the get() should have happened if there are other users). I wrote this patch > to try to track down a suspected 'mm_struct' reference counting bug. I definitely agree that hunting these bugs is a royal PITA, no question about that. I am just wondering whether this has been motivated by any particular bug recently. I do not seem to remember any such an issue for quite some time. > The basic idea is to keep track of all references, not just with a reference > counter, but with an actual reference _list_. Whenever you get() or put() a > reference, you also add or remove yourself, respectively, from the reference > list. This really helps debugging because (for example) you always put a > specific reference, meaning that if that reference was not yours to put, you > will notice it immediately (rather than when the reference counter goes to 0 > and you still have an active reference). But who is the owner of the reference? A function/task? It is not all that uncommon to take an mm reference from one context and release it from a different one. But I might be missing your point here. > The main interface is in <linux/mm_ref_types.h> and <linux/mm_ref.h>, while > the implementation lives in mm/mm_ref.c. Since 'struct mm_struct' has both > ->mm_users and ->mm_count, we introduce helpers for both of them, but use > the same data structure for each (struct mm_ref). The low-level rules (i.e. > the ones we have to follow, but which nobody else should really have to > care about since they use the higher-level interface) are: > > - after incrementing ->mm_count you also have to call get_mm_ref() > > - before decrementing ->mm_count you also have to call put_mm_ref() > > - after incrementing ->mm_users you also have to call get_mm_users_ref() > > - before decrementing ->mm_users you also have to call put_mm_users_ref() > > The rules that most of the rest of the kernel will care about are: > > - functions that acquire and return a mm_struct should take a > 'struct mm_ref *' which it can pass on to mmget()/mmgrab()/etc. > > - functions that release an mm_struct passed as a parameter should also > take a 'struct mm_ref *' which it can pass on to mmput()/mmdrop()/etc. > > - any function that temporarily acquires a mm_struct reference should > use MM_REF() to define an on-stack reference and pass it on to > mmget()/mmput()/mmgrab()/mmdrop()/etc. > > - any structure that holds an mm_struct pointer must also include a > 'struct mm_ref' member; when the mm_struct pointer is modified you > would typically also call mmget()/mmgrab()/mmput()/mmdrop() and they > should be called with this mm_ref > > - you can convert (for example) an on-stack reference to an in-struct > reference using move_mm_ref(). This is semantically equivalent to > (atomically) taking the new reference and dropping the old one, but > doesn't actually need to modify the reference count This all sounds way too intrusive to me so I am not really sure this is something we really want. A nice thing for debugging for sure but I am somehow skeptical whether it is really worth it considering how many those ref. count bugs we've had. [...] -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| Date | 2016-12-16 10:50 +0100 |
| Subject | Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging |
| Message-ID | <sOXjk-1Nk-25@gated-at.bofh.it> |
| In reply to | #1543283 |
On 12/16/2016 10:01 AM, Michal Hocko wrote:
> On Fri 16-12-16 09:22:02, Vegard Nossum wrote:
>> Reference counting bugs are hard to debug by their nature since the actual
>> manifestation of one can occur very far from where the error is introduced
>> (e.g. a missing get() only manifest as a use-after-free when the reference
>> count prematurely drops to 0, which could be arbitrarily long after where
>> the get() should have happened if there are other users). I wrote this patch
>> to try to track down a suspected 'mm_struct' reference counting bug.
>
> I definitely agree that hunting these bugs is a royal PITA, no question
> about that. I am just wondering whether this has been motivated by any
> particular bug recently. I do not seem to remember any such an issue for
> quite some time.
Yes, I've been hitting a use-after-free with trinity that happens when
the OOM killer reaps a task. I can reproduce it reliably within a few
seconds, but with the amount of refs and syscalls going on I haven't
been able to figure out what's actually going wrong (to put things into
perspective the refcounts goes into the thousands before eventually
dropping down to 0 and trying to trace_printk() each get/put results in
several hundred megabytes of log files).
The UAF itself (sometimes a NULL pointer deref) is on a struct file
(sometimes in the page fault path, sometimes in clone(), sometimes in
execve()), and my initial debugging lead me to believe it was actually a
problem with mm_struct getting freed prematurely (hence this patch). But
disappointingly this patch didn't turn up anything so I must reevaluate
my suspicion of an mm_struct leak.
I don't think it's a bug in the OOM reaper itself, but either of the
following two patches will fix the problem (without my understand how or
why):
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ec9f11d4f094..37b14b2e2af4 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct
*tsk, struct mm_struct *mm)
*/
mutex_lock(&oom_lock);
- if (!down_read_trylock(&mm->mmap_sem)) {
+ if (!down_write_trylock(&mm->mmap_sem)) {
ret = false;
goto unlock_oom;
}
@@ -496,7 +496,7 @@ static bool __oom_reap_task_mm(struct task_struct
*tsk, struct mm_struct *mm)
* and delayed __mmput doesn't matter that much
*/
if (!mmget_not_zero(mm)) {
- up_read(&mm->mmap_sem);
+ up_write(&mm->mmap_sem);
goto unlock_oom;
}
@@ -540,7 +540,7 @@ static bool __oom_reap_task_mm(struct task_struct
*tsk, struct mm_struct *mm)
K(get_mm_counter(mm, MM_ANONPAGES)),
K(get_mm_counter(mm, MM_FILEPAGES)),
K(get_mm_counter(mm, MM_SHMEMPAGES)));
- up_read(&mm->mmap_sem);
+ up_write(&mm->mmap_sem);
/*
* Drop our reference but make sure the mmput slow path is called from a
--OR--
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ec9f11d4f094..559aec0acd21 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -508,6 +508,7 @@ static bool __oom_reap_task_mm(struct task_struct
*tsk, struct mm_struct *mm)
*/
set_bit(MMF_UNSTABLE, &mm->flags);
+#if 0
tlb_gather_mmu(&tlb, mm, 0, -1);
for (vma = mm->mmap ; vma; vma = vma->vm_next) {
if (is_vm_hugetlb_page(vma))
@@ -535,6 +536,7 @@ static bool __oom_reap_task_mm(struct task_struct
*tsk, struct mm_struct *mm)
&details);
}
tlb_finish_mmu(&tlb, 0, -1);
+#endif
pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB,
file-rss:%lukB, shmem-rss:%lukB\n",
task_pid_nr(tsk), tsk->comm,
K(get_mm_counter(mm, MM_ANONPAGES)),
Maybe it's just the fact that we're not releasing the memory and so some
other bit of code is not able to make enough progress to trigger the
bug, although curiously, if I just move the #if 0..#endif inside
tlb_gather_mmu()..tlb_finish_mmu() itself (so just calling tlb_*()
without doing the for-loop), it still reproduces the crash.
Another clue, although it might just be a coincidence, is that it seems
the VMA/file in question is always a mapping for the exe file itself
(the reason I think this might be a coincidence is that the exe file
mapping is the first one and we usually traverse VMAs starting with this
one, that doesn't mean the other VMAs aren't affected by the same
problem, just that we never hit them).
I really wanted to figure out and fix the bug myself, it's a great way
to learn, after all, instead of just sending crash logs and letting
somebody else figure it out. But maybe I have to admit defeat on this one.
>> The basic idea is to keep track of all references, not just with a reference
>> counter, but with an actual reference _list_. Whenever you get() or put() a
>> reference, you also add or remove yourself, respectively, from the reference
>> list. This really helps debugging because (for example) you always put a
>> specific reference, meaning that if that reference was not yours to put, you
>> will notice it immediately (rather than when the reference counter goes to 0
>> and you still have an active reference).
>
> But who is the owner of the reference? A function/task? It is not all
> that uncommon to take an mm reference from one context and release it
> from a different one. But I might be missing your point here.
An owner is somebody who knows the pointer and increments the reference
counter for it.
You'll notice a bunch of functions just take a temporary on-stack
reference (e.g. struct mm_struct *mm = get_task_mm(tsk); ...
mmput(&mm)), in which case it's the function that owns the reference
until the mmput.
Some functions take a reference and stash it in some heap object, an
example from the patch could be 'struct vhost_dev' (which has a ->mm
field), and it does get_task_mm() in an init function and mmput() in a
cleanup function. In this case, it's the struct which is the owner of
the reference, for as long as ->mm points to something non-NULL. This
would be an example of taking the reference in one context and releasing
it in a different one. I guess the point is that we must always release
a _specific_ reference when we decrement a reference count. Yes, it's a
number, but that number does refer to a specific reference that was
taken at some point in the past (and we should know
which reference this is, otherwise we don't actually "have it").
We may not be used to thinking of reference counts as actual places of
reference, but that's what it is, fundamentally. This patch just makes
it very explicit what the owners are and where ownership transfers take
place.
>> The main interface is in <linux/mm_ref_types.h> and <linux/mm_ref.h>, while
>> the implementation lives in mm/mm_ref.c. Since 'struct mm_struct' has both
>> ->mm_users and ->mm_count, we introduce helpers for both of them, but use
>> the same data structure for each (struct mm_ref). The low-level rules (i.e.
>> the ones we have to follow, but which nobody else should really have to
>> care about since they use the higher-level interface) are:
[...]
>
> This all sounds way too intrusive to me so I am not really sure this is
> something we really want. A nice thing for debugging for sure but I am
> somehow skeptical whether it is really worth it considering how many
> those ref. count bugs we've had.
Yeah, I agree it's intrusive. And it did start out as just a debugging
patch, but I figured after having done all the work I might as well
slap on a changelog and submit it to see what people think.
However, it may have some value as documentation of who is the owner of
each reference and where/when those owners change. Maybe I should just
extract that knowledge and add it in as comments instead.
Thanks for your comments!
Vegard
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 11:20 +0100 |
| Subject | crash during oom reaper (was: Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging) |
| Message-ID | <sOXMm-2df-27@gated-at.bofh.it> |
| In reply to | #1543323 |
On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
[...]
> I don't think it's a bug in the OOM reaper itself, but either of the
> following two patches will fix the problem (without my understand how or
> why):
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index ec9f11d4f094..37b14b2e2af4 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> struct mm_struct *mm)
> */
> mutex_lock(&oom_lock);
>
> - if (!down_read_trylock(&mm->mmap_sem)) {
> + if (!down_write_trylock(&mm->mmap_sem)) {
__oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
doesn't require the exlusive mmap_sem. So this looks correct to me.
[...]
> --OR--
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index ec9f11d4f094..559aec0acd21 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -508,6 +508,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> struct mm_struct *mm)
> */
> set_bit(MMF_UNSTABLE, &mm->flags);
>
> +#if 0
> tlb_gather_mmu(&tlb, mm, 0, -1);
> for (vma = mm->mmap ; vma; vma = vma->vm_next) {
> if (is_vm_hugetlb_page(vma))
> @@ -535,6 +536,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> struct mm_struct *mm)
> &details);
> }
> tlb_finish_mmu(&tlb, 0, -1);
> +#endif
same here, nothing different from the madvise... Well, except for the
MMF_UNSTABLE part which will force any page fault on this mm to SEGV.
> pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB,
> file-rss:%lukB, shmem-rss:%lukB\n",
> task_pid_nr(tsk), tsk->comm,
> K(get_mm_counter(mm, MM_ANONPAGES)),
>
> Maybe it's just the fact that we're not releasing the memory and so some
> other bit of code is not able to make enough progress to trigger the
> bug, although curiously, if I just move the #if 0..#endif inside
> tlb_gather_mmu()..tlb_finish_mmu() itself (so just calling tlb_*()
> without doing the for-loop), it still reproduces the crash.
What is the atual crash?
> Another clue, although it might just be a coincidence, is that it seems
> the VMA/file in question is always a mapping for the exe file itself
> (the reason I think this might be a coincidence is that the exe file
> mapping is the first one and we usually traverse VMAs starting with this
> one, that doesn't mean the other VMAs aren't affected by the same
> problem, just that we never hit them).
You can experiment a bit and exclude PROT_EXEC vmas...
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | "Kirill A. Shutemov" <kirill@shutemov.name> |
|---|---|
| Date | 2016-12-16 11:50 +0100 |
| Subject | Re: crash during oom reaper (was: Re: [PATCH 4/4] [RFC!] mm: 'struct mm_struct' reference counting debugging) |
| Message-ID | <sOYfn-2oy-3@gated-at.bofh.it> |
| In reply to | #1543339 |
On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> [...]
> > I don't think it's a bug in the OOM reaper itself, but either of the
> > following two patches will fix the problem (without my understand how or
> > why):
> >
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index ec9f11d4f094..37b14b2e2af4 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > struct mm_struct *mm)
> > */
> > mutex_lock(&oom_lock);
> >
> > - if (!down_read_trylock(&mm->mmap_sem)) {
> > + if (!down_write_trylock(&mm->mmap_sem)) {
>
> __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> doesn't require the exlusive mmap_sem. So this looks correct to me.
BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
least.
MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
on __oom_reap_task_mm() side.
Other difference is that you use unmap_page_range() witch doesn't touch
mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
the range. Not sure if it can make any difference here.
--
Kirill A. Shutemov
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 13:00 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sOZl8-36c-23@gated-at.bofh.it> |
| In reply to | #1543347 |
On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
> On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> > On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> > [...]
> > > I don't think it's a bug in the OOM reaper itself, but either of the
> > > following two patches will fix the problem (without my understand how or
> > > why):
> > >
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index ec9f11d4f094..37b14b2e2af4 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > > struct mm_struct *mm)
> > > */
> > > mutex_lock(&oom_lock);
> > >
> > > - if (!down_read_trylock(&mm->mmap_sem)) {
> > > + if (!down_write_trylock(&mm->mmap_sem)) {
> >
> > __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> > doesn't require the exlusive mmap_sem. So this looks correct to me.
>
> BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
> least.
>
> MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
> on __oom_reap_task_mm() side.
I guess you are right and we should match the MADV_DONTNEED behavior
here. Care to send a patch?
> Other difference is that you use unmap_page_range() witch doesn't touch
> mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
> the range. Not sure if it can make any difference here.
Which mmu notifier would care about this? I am not really familiar with
those users so I might miss something easily.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 13:20 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sOZEt-3tv-5@gated-at.bofh.it> |
| In reply to | #1543415 |
On Fri 16-12-16 12:42:43, Michal Hocko wrote:
> On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
> > On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> > > On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> > > [...]
> > > > I don't think it's a bug in the OOM reaper itself, but either of the
> > > > following two patches will fix the problem (without my understand how or
> > > > why):
> > > >
> > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > index ec9f11d4f094..37b14b2e2af4 100644
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > > > struct mm_struct *mm)
> > > > */
> > > > mutex_lock(&oom_lock);
> > > >
> > > > - if (!down_read_trylock(&mm->mmap_sem)) {
> > > > + if (!down_write_trylock(&mm->mmap_sem)) {
> > >
> > > __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> > > doesn't require the exlusive mmap_sem. So this looks correct to me.
> >
> > BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
> > least.
> >
> > MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
> > on __oom_reap_task_mm() side.
>
> I guess you are right and we should match the MADV_DONTNEED behavior
> here. Care to send a patch?
>
> > Other difference is that you use unmap_page_range() witch doesn't touch
> > mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
> > the range. Not sure if it can make any difference here.
>
> Which mmu notifier would care about this? I am not really familiar with
> those users so I might miss something easily.
Just forgot to add. Unlike the MADV_DONTNEED, there is nobody who should
observe the address space of the oom killed (and reaped) task so why
should notifiers matter in the first place?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | "Kirill A. Shutemov" <kirill@shutemov.name> |
|---|---|
| Date | 2016-12-16 13:50 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP07v-3Df-7@gated-at.bofh.it> |
| In reply to | #1543415 |
On Fri, Dec 16, 2016 at 12:42:43PM +0100, Michal Hocko wrote:
> On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
> > On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> > > On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> > > [...]
> > > > I don't think it's a bug in the OOM reaper itself, but either of the
> > > > following two patches will fix the problem (without my understand how or
> > > > why):
> > > >
> > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > index ec9f11d4f094..37b14b2e2af4 100644
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > > > struct mm_struct *mm)
> > > > */
> > > > mutex_lock(&oom_lock);
> > > >
> > > > - if (!down_read_trylock(&mm->mmap_sem)) {
> > > > + if (!down_write_trylock(&mm->mmap_sem)) {
> > >
> > > __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> > > doesn't require the exlusive mmap_sem. So this looks correct to me.
> >
> > BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
> > least.
> >
> > MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
> > on __oom_reap_task_mm() side.
>
> I guess you are right and we should match the MADV_DONTNEED behavior
> here. Care to send a patch?
Below. Testing required.
> > Other difference is that you use unmap_page_range() witch doesn't touch
> > mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
> > the range. Not sure if it can make any difference here.
>
> Which mmu notifier would care about this? I am not really familiar with
> those users so I might miss something easily.
No idea either.
Is there any reason not to use zap_page_range here too?
Few more notes:
I propably miss something, but why do we need details->ignore_dirty?
It only appiled for non-anon pages, but since we filter out shared
mappings, how can we have pte_dirty() for !PageAnon()?
check_swap_entries is also sloppy: the behavior doesn't match the comment:
details == NULL makes it check swap entries. I removed it and restore
details->check_mapping test as we had before.
After the change no user of zap_page_range() wants non-NULL details, I've
dropped the argument.
If it looks okay, I'll split it into several patches with proper commit
messages.
-----8<-----
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index ec1f0dedb948..59ac93714fa4 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -687,7 +687,7 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
/* Find vma in the parent mm */
vma = find_vma(gmap->mm, vmaddr);
size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
- zap_page_range(vma, vmaddr, size, NULL);
+ zap_page_range(vma, vmaddr, size);
}
up_read(&gmap->mm->mmap_sem);
}
diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index e4f800999b32..4bfb31e79d5d 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -796,7 +796,7 @@ static noinline int zap_bt_entries_mapping(struct mm_struct *mm,
return -EINVAL;
len = min(vma->vm_end, end) - addr;
- zap_page_range(vma, addr, len, NULL);
+ zap_page_range(vma, addr, len);
trace_mpx_unmap_zap(addr, addr+len);
vma = vma->vm_next;
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 3c71b982bf2a..d97f6725cf8c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -629,7 +629,7 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
page = &proc->pages[(page_addr - proc->buffer) / PAGE_SIZE];
if (vma)
zap_page_range(vma, (uintptr_t)page_addr +
- proc->user_buffer_offset, PAGE_SIZE, NULL);
+ proc->user_buffer_offset, PAGE_SIZE);
err_vm_insert_page_failed:
unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
err_map_kernel_failed:
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index b653451843c8..0fb0e28ace70 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -865,8 +865,7 @@ static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
list_for_each_entry(vma_list, &buffer->vmas, list) {
struct vm_area_struct *vma = vma_list->vma;
- zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start,
- NULL);
+ zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start);
}
mutex_unlock(&buffer->lock);
}
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 4424784ac374..92dcada8caaf 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1148,8 +1148,6 @@ struct zap_details {
struct address_space *check_mapping; /* Check page->mapping if set */
pgoff_t first_index; /* Lowest page->index to unmap */
pgoff_t last_index; /* Highest page->index to unmap */
- bool ignore_dirty; /* Ignore dirty pages */
- bool check_swap_entries; /* Check also swap entries */
};
struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
@@ -1160,7 +1158,7 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
unsigned long size);
void zap_page_range(struct vm_area_struct *vma, unsigned long address,
- unsigned long size, struct zap_details *);
+ unsigned long size);
void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
unsigned long start, unsigned long end);
diff --git a/mm/internal.h b/mm/internal.h
index 44d68895a9b9..5c355855e4ad 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -41,10 +41,9 @@ int do_swap_page(struct vm_fault *vmf);
void free_pgtables(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
unsigned long floor, unsigned long ceiling);
-void unmap_page_range(struct mmu_gather *tlb,
- struct vm_area_struct *vma,
- unsigned long addr, unsigned long end,
- struct zap_details *details);
+long madvise_dontneed(struct vm_area_struct *vma,
+ struct vm_area_struct **prev,
+ unsigned long start, unsigned long end);
extern int __do_page_cache_readahead(struct address_space *mapping,
struct file *filp, pgoff_t offset, unsigned long nr_to_read,
diff --git a/mm/madvise.c b/mm/madvise.c
index 0e3828eae9f8..8c9f19b62b4a 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -468,7 +468,7 @@ static long madvise_free(struct vm_area_struct *vma,
* An interface that causes the system to free clean pages and flush
* dirty pages is already available as msync(MS_INVALIDATE).
*/
-static long madvise_dontneed(struct vm_area_struct *vma,
+long madvise_dontneed(struct vm_area_struct *vma,
struct vm_area_struct **prev,
unsigned long start, unsigned long end)
{
@@ -476,7 +476,7 @@ static long madvise_dontneed(struct vm_area_struct *vma,
if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
return -EINVAL;
- zap_page_range(vma, start, end - start, NULL);
+ zap_page_range(vma, start, end - start);
return 0;
}
diff --git a/mm/memory.c b/mm/memory.c
index 455c3e628d52..f8836232a492 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1155,12 +1155,6 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
if (!PageAnon(page)) {
if (pte_dirty(ptent)) {
- /*
- * oom_reaper cannot tear down dirty
- * pages
- */
- if (unlikely(details && details->ignore_dirty))
- continue;
force_flush = 1;
set_page_dirty(page);
}
@@ -1179,8 +1173,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
}
continue;
}
- /* only check swap_entries if explicitly asked for in details */
- if (unlikely(details && !details->check_swap_entries))
+ /* If details->check_mapping, we leave swap entries. */
+ if (unlikely(details))
continue;
entry = pte_to_swp_entry(ptent);
@@ -1277,7 +1271,7 @@ static inline unsigned long zap_pud_range(struct mmu_gather *tlb,
return addr;
}
-void unmap_page_range(struct mmu_gather *tlb,
+static void unmap_page_range(struct mmu_gather *tlb,
struct vm_area_struct *vma,
unsigned long addr, unsigned long end,
struct zap_details *details)
@@ -1381,7 +1375,7 @@ void unmap_vmas(struct mmu_gather *tlb,
* Caller must protect the VMA list
*/
void zap_page_range(struct vm_area_struct *vma, unsigned long start,
- unsigned long size, struct zap_details *details)
+ unsigned long size)
{
struct mm_struct *mm = vma->vm_mm;
struct mmu_gather tlb;
@@ -1392,7 +1386,7 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start,
update_hiwater_rss(mm);
mmu_notifier_invalidate_range_start(mm, start, end);
for ( ; vma && vma->vm_start < end; vma = vma->vm_next)
- unmap_single_vma(&tlb, vma, start, end, details);
+ unmap_single_vma(&tlb, vma, start, end, NULL);
mmu_notifier_invalidate_range_end(mm, start, end);
tlb_finish_mmu(&tlb, start, end);
}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ec9f11d4f094..f6451eacb0aa 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -465,8 +465,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
{
struct mmu_gather tlb;
struct vm_area_struct *vma;
- struct zap_details details = {.check_swap_entries = true,
- .ignore_dirty = true};
bool ret = true;
/*
@@ -481,7 +479,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
* out_of_memory
* select_bad_process
* # no TIF_MEMDIE task selects new victim
- * unmap_page_range # frees some memory
+ * madv_dontneed # frees some memory
*/
mutex_lock(&oom_lock);
@@ -510,16 +508,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
tlb_gather_mmu(&tlb, mm, 0, -1);
for (vma = mm->mmap ; vma; vma = vma->vm_next) {
- if (is_vm_hugetlb_page(vma))
- continue;
-
- /*
- * mlocked VMAs require explicit munlocking before unmap.
- * Let's keep it simple here and skip such VMAs.
- */
- if (vma->vm_flags & VM_LOCKED)
- continue;
-
/*
* Only anonymous pages have a good chance to be dropped
* without additional steps which we cannot afford as we
@@ -531,8 +519,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
* count elevated without a good reason.
*/
if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
- unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
- &details);
+ madvise_dontneed(vma, &vma, vma->vm_start, vma->vm_end);
}
tlb_finish_mmu(&tlb, 0, -1);
pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
--
Kirill A. Shutemov
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 14:00 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP0hc-3GN-25@gated-at.bofh.it> |
| In reply to | #1543442 |
On Fri 16-12-16 15:35:55, Kirill A. Shutemov wrote:
> On Fri, Dec 16, 2016 at 12:42:43PM +0100, Michal Hocko wrote:
> > On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
> > > On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> > > > On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> > > > [...]
> > > > > I don't think it's a bug in the OOM reaper itself, but either of the
> > > > > following two patches will fix the problem (without my understand how or
> > > > > why):
> > > > >
> > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > index ec9f11d4f094..37b14b2e2af4 100644
> > > > > --- a/mm/oom_kill.c
> > > > > +++ b/mm/oom_kill.c
> > > > > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > > > > struct mm_struct *mm)
> > > > > */
> > > > > mutex_lock(&oom_lock);
> > > > >
> > > > > - if (!down_read_trylock(&mm->mmap_sem)) {
> > > > > + if (!down_write_trylock(&mm->mmap_sem)) {
> > > >
> > > > __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> > > > doesn't require the exlusive mmap_sem. So this looks correct to me.
> > >
> > > BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
> > > least.
> > >
> > > MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
> > > on __oom_reap_task_mm() side.
> >
> > I guess you are right and we should match the MADV_DONTNEED behavior
> > here. Care to send a patch?
>
> Below. Testing required.
>
> > > Other difference is that you use unmap_page_range() witch doesn't touch
> > > mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
> > > the range. Not sure if it can make any difference here.
> >
> > Which mmu notifier would care about this? I am not really familiar with
> > those users so I might miss something easily.
>
> No idea either.
>
> Is there any reason not to use zap_page_range here too?
Yes, zap_page_range is much more heavy and performs operations which
might lock AFAIR which I really would like to prevent from.
> Few more notes:
>
> I propably miss something, but why do we need details->ignore_dirty?
>
> It only appiled for non-anon pages, but since we filter out shared
> mappings, how can we have pte_dirty() for !PageAnon()?
Why couldn't we have dirty pages on the private file mappings? The
underlying page might be still in the page cache, right?
> check_swap_entries is also sloppy: the behavior doesn't match the comment:
> details == NULL makes it check swap entries. I removed it and restore
> details->check_mapping test as we had before.
the reason is unmap_mapping_range which didn't use to check swap entries
so I wanted to have it opt in AFAIR.
> @@ -531,8 +519,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
> * count elevated without a good reason.
> */
> if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
> - unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
> - &details);
> + madvise_dontneed(vma, &vma, vma->vm_start, vma->vm_end);
I would rather keep the unmap_page_range because it is the bare minumum
we have to do. Currently we are doing
if (is_vm_hugetlb_page(vma))
continue;
so I would rather do something like
if (!can_vma_madv_dontneed(vma))
continue;
instead.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | "Kirill A. Shutemov" <kirill@shutemov.name> |
|---|---|
| Date | 2016-12-16 14:10 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP0qS-42X-17@gated-at.bofh.it> |
| In reply to | #1543447 |
On Fri, Dec 16, 2016 at 01:56:50PM +0100, Michal Hocko wrote:
> On Fri 16-12-16 15:35:55, Kirill A. Shutemov wrote:
> > On Fri, Dec 16, 2016 at 12:42:43PM +0100, Michal Hocko wrote:
> > > On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
> > > > On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> > > > > On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> > > > > [...]
> > > > > > I don't think it's a bug in the OOM reaper itself, but either of the
> > > > > > following two patches will fix the problem (without my understand how or
> > > > > > why):
> > > > > >
> > > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > > index ec9f11d4f094..37b14b2e2af4 100644
> > > > > > --- a/mm/oom_kill.c
> > > > > > +++ b/mm/oom_kill.c
> > > > > > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > > > > > struct mm_struct *mm)
> > > > > > */
> > > > > > mutex_lock(&oom_lock);
> > > > > >
> > > > > > - if (!down_read_trylock(&mm->mmap_sem)) {
> > > > > > + if (!down_write_trylock(&mm->mmap_sem)) {
> > > > >
> > > > > __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> > > > > doesn't require the exlusive mmap_sem. So this looks correct to me.
> > > >
> > > > BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
> > > > least.
> > > >
> > > > MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
> > > > on __oom_reap_task_mm() side.
> > >
> > > I guess you are right and we should match the MADV_DONTNEED behavior
> > > here. Care to send a patch?
> >
> > Below. Testing required.
> >
> > > > Other difference is that you use unmap_page_range() witch doesn't touch
> > > > mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
> > > > the range. Not sure if it can make any difference here.
> > >
> > > Which mmu notifier would care about this? I am not really familiar with
> > > those users so I might miss something easily.
> >
> > No idea either.
> >
> > Is there any reason not to use zap_page_range here too?
>
> Yes, zap_page_range is much more heavy and performs operations which
> might lock AFAIR which I really would like to prevent from.
What exactly can block there? I don't see anything with that potential.
> > Few more notes:
> >
> > I propably miss something, but why do we need details->ignore_dirty?
> >
> > It only appiled for non-anon pages, but since we filter out shared
> > mappings, how can we have pte_dirty() for !PageAnon()?
>
> Why couldn't we have dirty pages on the private file mappings? The
> underlying page might be still in the page cache, right?
The check is about dirty PTE, not dirty page.
> > check_swap_entries is also sloppy: the behavior doesn't match the comment:
> > details == NULL makes it check swap entries. I removed it and restore
> > details->check_mapping test as we had before.
>
> the reason is unmap_mapping_range which didn't use to check swap entries
> so I wanted to have it opt in AFAIR.
details == NULL would give you it in both cases.
> > @@ -531,8 +519,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
> > * count elevated without a good reason.
> > */
> > if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
> > - unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
> > - &details);
> > + madvise_dontneed(vma, &vma, vma->vm_start, vma->vm_end);
>
> I would rather keep the unmap_page_range because it is the bare minumum
> we have to do. Currently we are doing
>
> if (is_vm_hugetlb_page(vma))
> continue;
>
> so I would rather do something like
> if (!can_vma_madv_dontneed(vma))
> continue;
> instead.
We can do that.
But let's first understand why code should differ from madvise_dontneed().
It's not obvious to me.
--
Kirill A. Shutemov
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 14:20 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP0Ax-4ds-1@gated-at.bofh.it> |
| In reply to | #1543454 |
On Fri 16-12-16 16:07:30, Kirill A. Shutemov wrote:
> On Fri, Dec 16, 2016 at 01:56:50PM +0100, Michal Hocko wrote:
> > On Fri 16-12-16 15:35:55, Kirill A. Shutemov wrote:
> > > On Fri, Dec 16, 2016 at 12:42:43PM +0100, Michal Hocko wrote:
> > > > On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
> > > > > On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
> > > > > > On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> > > > > > [...]
> > > > > > > I don't think it's a bug in the OOM reaper itself, but either of the
> > > > > > > following two patches will fix the problem (without my understand how or
> > > > > > > why):
> > > > > > >
> > > > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > > > index ec9f11d4f094..37b14b2e2af4 100644
> > > > > > > --- a/mm/oom_kill.c
> > > > > > > +++ b/mm/oom_kill.c
> > > > > > > @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
> > > > > > > struct mm_struct *mm)
> > > > > > > */
> > > > > > > mutex_lock(&oom_lock);
> > > > > > >
> > > > > > > - if (!down_read_trylock(&mm->mmap_sem)) {
> > > > > > > + if (!down_write_trylock(&mm->mmap_sem)) {
> > > > > >
> > > > > > __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
> > > > > > doesn't require the exlusive mmap_sem. So this looks correct to me.
> > > > >
> > > > > BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
> > > > > least.
> > > > >
> > > > > MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
> > > > > on __oom_reap_task_mm() side.
> > > >
> > > > I guess you are right and we should match the MADV_DONTNEED behavior
> > > > here. Care to send a patch?
> > >
> > > Below. Testing required.
> > >
> > > > > Other difference is that you use unmap_page_range() witch doesn't touch
> > > > > mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
> > > > > the range. Not sure if it can make any difference here.
> > > >
> > > > Which mmu notifier would care about this? I am not really familiar with
> > > > those users so I might miss something easily.
> > >
> > > No idea either.
> > >
> > > Is there any reason not to use zap_page_range here too?
> >
> > Yes, zap_page_range is much more heavy and performs operations which
> > might lock AFAIR which I really would like to prevent from.
>
> What exactly can block there? I don't see anything with that potential.
I would have to rememeber all the details. This is mostly off-topic for
this particular thread so I think it would be better if you could send a
full patch separatelly and we can discuss it there?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2016-12-18 14:50 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sPK0F-8tB-7@gated-at.bofh.it> |
| In reply to | #1543455 |
On 2016/12/16 22:14, Michal Hocko wrote:
> On Fri 16-12-16 16:07:30, Kirill A. Shutemov wrote:
>> On Fri, Dec 16, 2016 at 01:56:50PM +0100, Michal Hocko wrote:
>>> On Fri 16-12-16 15:35:55, Kirill A. Shutemov wrote:
>>>> On Fri, Dec 16, 2016 at 12:42:43PM +0100, Michal Hocko wrote:
>>>>> On Fri 16-12-16 13:44:38, Kirill A. Shutemov wrote:
>>>>>> On Fri, Dec 16, 2016 at 11:11:13AM +0100, Michal Hocko wrote:
>>>>>>> On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
>>>>>>> [...]
>>>>>>>> I don't think it's a bug in the OOM reaper itself, but either of the
>>>>>>>> following two patches will fix the problem (without my understand how or
>>>>>>>> why):
>>>>>>>>
>>>>>>>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>>>>>>>> index ec9f11d4f094..37b14b2e2af4 100644
>>>>>>>> --- a/mm/oom_kill.c
>>>>>>>> +++ b/mm/oom_kill.c
>>>>>>>> @@ -485,7 +485,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk,
>>>>>>>> struct mm_struct *mm)
>>>>>>>> */
>>>>>>>> mutex_lock(&oom_lock);
>>>>>>>>
>>>>>>>> - if (!down_read_trylock(&mm->mmap_sem)) {
>>>>>>>> + if (!down_write_trylock(&mm->mmap_sem)) {
>>>>>>>
>>>>>>> __oom_reap_task_mm is basically the same thing as MADV_DONTNEED and that
>>>>>>> doesn't require the exlusive mmap_sem. So this looks correct to me.
>>>>>>
>>>>>> BTW, shouldn't we filter out all VM_SPECIAL VMAs there? Or VM_PFNMAP at
>>>>>> least.
>>>>>>
>>>>>> MADV_DONTNEED doesn't touch VM_PFNMAP, but I don't see anything matching
>>>>>> on __oom_reap_task_mm() side.
>>>>>
>>>>> I guess you are right and we should match the MADV_DONTNEED behavior
>>>>> here. Care to send a patch?
>>>>
>>>> Below. Testing required.
>>>>
>>>>>> Other difference is that you use unmap_page_range() witch doesn't touch
>>>>>> mmu_notifiers. MADV_DONTNEED goes via zap_page_range(), which invalidates
>>>>>> the range. Not sure if it can make any difference here.
>>>>>
>>>>> Which mmu notifier would care about this? I am not really familiar with
>>>>> those users so I might miss something easily.
>>>>
>>>> No idea either.
>>>>
>>>> Is there any reason not to use zap_page_range here too?
>>>
>>> Yes, zap_page_range is much more heavy and performs operations which
>>> might lock AFAIR which I really would like to prevent from.
>>
>> What exactly can block there? I don't see anything with that potential.
>
> I would have to rememeber all the details. This is mostly off-topic for
> this particular thread so I think it would be better if you could send a
> full patch separatelly and we can discuss it there?
>
zap_page_range() calls mmu_notifier_invalidate_range_start().
mmu_notifier_invalidate_range_start() calls __mmu_notifier_invalidate_range_start().
__mmu_notifier_invalidate_range_start() calls srcu_read_lock()/srcu_read_unlock().
This means that zap_page_range() might sleep.
I don't know what individual notifier will do, but for example
static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
.invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
};
i915_gem_userptr_mn_invalidate_range_start() calls flush_workqueue()
which means that we can OOM livelock if work item involves memory allocation.
Some of other notifiers call mutex_lock()/mutex_unlock().
Even if none of currently in-tree notifier users are blocked on memory
allocation, I think it is not guaranteed that future changes/users won't be
blocked on memory allocation.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-18 17:10 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sPMc9-280-25@gated-at.bofh.it> |
| In reply to | #1544141 |
On Sun 18-12-16 22:47:07, Tetsuo Handa wrote:
> On 2016/12/16 22:14, Michal Hocko wrote:
[...]
> > I would have to rememeber all the details. This is mostly off-topic for
> > this particular thread so I think it would be better if you could send a
> > full patch separatelly and we can discuss it there?
> >
>
> zap_page_range() calls mmu_notifier_invalidate_range_start().
> mmu_notifier_invalidate_range_start() calls __mmu_notifier_invalidate_range_start().
> __mmu_notifier_invalidate_range_start() calls srcu_read_lock()/srcu_read_unlock().
> This means that zap_page_range() might sleep.
>
> I don't know what individual notifier will do, but for example
>
> static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
> .invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
> };
>
> i915_gem_userptr_mn_invalidate_range_start() calls flush_workqueue()
> which means that we can OOM livelock if work item involves memory allocation.
> Some of other notifiers call mutex_lock()/mutex_unlock().
>
> Even if none of currently in-tree notifier users are blocked on memory
> allocation, I think it is not guaranteed that future changes/users won't be
> blocked on memory allocation.
Kirill has sent this as a separate patchset [1]. Could you follow up on
that there please?
http://lkml.kernel.org/r/20161216141556.75130-4-kirill.shutemov@linux.intel.com
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| Date | 2016-12-16 14:20 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP0Ax-4ds-7@gated-at.bofh.it> |
| In reply to | #1543339 |
On 12/16/2016 11:11 AM, Michal Hocko wrote:
> On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
> [...]
>> I don't think it's a bug in the OOM reaper itself, but either of the
>> following two patches will fix the problem (without my understand how or
>> why):
>>
> What is the atual crash?
Annoyingly it doesn't seem to reproduce with the very latest
linus/master, so maybe it's been fixed recently after all and I missed it.
I've started a bisect to see what fixed it. Just in case, I added 4
different crashes I saw with various kernels. I think there may have
been a few others too (I remember seeing one in a page fault path), but
these were the most frequent ones.
Vegard
--
Manifestation 1:
Out of memory: Kill process 1650 (trinity-main) score 90 or sacrifice child
Killed process 1724 (trinity-c14) total-vm:37280kB, anon-rss:236kB,
file-rss:112kB, shmem-rss:112kB
BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8
IP: [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580
PGD c001067 PUD c000067
PMD 0
Oops: 0002 [#1] PREEMPT SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
CPU: 28 PID: 1650 Comm: trinity-main Not tainted 4.9.0-rc6+ #317
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
Ubuntu-1.8.2-1ubuntu1 04/01/2014
task: ffff88000f9bc440 task.stack: ffff88000c778000
RIP: 0010:[<ffffffff8126b1c0>] [<ffffffff8126b1c0>]
copy_process.part.41+0x2150/0x5580
RSP: 0018:ffff88000c77fc18 EFLAGS: 00010297
RAX: 0000000000000000 RBX: ffff88000fa11c00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: dffffc0000000000 RDI: ffff88000f2a33b0
RBP: ffff88000c77fdb0 R08: ffff88000c77f900 R09: 0000000000000002
R10: 00000000cb9401ca R11: 00000000c6eda739 R12: ffff88000f894d00
R13: ffff88000c7c4700 R14: ffff88000fa11c50 R15: ffff88000f2a3200
FS: 00007fb7d2a24700(0000) GS:ffff880011b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000000001e8 CR3: 000000001010d000 CR4: 00000000000406e0
Stack:
0000000000000046 0000000001200011 ffffed0001f129ac ffff88000f894d60
0000000000000000 0000000000000000 ffff88000f894d08 ffff88000f894da0
ffff88000c7a8620 ffff88000f020318 ffff88000fa11c18 ffff88000f894e40
Call Trace:
[<ffffffff81269070>] ? __cleanup_sighand+0x50/0x50
[<ffffffff81fd552e>] ? memzero_explicit+0xe/0x10
[<ffffffff822cb592>] ? urandom_read+0x232/0x4d0
[<ffffffff8126e974>] _do_fork+0x1a4/0xa40
[<ffffffff8126e7d0>] ? fork_idle+0x180/0x180
[<ffffffff81002dba>] ? syscall_trace_enter+0x3aa/0xd40
[<ffffffff815179ea>] ? __context_tracking_exit.part.4+0x9a/0x1e0
[<ffffffff81002a10>] ? exit_to_usermode_loop+0x150/0x150
[<ffffffff8201df57>] ? check_preemption_disabled+0x37/0x1e0
[<ffffffff8126f2e7>] SyS_clone+0x37/0x50
[<ffffffff83caea50>] ? ptregs_sys_rt_sigreturn+0x10/0x10
[<ffffffff8100524f>] do_syscall_64+0x1af/0x4d0
[<ffffffff83cae974>] entry_SYSCALL64_slow_path+0x25/0x25
Code: be 00 00 00 00 00 fc ff df 48 c1 e8 03 80 3c 30 00 74 08 4c 89 f7
e8 d0 7d 3c 00 f6 43 51 08 74 11 e8 45 fa 1d 00 48 8b 44 24 20 <f0> ff
88 e8 01 00 00 e8 34 fa 1d 00 48 8b 44 24 70 48 83 c0 60
RIP [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580
RSP <ffff88000c77fc18>
CR2: 00000000000001e8
---[ end trace b8f81ad60c106e75 ]---
Manifestation 2:
Killed process 1775 (trinity-c21) total-vm:37404kB, anon-rss:232kB,
file-rss:420kB, shmem-rss:116kB
oom_reaper: reaped process 1775 (trinity-c21), now anon-rss:0kB,
file-rss:0kB, shmem-rss:116kB
==================================================================
BUG: KASAN: use-after-free in p9_client_read+0x8f0/0x960 at addr
ffff880010284d00
Read of size 8 by task trinity-main/1649
CPU: 3 PID: 1649 Comm: trinity-main Not tainted 4.9.0+ #318
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
Ubuntu-1.8.2-1ubuntu1 04/01/2014
ffff8800068a7770 ffffffff82012301 ffff88001100f600 ffff880010284d00
ffff880010284d60 ffff880010284d00 ffff8800068a7798 ffffffff8165872c
ffff8800068a7828 ffff880010284d00 ffff88001100f600 ffff8800068a7818
Call Trace:
[<ffffffff82012301>] dump_stack+0x83/0xb2
[<ffffffff8165872c>] kasan_object_err+0x1c/0x70
[<ffffffff816589c5>] kasan_report_error+0x1f5/0x4e0
[<ffffffff81657d92>] ? kasan_slab_alloc+0x12/0x20
[<ffffffff82079357>] ? check_preemption_disabled+0x37/0x1e0
[<ffffffff81658e4e>] __asan_report_load8_noabort+0x3e/0x40
[<ffffffff82079300>] ? assoc_array_gc+0x1310/0x1330
[<ffffffff83b84c30>] ? p9_client_read+0x8f0/0x960
[<ffffffff83b84c30>] p9_client_read+0x8f0/0x960
[<ffffffff8207953c>] ? __this_cpu_preempt_check+0x1c/0x20
[<ffffffff81685388>] ? memcg_check_events+0x28/0x460
[<ffffffff83b84340>] ? p9_client_unlinkat+0x100/0x100
[<ffffffff8168f147>] ? mem_cgroup_commit_charge+0xb7/0x11b0
[<ffffffff815357f4>] ? __add_to_page_cache_locked+0x3a4/0x520
[<ffffffff815a6bdb>] ? __inc_node_state+0x6b/0xe0
[<ffffffff813727c7>] ? do_raw_spin_unlock+0x137/0x210
[<ffffffff820587d0>] ? iov_iter_bvec+0x30/0x120
[<ffffffff81b7d6ac>] v9fs_fid_readpage+0x15c/0x390
[<ffffffff81b7d550>] ? v9fs_write_end+0x410/0x410
[<ffffffff81571205>] ? __lru_cache_add+0x145/0x1f0
[<ffffffff815713d5>] ? lru_cache_add+0x15/0x20
[<ffffffff8153761b>] ? add_to_page_cache_lru+0x13b/0x280
[<ffffffff815374e0>] ? add_to_page_cache_locked+0x40/0x40
[<ffffffff8153786d>] ? __page_cache_alloc+0x10d/0x290
[<ffffffff81b7d91f>] v9fs_vfs_readpage+0x3f/0x50
[<ffffffff8153b188>] filemap_fault+0xbe8/0x1140
[<ffffffff8153af1d>] ? filemap_fault+0x97d/0x1140
[<ffffffff815d2eb6>] __do_fault+0x206/0x410
[<ffffffff815d2cb0>] ? do_page_mkwrite+0x320/0x320
[<ffffffff815ddc4c>] ? handle_mm_fault+0x1cc/0x2a60
[<ffffffff815df76f>] handle_mm_fault+0x1cef/0x2a60
[<ffffffff815ddbb2>] ? handle_mm_fault+0x132/0x2a60
[<ffffffff815dda80>] ? __pmd_alloc+0x370/0x370
[<ffffffff81302550>] ? dl_bw_of+0x80/0x80
[<ffffffff8123b5f0>] ? __do_page_fault+0x220/0x9f0
[<ffffffff815f1820>] ? find_vma+0x30/0x150
[<ffffffff8123b822>] __do_page_fault+0x452/0x9f0
[<ffffffff8123c075>] trace_do_page_fault+0x1e5/0x3a0
[<ffffffff8122e497>] do_async_page_fault+0x27/0xa0
[<ffffffff83d86c58>] async_page_fault+0x28/0x30
Object at ffff880010284d00, in cache kmalloc-96 size: 96
Allocated:
PID = 1649
[<ffffffff811db686>] save_stack_trace+0x16/0x20
[<ffffffff81657566>] save_stack+0x46/0xd0
[<ffffffff81657d4d>] kasan_kmalloc+0xad/0xe0
[<ffffffff81653532>] kmem_cache_alloc_trace+0x152/0x2c0
[<ffffffff83b7cc58>] p9_fid_create+0x58/0x3a0
[<ffffffff83b83a3d>] p9_client_walk+0xbd/0x7a0
[<ffffffff81b7df1c>] v9fs_file_open+0x38c/0x740
[<ffffffff816ab927>] do_dentry_open+0x5c7/0xc50
[<ffffffff816af4c5>] vfs_open+0x105/0x220
[<ffffffff816e07a0>] path_openat+0x8f0/0x2920
[<ffffffff816e539e>] do_filp_open+0x18e/0x250
[<ffffffff816c4403>] do_open_execat+0xe3/0x4c0
[<ffffffff816cad41>] do_execveat_common.isra.36+0x671/0x1d00
[<ffffffff816cce52>] SyS_execve+0x42/0x50
[<ffffffff8100524f>] do_syscall_64+0x1af/0x4d0
[<ffffffff83d85b74>] return_from_SYSCALL_64+0x0/0x6a
Freed:
PID = 1280
[<ffffffff811db686>] save_stack_trace+0x16/0x20
[<ffffffff81657566>] save_stack+0x46/0xd0
[<ffffffff81657c61>] kasan_slab_free+0x71/0xb0
[<ffffffff8165506c>] kfree+0xfc/0x230
[<ffffffff83b7cb42>] p9_fid_destroy+0x1c2/0x280
[<ffffffff83b838bd>] p9_client_clunk+0xdd/0x1a0
[<ffffffff81b80694>] v9fs_dir_release+0x44/0x60
[<ffffffff816b9d67>] __fput+0x287/0x710
[<ffffffff816ba239>] delayed_fput+0x49/0x70
[<ffffffff812c6600>] process_one_work+0x8b0/0x14c0
[<ffffffff812c72fb>] worker_thread+0xeb/0x1210
[<ffffffff812da5a4>] kthread+0x244/0x2d0
[<ffffffff83d85d25>] ret_from_fork+0x25/0x30
Memory state around the buggy address:
ffff880010284c00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
ffff880010284c80: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
>ffff880010284d00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
^
ffff880010284d80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff880010284e00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
Disabling lock debugging due to kernel taint
==================================================================
Manifestation 3:
Out of memory: Kill process 1650 (trinity-main) score 91 or sacrifice child
Killed process 1731 (trinity-main) total-vm:37140kB, anon-rss:192kB,
file-rss:0kB, shmem-rss:0kB
==================================================================
BUG: KASAN: use-after-free in unlink_file_vma+0xa5/0xb0 at addr
ffff880006689db0
Read of size 8 by task trinity-main/1731
CPU: 5 PID: 1731 Comm: trinity-main Not tainted 4.9.0 #314
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
Ubuntu-1.8.2-1ubuntu1 04/01/2014
ffff880000aaf7f8 ffffffff81fb1ab1 ffff8800110ed500 ffff880006689c00
ffff880006689db8 ffff880000aaf998 ffff880000aaf820 ffffffff8162c5ac
ffff880000aaf8b0 ffff880006689c00Out of memory: Kill process 1650
(trinity-main) score 91 or sacrifice child
Killed process 1650 (trinity-main) total-vm:37140kB, anon-rss:192kB,
file-rss:140kB, shmem-rss:18632kB
oom_reaper: reaped process 1650 (trinity-main), now anon-rss:0kB,
file-rss:0kB, shmem-rss:18632kB
ffff8800110ed500 ffff880000aaf8a0
Call Trace:
[<ffffffff81fb1ab1>] dump_stack+0x83/0xb2
[<ffffffff8162c5ac>] kasan_object_err+0x1c/0x70
[<ffffffff8162c845>] kasan_report_error+0x1f5/0x4e0
[<ffffffff815afd80>] ? vm_normal_page_pmd+0x240/0x240
[<ffffffff8162ccce>] __asan_report_load8_noabort+0x3e/0x40
[<ffffffff815c4305>] ? unlink_file_vma+0xa5/0xb0
[<ffffffff815c4305>] unlink_file_vma+0xa5/0xb0
[<ffffffff815ad610>] free_pgtables+0x80/0x350
[<ffffffff815ca662>] exit_mmap+0x212/0x3d0
[<ffffffff815ca450>] ? SyS_munmap+0xa0/0xa0
[<ffffffff8130b2a5>] ? __might_sleep+0x95/0x1a0
[<ffffffff812684a0>] mmput+0x90/0x1c0
[<ffffffff8127e3dd>] do_exit+0x71d/0x2930
[<ffffffff815adeb0>] ? vm_normal_page+0x200/0x200
[<ffffffff8127dcc0>] ? mm_update_next_owner+0x710/0x710
[<ffffffff815b614f>] ? handle_mm_fault+0xcbf/0x2a60
[<ffffffff81298403>] ? __dequeue_signal+0x133/0x470
[<ffffffff81280768>] do_group_exit+0x108/0x330
[<ffffffff812a1d83>] get_signal+0x613/0x1390
[<ffffffff8135e912>] ? __lock_acquire.isra.32+0xc2/0x1a30
[<ffffffff811b1b9f>] do_signal+0x7f/0x18f0
[<ffffffff81237354>] ? __do_page_fault+0x474/0x9f0
[<ffffffff811b1b20>] ? setup_sigcontext+0x7d0/0x7d0
[<ffffffff8123719c>] ? __do_page_fault+0x2bc/0x9f0
[<ffffffff82017d27>] ? check_preemption_disabled+0x37/0x1e0
[<ffffffff81004ee8>] ? prepare_exit_to_usermode+0xb8/0xd0
[<ffffffff81237b94>] ? trace_do_page_fault+0x1f4/0x3a0
[<ffffffff81004ee8>] ? prepare_exit_to_usermode+0xb8/0xd0
[<ffffffff81229fa7>] ? do_async_page_fault+0x27/0xa0
[<ffffffff83c993d8>] ? async_page_fault+0x28/0x30
[<ffffffff81004ee8>] ? prepare_exit_to_usermode+0xb8/0xd0
[<ffffffff81002975>] exit_to_usermode_loop+0xb5/0x150
[<ffffffff81004ee8>] ? prepare_exit_to_usermode+0xb8/0xd0
[<ffffffff8100506e>] syscall_return_slowpath+0x16e/0x1a0
[<ffffffff83c98495>] ret_from_fork+0x15/0x30
Object at ffff880006689c00, in cache filp size: 440
Allocated:
PID = 1650
[<ffffffff811d77d6>] save_stack_trace+0x16/0x20
[<ffffffff8162b3e6>] save_stack+0x46/0xd0
[<ffffffff8162bbcd>] kasan_kmalloc+0xad/0xe0
[<ffffffff8162bc12>] kasan_slab_alloc+0x12/0x20
[<ffffffff81627195>] kmem_cache_alloc+0xf5/0x2c0
[<ffffffff8168aaa1>] get_empty_filp+0x91/0x3e0
[<ffffffff816affd2>] path_openat+0xb2/0x2920
[<ffffffff816b540e>] do_filp_open+0x18e/0x250
[<ffffffff816946f3>] do_open_execat+0xe3/0x4c0
[<ffffffff8169adb1>] do_execveat_common.isra.36+0x671/0x1d00
[<ffffffff8169cec2>] SyS_execve+0x42/0x50
[<ffffffff8100524f>] do_syscall_64+0x1af/0x4d0
[<ffffffff83c982f4>] return_from_SYSCALL_64+0x0/0x6a
Freed:
PID = 2
[<ffffffff811d77d6>] save_stack_trace+0x16/0x20
[<ffffffff8162b3e6>] save_stack+0x46/0xd0
[<ffffffff8162bae1>] kasan_slab_free+0x71/0xb0
[<ffffffff81627ddf>] kmem_cache_free+0xaf/0x2a0
[<ffffffff8168a1b5>] file_free_rcu+0x65/0xa0
[<ffffffff8139bd87>] rcu_process_callbacks+0x9b7/0x10e0
[<ffffffff83c9ae01>] __do_softirq+0x1c1/0x5ba
Memory state around the buggy address:
ffff880006689c80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff880006689d00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff880006689d80: fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc fc
^
ffff880006689e00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
Manifestation 4:
Killed process 1650 (trinity-main) total-vm:37140kB, anon-rss:192kB,
file-rss:144kB, shmem-rss:18632kB
oom_reaper: reaped process 1650 (trinity-main), now anon-rss:0kB,
file-rss:0kB, shmem-rss:18632kB
==================================================================
BUG: KASAN: use-after-free in unlink_file_vma+0xa5/0xb0 at addr
ffff880006b523b0
Read of size 8 by task kworker/3:1/1344
CPU: 3 PID: 1344 Comm: kworker/3:1 Not tainted 4.9.0 #314
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
Ubuntu-1.8.2-1ubuntu1 04/01/2014
Workqueue: events mmput_async_fn
ffff88000d3bf978 ffffffff81fb1ab1 ffff8800110ed500 ffff880006b52200
ffff880006b523b8 ffff88000d3bfb18 ffff88000d3bf9a0/home/vegard/lin
ffffffff8162c5acux/init-trinity.
sh: line 127: 1 ffff88000d3bfa30650 Killed ffff880006b52200
/sbi ffff8800110ed500n/capsh --user=n ffff88000d3bfa20obody --caps= --
-c '/home/vegarCall Trace:
d/trinity/trinit [<ffffffff81fb1ab1>] dump_stack+0x83/0xb2
y -qq -C32 --ena [<ffffffff8162c5ac>] kasan_object_err+0x1c/0x70
ble-fds=pseudo - [<ffffffff8162c845>] kasan_report_error+0x1f5/0x4e0
cexecve,64 -V /p [<ffffffff815afd80>] ? vm_normal_page_pmd+0x240/0x240
roc/self/mem'
[<ffffffff8162ccce>] __asan_report_load8_noabort+0x3e/0x40
[<ffffffff815c4305>] ? unlink_file_vma+0xa5/0xb0
[<ffffffff815c4305>] unlink_file_vma+0xa5/0xb0
[<ffffffff815ad610>] free_pgtables+0x80/0x350
[<ffffffff815ca662>] exit_mmap+0x212/0x3d0
[<ffffffff815ca450>] ? SyS_munmap+0xa0/0xa0
[<ffffffff812bf478>] ? process_one_work+0x6e8/0x13a0
+ true [<ffffffff812682d1>] mmput_async_fn+0x61/0x1a0
[<ffffffff812bf54f>] process_one_work+0x7bf/0x13a0
[<ffffffff812bf478>] ? process_one_work+0x6e8/0x13a0
[<ffffffff812ebf84>] ? finish_task_switch+0x184/0x660
[<ffffffff812bed90>] ? __cancel_work+0x220/0x220
[<ffffffff812c021b>] worker_thread+0xeb/0x1150
[<ffffffff83c88471>] ? __schedule+0x461/0x17c0
[<ffffffff812d29f4>] kthread+0x244/0x2d0
[<ffffffff812c0130>] ? process_one_work+0x13a0/0x13a0
+ true
[<ffffffff812d27b0>] ? __kthread_create_on_node+0x380/0x380
[<ffffffff812d27b0>] ? __kthread_create_on_node+0x380/0x380
[<ffffffff812d27b0>] ? __kthread_create_on_node+0x380/0x380
[<ffffffff83c984a5>] ret_from_fork+0x25/0x30
Object at ffff880006b52200, in cache filp size: 440
Allocated:
PID = 1650
[<ffffffff811d77d6>] save_stack_trace+0x16/0x20
[<ffffffff8162b3e6>] save_stack+0x46/0xd0
[<ffffffff8162bbcd>] kasan_kmalloc+0xad/0xe0
[<ffffffff8162bc12>] kasan_slab_alloc+0x12/0x20
[<ffffffff81627195>] kmem_cache_alloc+0xf5/0x2c0
[<ffffffff8168aaa1>] get_empty_filp+0x91/0x3e0
[<ffffffff816affd2>] path_openat+0xb2/0x2920
[<ffffffff816b540e>] do_filp_open+0x18e/0x250
[<ffffffff816946f3>] do_open_execat+0xe3/0x4c0
[<ffffffff8169adb1>] do_execveat_common.isra.36+0x671/0x1d00
[<ffffffff8169cec2>] SyS_execve+0x42/0x50
[<ffffffff8100524f>] do_syscall_64+0x1af/0x4d0
[<ffffffff83c982f4>] return_from_SYSCALL_64+0x0/0x6a
Freed:
PID = 0
[<ffffffff811d77d6>] save_stack_trace+0x16/0x20
[<ffffffff8162b3e6>] save_stack+0x46/0xd0
[<ffffffff8162bae1>] kasan_slab_free+0x71/0xb0
[<ffffffff81627ddf>] kmem_cache_free+0xaf/0x2a0
[<ffffffff8168a1b5>] file_free_rcu+0x65/0xa0
[<ffffffff8139bd87>] rcu_process_callbacks+0x9b7/0x10e0
[<ffffffff83c9ae01>] __do_softirq+0x1c1/0x5ba
Memory state around the buggy address:
ffff880006b52280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff880006b52300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff880006b52380: fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc fc
^
ffff880006b52400: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff880006b52480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 15:10 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP1mV-4JQ-3@gated-at.bofh.it> |
| In reply to | #1543457 |
On Fri 16-12-16 14:14:17, Vegard Nossum wrote: [...] > Out of memory: Kill process 1650 (trinity-main) score 90 or sacrifice child > Killed process 1724 (trinity-c14) total-vm:37280kB, anon-rss:236kB, > file-rss:112kB, shmem-rss:112kB > BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8 > IP: [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580 > PGD c001067 PUD c000067 > PMD 0 > Oops: 0002 [#1] PREEMPT SMP KASAN > Dumping ftrace buffer: > (ftrace buffer empty) > CPU: 28 PID: 1650 Comm: trinity-main Not tainted 4.9.0-rc6+ #317 Hmm, so this was the oom victim initially but we have decided to kill its child 1724 instead. > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > Ubuntu-1.8.2-1ubuntu1 04/01/2014 > task: ffff88000f9bc440 task.stack: ffff88000c778000 > RIP: 0010:[<ffffffff8126b1c0>] [<ffffffff8126b1c0>] > copy_process.part.41+0x2150/0x5580 Could you match this to the kernel source please? > RSP: 0018:ffff88000c77fc18 EFLAGS: 00010297 > RAX: 0000000000000000 RBX: ffff88000fa11c00 RCX: 0000000000000000 > RDX: 0000000000000000 RSI: dffffc0000000000 RDI: ffff88000f2a33b0 > RBP: ffff88000c77fdb0 R08: ffff88000c77f900 R09: 0000000000000002 > R10: 00000000cb9401ca R11: 00000000c6eda739 R12: ffff88000f894d00 > R13: ffff88000c7c4700 R14: ffff88000fa11c50 R15: ffff88000f2a3200 > FS: 00007fb7d2a24700(0000) GS:ffff880011b00000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00000000000001e8 CR3: 000000001010d000 CR4: 00000000000406e0 > Stack: > 0000000000000046 0000000001200011 ffffed0001f129ac ffff88000f894d60 > 0000000000000000 0000000000000000 ffff88000f894d08 ffff88000f894da0 > ffff88000c7a8620 ffff88000f020318 ffff88000fa11c18 ffff88000f894e40 > Call Trace: > [<ffffffff81269070>] ? __cleanup_sighand+0x50/0x50 > [<ffffffff81fd552e>] ? memzero_explicit+0xe/0x10 > [<ffffffff822cb592>] ? urandom_read+0x232/0x4d0 > [<ffffffff8126e974>] _do_fork+0x1a4/0xa40 and here we are copying the pid 1650 to its child. I am wondering whether that might be the killed child. But the child is visible only very late during the fork to the oom killer. > [<ffffffff8126e7d0>] ? fork_idle+0x180/0x180 > [<ffffffff81002dba>] ? syscall_trace_enter+0x3aa/0xd40 > [<ffffffff815179ea>] ? __context_tracking_exit.part.4+0x9a/0x1e0 > [<ffffffff81002a10>] ? exit_to_usermode_loop+0x150/0x150 > [<ffffffff8201df57>] ? check_preemption_disabled+0x37/0x1e0 > [<ffffffff8126f2e7>] SyS_clone+0x37/0x50 > [<ffffffff83caea50>] ? ptregs_sys_rt_sigreturn+0x10/0x10 > [<ffffffff8100524f>] do_syscall_64+0x1af/0x4d0 > [<ffffffff83cae974>] entry_SYSCALL64_slow_path+0x25/0x25 > Code: be 00 00 00 00 00 fc ff df 48 c1 e8 03 80 3c 30 00 74 08 4c 89 f7 e8 > d0 7d 3c 00 f6 43 51 08 74 11 e8 45 fa 1d 00 48 8b 44 24 20 <f0> ff 88 e8 01 > 00 00 e8 34 fa 1d 00 48 8b 44 24 70 48 83 c0 60 > RIP [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580 > RSP <ffff88000c77fc18> > CR2: 00000000000001e8 > ---[ end trace b8f81ad60c106e75 ]--- > > Manifestation 2: > > Killed process 1775 (trinity-c21) total-vm:37404kB, anon-rss:232kB, > file-rss:420kB, shmem-rss:116kB > oom_reaper: reaped process 1775 (trinity-c21), now anon-rss:0kB, > file-rss:0kB, shmem-rss:116kB > ================================================================== > BUG: KASAN: use-after-free in p9_client_read+0x8f0/0x960 at addr > ffff880010284d00 > Read of size 8 by task trinity-main/1649 > CPU: 3 PID: 1649 Comm: trinity-main Not tainted 4.9.0+ #318 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > Ubuntu-1.8.2-1ubuntu1 04/01/2014 > ffff8800068a7770 ffffffff82012301 ffff88001100f600 ffff880010284d00 > ffff880010284d60 ffff880010284d00 ffff8800068a7798 ffffffff8165872c > ffff8800068a7828 ffff880010284d00 ffff88001100f600 ffff8800068a7818 > Call Trace: > [<ffffffff82012301>] dump_stack+0x83/0xb2 > [<ffffffff8165872c>] kasan_object_err+0x1c/0x70 > [<ffffffff816589c5>] kasan_report_error+0x1f5/0x4e0 > [<ffffffff81657d92>] ? kasan_slab_alloc+0x12/0x20 > [<ffffffff82079357>] ? check_preemption_disabled+0x37/0x1e0 > [<ffffffff81658e4e>] __asan_report_load8_noabort+0x3e/0x40 > [<ffffffff82079300>] ? assoc_array_gc+0x1310/0x1330 > [<ffffffff83b84c30>] ? p9_client_read+0x8f0/0x960 > [<ffffffff83b84c30>] p9_client_read+0x8f0/0x960 no idea how we would end up with use after here. Even if I unmapped the page then the read code should be able to cope with that. This smells like a p9 issue to me. [...] > Manifestation 3: > > Out of memory: Kill process 1650 (trinity-main) score 91 or sacrifice child > Killed process 1731 (trinity-main) total-vm:37140kB, anon-rss:192kB, > file-rss:0kB, shmem-rss:0kB > ================================================================== > BUG: KASAN: use-after-free in unlink_file_vma+0xa5/0xb0 at addr > ffff880006689db0 > Read of size 8 by task trinity-main/1731 > CPU: 5 PID: 1731 Comm: trinity-main Not tainted 4.9.0 #314 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > Ubuntu-1.8.2-1ubuntu1 04/01/2014 > ffff880000aaf7f8 ffffffff81fb1ab1 ffff8800110ed500 ffff880006689c00 > ffff880006689db8 ffff880000aaf998 ffff880000aaf820 ffffffff8162c5ac > ffff880000aaf8b0 ffff880006689c00Out of memory: Kill process 1650 > (trinity-main) score 91 or sacrifice child > Killed process 1650 (trinity-main) total-vm:37140kB, anon-rss:192kB, > file-rss:140kB, shmem-rss:18632kB > oom_reaper: reaped process 1650 (trinity-main), now anon-rss:0kB, > file-rss:0kB, shmem-rss:18632kB > ffff8800110ed500 ffff880000aaf8a0 > Call Trace: > [<ffffffff81fb1ab1>] dump_stack+0x83/0xb2 > [<ffffffff8162c5ac>] kasan_object_err+0x1c/0x70 > [<ffffffff8162c845>] kasan_report_error+0x1f5/0x4e0 > [<ffffffff815afd80>] ? vm_normal_page_pmd+0x240/0x240 > [<ffffffff8162ccce>] __asan_report_load8_noabort+0x3e/0x40 > [<ffffffff815c4305>] ? unlink_file_vma+0xa5/0xb0 > [<ffffffff815c4305>] unlink_file_vma+0xa5/0xb0 Hmm, the oom repaper doesn't touch vma->vm_file so I do not see how it could be related to the activity of the reaper. I will have a look closer. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| Date | 2016-12-16 15:30 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP1Gh-4Ql-1@gated-at.bofh.it> |
| In reply to | #1543473 |
On 12/16/2016 03:00 PM, Michal Hocko wrote: > On Fri 16-12-16 14:14:17, Vegard Nossum wrote: > [...] >> Out of memory: Kill process 1650 (trinity-main) score 90 or sacrifice child >> Killed process 1724 (trinity-c14) total-vm:37280kB, anon-rss:236kB, >> file-rss:112kB, shmem-rss:112kB >> BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8 >> IP: [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580 >> PGD c001067 PUD c000067 >> PMD 0 >> Oops: 0002 [#1] PREEMPT SMP KASAN >> Dumping ftrace buffer: >> (ftrace buffer empty) >> CPU: 28 PID: 1650 Comm: trinity-main Not tainted 4.9.0-rc6+ #317 > > Hmm, so this was the oom victim initially but we have decided to kill > its child 1724 instead. > >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >> Ubuntu-1.8.2-1ubuntu1 04/01/2014 >> task: ffff88000f9bc440 task.stack: ffff88000c778000 >> RIP: 0010:[<ffffffff8126b1c0>] [<ffffffff8126b1c0>] >> copy_process.part.41+0x2150/0x5580 > > Could you match this to the kernel source please? kernel/fork.c:629 dup_mmap() it's atomic_dec(&inode->i_writecount), it matches up with file_inode(file) == NULL: (gdb) p &((struct inode *)0)->i_writecount $1 = (atomic_t *) 0x1e8 <irq_stack_union+488> >> Killed process 1775 (trinity-c21) total-vm:37404kB, anon-rss:232kB, >> file-rss:420kB, shmem-rss:116kB >> oom_reaper: reaped process 1775 (trinity-c21), now anon-rss:0kB, >> file-rss:0kB, shmem-rss:116kB >> ================================================================== >> BUG: KASAN: use-after-free in p9_client_read+0x8f0/0x960 at addr >> ffff880010284d00 >> Read of size 8 by task trinity-main/1649 >> CPU: 3 PID: 1649 Comm: trinity-main Not tainted 4.9.0+ #318 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >> Ubuntu-1.8.2-1ubuntu1 04/01/2014 >> ffff8800068a7770 ffffffff82012301 ffff88001100f600 ffff880010284d00 >> ffff880010284d60 ffff880010284d00 ffff8800068a7798 ffffffff8165872c >> ffff8800068a7828 ffff880010284d00 ffff88001100f600 ffff8800068a7818 >> Call Trace: >> [<ffffffff82012301>] dump_stack+0x83/0xb2 >> [<ffffffff8165872c>] kasan_object_err+0x1c/0x70 >> [<ffffffff816589c5>] kasan_report_error+0x1f5/0x4e0 >> [<ffffffff81657d92>] ? kasan_slab_alloc+0x12/0x20 >> [<ffffffff82079357>] ? check_preemption_disabled+0x37/0x1e0 >> [<ffffffff81658e4e>] __asan_report_load8_noabort+0x3e/0x40 >> [<ffffffff82079300>] ? assoc_array_gc+0x1310/0x1330 >> [<ffffffff83b84c30>] ? p9_client_read+0x8f0/0x960 >> [<ffffffff83b84c30>] p9_client_read+0x8f0/0x960 > > no idea how we would end up with use after here. Even if I unmapped the > page then the read code should be able to cope with that. This smells > like a p9 issue to me. This is fid->clnt dereference at the top of p9_client_read(). Ah, yes, this is the one coming from a page fault: p9_client_read v9fs_fid_readpage v9fs_vfs_readpage handle_mm_fault __do_page_fault the bad fid pointer is filp->private_data. Hm, so I guess the file itself was NOT freed prematurely (as otherwise we'd probably have seen a KASAN report for the filp->private_data dereference), but the ->private_data itself was. Maybe the whole thing is fundamentally a 9p bug and the OOM killer just happens to trigger it. Vegard
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 15:50 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP1ZD-4WE-1@gated-at.bofh.it> |
| In reply to | #1543489 |
On Fri 16-12-16 15:25:27, Vegard Nossum wrote: > On 12/16/2016 03:00 PM, Michal Hocko wrote: > > On Fri 16-12-16 14:14:17, Vegard Nossum wrote: > > [...] > > > Out of memory: Kill process 1650 (trinity-main) score 90 or sacrifice child > > > Killed process 1724 (trinity-c14) total-vm:37280kB, anon-rss:236kB, > > > file-rss:112kB, shmem-rss:112kB > > > BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8 > > > IP: [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580 > > > PGD c001067 PUD c000067 > > > PMD 0 > > > Oops: 0002 [#1] PREEMPT SMP KASAN > > > Dumping ftrace buffer: > > > (ftrace buffer empty) > > > CPU: 28 PID: 1650 Comm: trinity-main Not tainted 4.9.0-rc6+ #317 > > > > Hmm, so this was the oom victim initially but we have decided to kill > > its child 1724 instead. > > > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > > > Ubuntu-1.8.2-1ubuntu1 04/01/2014 > > > task: ffff88000f9bc440 task.stack: ffff88000c778000 > > > RIP: 0010:[<ffffffff8126b1c0>] [<ffffffff8126b1c0>] > > > copy_process.part.41+0x2150/0x5580 > > > > Could you match this to the kernel source please? > > kernel/fork.c:629 dup_mmap() Ok, so this is before the child is made visible so the oom reaper couldn't have seen it. > it's atomic_dec(&inode->i_writecount), it matches up with > file_inode(file) == NULL: > > (gdb) p &((struct inode *)0)->i_writecount > $1 = (atomic_t *) 0x1e8 <irq_stack_union+488> is this a p9 inode? > > > > Killed process 1775 (trinity-c21) total-vm:37404kB, anon-rss:232kB, > > > file-rss:420kB, shmem-rss:116kB > > > oom_reaper: reaped process 1775 (trinity-c21), now anon-rss:0kB, > > > file-rss:0kB, shmem-rss:116kB > > > ================================================================== > > > BUG: KASAN: use-after-free in p9_client_read+0x8f0/0x960 at addr > > > ffff880010284d00 > > > Read of size 8 by task trinity-main/1649 > > > CPU: 3 PID: 1649 Comm: trinity-main Not tainted 4.9.0+ #318 > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > > > Ubuntu-1.8.2-1ubuntu1 04/01/2014 > > > ffff8800068a7770 ffffffff82012301 ffff88001100f600 ffff880010284d00 > > > ffff880010284d60 ffff880010284d00 ffff8800068a7798 ffffffff8165872c > > > ffff8800068a7828 ffff880010284d00 ffff88001100f600 ffff8800068a7818 > > > Call Trace: > > > [<ffffffff82012301>] dump_stack+0x83/0xb2 > > > [<ffffffff8165872c>] kasan_object_err+0x1c/0x70 > > > [<ffffffff816589c5>] kasan_report_error+0x1f5/0x4e0 > > > [<ffffffff81657d92>] ? kasan_slab_alloc+0x12/0x20 > > > [<ffffffff82079357>] ? check_preemption_disabled+0x37/0x1e0 > > > [<ffffffff81658e4e>] __asan_report_load8_noabort+0x3e/0x40 > > > [<ffffffff82079300>] ? assoc_array_gc+0x1310/0x1330 > > > [<ffffffff83b84c30>] ? p9_client_read+0x8f0/0x960 > > > [<ffffffff83b84c30>] p9_client_read+0x8f0/0x960 > > > > no idea how we would end up with use after here. Even if I unmapped the > > page then the read code should be able to cope with that. This smells > > like a p9 issue to me. > > This is fid->clnt dereference at the top of p9_client_read(). > > Ah, yes, this is the one coming from a page fault: > > p9_client_read > v9fs_fid_readpage > v9fs_vfs_readpage > handle_mm_fault > __do_page_fault > > the bad fid pointer is filp->private_data. > > Hm, so I guess the file itself was NOT freed prematurely (as otherwise > we'd probably have seen a KASAN report for the filp->private_data > dereference), but the ->private_data itself was. > > Maybe the whole thing is fundamentally a 9p bug and the OOM killer just > happens to trigger it. It smells like that. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| Date | 2016-12-16 16:00 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP29j-503-21@gated-at.bofh.it> |
| In reply to | #1543498 |
On 12/16/2016 03:32 PM, Michal Hocko wrote: > On Fri 16-12-16 15:25:27, Vegard Nossum wrote: >> On 12/16/2016 03:00 PM, Michal Hocko wrote: >>> On Fri 16-12-16 14:14:17, Vegard Nossum wrote: >>> [...] >>>> Out of memory: Kill process 1650 (trinity-main) score 90 or sacrifice child >>>> Killed process 1724 (trinity-c14) total-vm:37280kB, anon-rss:236kB, >>>> file-rss:112kB, shmem-rss:112kB >>>> BUG: unable to handle kernel NULL pointer dereference at 00000000000001e8 >>>> IP: [<ffffffff8126b1c0>] copy_process.part.41+0x2150/0x5580 >>>> PGD c001067 PUD c000067 >>>> PMD 0 >>>> Oops: 0002 [#1] PREEMPT SMP KASAN >>>> Dumping ftrace buffer: >>>> (ftrace buffer empty) >>>> CPU: 28 PID: 1650 Comm: trinity-main Not tainted 4.9.0-rc6+ #317 >>> >>> Hmm, so this was the oom victim initially but we have decided to kill >>> its child 1724 instead. >>> >>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS >>>> Ubuntu-1.8.2-1ubuntu1 04/01/2014 >>>> task: ffff88000f9bc440 task.stack: ffff88000c778000 >>>> RIP: 0010:[<ffffffff8126b1c0>] [<ffffffff8126b1c0>] >>>> copy_process.part.41+0x2150/0x5580 >>> >>> Could you match this to the kernel source please? >> >> kernel/fork.c:629 dup_mmap() > > Ok, so this is before the child is made visible so the oom reaper > couldn't have seen it. > >> it's atomic_dec(&inode->i_writecount), it matches up with >> file_inode(file) == NULL: >> >> (gdb) p &((struct inode *)0)->i_writecount >> $1 = (atomic_t *) 0x1e8 <irq_stack_union+488> > > is this a p9 inode? When I looked at this before it always crashed in this spot for the very first VMA in the mm (which happens to be the exe, which is on a 9p root fs). I added a trace_printk() to dup_mmap() to print inode->i_sb->s_type and the last thing I see for a new crash in the same place is: trinity--9280 28.... 136345090us : copy_process.part.41: ffffffff8485ec40 --------------------------------- CPU: 0 PID: 9302 Comm: trinity-c0 Not tainted 4.9.0-rc8+ #332 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 task: ffff880000070000 task.stack: ffff8800099e0000 RIP: 0010:[<ffffffff8126c7c9>] [<ffffffff8126c7c9>] copy_process.part.41+0x22c9/0x55b0 As you can see, the addresses match: (gdb) p &v9fs_fs_type $1 = (struct file_system_type *) 0xffffffff8485ec40 <v9fs_fs_type> So I think we can safely say that yes, it's a p9 inode. Vegard
[toc] | [prev] | [next] | [standalone]
| From | Vegard Nossum <vegard.nossum@oracle.com> |
|---|---|
| Date | 2016-12-16 15:10 +0100 |
| Subject | Re: crash during oom reaper |
| Message-ID | <sP1mW-4JQ-13@gated-at.bofh.it> |
| In reply to | #1543457 |
On 12/16/2016 02:14 PM, Vegard Nossum wrote:
> On 12/16/2016 11:11 AM, Michal Hocko wrote:
>> On Fri 16-12-16 10:43:52, Vegard Nossum wrote:
>> [...]
>>> I don't think it's a bug in the OOM reaper itself, but either of the
>>> following two patches will fix the problem (without my understand how or
>>> why):
>>>
>> What is the atual crash?
>
> Annoyingly it doesn't seem to reproduce with the very latest
> linus/master, so maybe it's been fixed recently after all and I missed it.
>
> I've started a bisect to see what fixed it. Just in case, I added 4
> different crashes I saw with various kernels. I think there may have
> been a few others too (I remember seeing one in a page fault path), but
> these were the most frequent ones.
The bisect points to:
commit 6b94780e45c17b83e3e75f8aaca5a328db583c74
Author: Vincent Guittot <vincent.guittot@linaro.org>
Date: Thu Dec 8 17:56:54 2016 +0100
sched/core: Use load_avg for selecting idlest group
as fixing the crash, which seems odd to me. The only bit that sticks out
from the changelog to me:
"""
For use case like hackbench, this enable the scheduler to select
different CPUs during the fork sequence and to spread tasks across the
system.
"""
Reverting it from linus/master doesn't reintroduce the crash, but the
commit just before (6b94780e4^) does crash, so I'm not sure what's going
on. Maybe the crash is just really sensitive to scheduling decisions or
something.
Vegard
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-12-16 10:30 +0100 |
| Subject | Re: [PATCH 3/4] mm: use mmget_not_zero() helper |
| Message-ID | <sOWZY-1DO-35@gated-at.bofh.it> |
| In reply to | #1543269 |
On Fri 16-12-16 09:22:01, Vegard Nossum wrote:
> We already have the helper, we can convert the rest of the kernel
> mechanically using:
>
> git grep -l 'atomic_inc_not_zero.*mm_users' | xargs sed -i 's/atomic_inc_not_zero(&\(.*\)->mm_users)/mmget_not_zero\(\1\)/'
>
> This is needed for a later patch that hooks into the helper, but might be
> a worthwhile cleanup on its own.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> drivers/gpu/drm/i915/i915_gem_userptr.c | 2 +-
> drivers/iommu/intel-svm.c | 2 +-
> fs/proc/base.c | 4 ++--
> fs/proc/task_mmu.c | 4 ++--
> fs/proc/task_nommu.c | 2 +-
> kernel/events/uprobes.c | 2 +-
> mm/swapfile.c | 2 +-
> 7 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index f21ca404af79..e97f9ade99fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -514,7 +514,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
> flags |= FOLL_WRITE;
>
> ret = -EFAULT;
> - if (atomic_inc_not_zero(&mm->mm_users)) {
> + if (mmget_not_zero(mm)) {
> down_read(&mm->mmap_sem);
> while (pinned < npages) {
> ret = get_user_pages_remote
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index cb72e0011310..51f2b228723f 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -579,7 +579,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> if (!svm->mm)
> goto bad_req;
> /* If the mm is already defunct, don't handle faults. */
> - if (!atomic_inc_not_zero(&svm->mm->mm_users))
> + if (!mmget_not_zero(svm->mm))
> goto bad_req;
> down_read(&svm->mm->mmap_sem);
> vma = find_extend_vma(svm->mm, address);
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 0b8ccacae8b3..87fd5bf07578 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -842,7 +842,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf,
> return -ENOMEM;
>
> copied = 0;
> - if (!atomic_inc_not_zero(&mm->mm_users))
> + if (!mmget_not_zero(mm))
> goto free;
>
> /* Maybe we should limit FOLL_FORCE to actual ptrace users? */
> @@ -950,7 +950,7 @@ static ssize_t environ_read(struct file *file, char __user *buf,
> return -ENOMEM;
>
> ret = 0;
> - if (!atomic_inc_not_zero(&mm->mm_users))
> + if (!mmget_not_zero(mm))
> goto free;
>
> down_read(&mm->mmap_sem);
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 35b92d81692f..c71975293dc8 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -167,7 +167,7 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> return ERR_PTR(-ESRCH);
>
> mm = priv->mm;
> - if (!mm || !atomic_inc_not_zero(&mm->mm_users))
> + if (!mm || !mmget_not_zero(mm))
> return NULL;
>
> down_read(&mm->mmap_sem);
> @@ -1352,7 +1352,7 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
> unsigned long end_vaddr;
> int ret = 0, copied = 0;
>
> - if (!mm || !atomic_inc_not_zero(&mm->mm_users))
> + if (!mm || !mmget_not_zero(mm))
> goto out;
>
> ret = -EINVAL;
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 37175621e890..1ef97cfcf422 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -219,7 +219,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
> return ERR_PTR(-ESRCH);
>
> mm = priv->mm;
> - if (!mm || !atomic_inc_not_zero(&mm->mm_users))
> + if (!mm || !mmget_not_zero(mm))
> return NULL;
>
> down_read(&mm->mmap_sem);
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index f9ec9add2164..bcf0f9d77d4d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -741,7 +741,7 @@ build_map_info(struct address_space *mapping, loff_t offset, bool is_register)
> continue;
> }
>
> - if (!atomic_inc_not_zero(&vma->vm_mm->mm_users))
> + if (!mmget_not_zero(vma->vm_mm))
> continue;
>
> info = prev;
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index cf73169ce153..8c92829326cb 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1494,7 +1494,7 @@ int try_to_unuse(unsigned int type, bool frontswap,
> while (swap_count(*swap_map) && !retval &&
> (p = p->next) != &start_mm->mmlist) {
> mm = list_entry(p, struct mm_struct, mmlist);
> - if (!atomic_inc_not_zero(&mm->mm_users))
> + if (!mmget_not_zero(mm))
> continue;
> spin_unlock(&mmlist_lock);
> mmput(prev_mm);
> --
> 2.11.0.1.gaa10c3f
>
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web