[PATCH 0/5] mm: poison critical mm/ structs

[PATCH 0/5] mm: poison critical mm/ structs

[Sasha Levin]

Currently we're seeing a few issues which are unexplainable by looking at the
data we see and are most likely caused by a memory corruption caused
elsewhere.

This is wasting time for folks who are trying to figure out an issue provided
a stack trace that can't really point out the real issue.

This patch introduces poisoning on struct page, vm_area_struct, and mm_struct,
and places checks in busy paths to catch corruption early.

This series was tested, and it detects corruption in vm_area_struct. Right now
I'm working on figuring out the source of the corruption, (which is a long
standing bug) using KASan, but the current code is useful as it is.

Sasha Levin (5):
  mm: add poisoning basics
  mm: constify dump_page and friends
  mm: poison mm_struct
  mm: poison vm_area_struct
  mm: poison page struct

 fs/exec.c                   |  5 +++++
 include/linux/memcontrol.h  |  8 ++++----
 include/linux/mm.h          | 11 ++++++++++-
 include/linux/mm_types.h    | 18 ++++++++++++++++++
 include/linux/mmdebug.h     | 24 ++++++++++++++++++++++--
 include/linux/page-flags.h  | 24 ++++++++++++++++--------
 include/linux/page_cgroup.h |  4 ++--
 include/linux/poison.h      |  6 ++++++
 kernel/fork.c               | 13 +++++++++++++
 lib/Kconfig.debug           |  9 +++++++++
 mm/debug.c                  | 22 ++++++++++++++++++----
 mm/memcontrol.c             |  6 +++---
 mm/mmap.c                   | 21 ++++++++++++++++++++-
 mm/nommu.c                  |  7 +++++++
 mm/page_cgroup.c            |  4 ++--
 mm/vmacache.c               |  5 +++++
 16 files changed, 160 insertions(+), 27 deletions(-)

-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/5] mm: add poisoning basics

[Sasha Levin]

Add poisining basics along with a config option to enable poisoning.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 include/linux/poison.h | 6 ++++++
 lib/Kconfig.debug      | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/poison.h b/include/linux/poison.h
index 2110a81..db4d03e 100644
--- a/include/linux/poison.h
+++ b/include/linux/poison.h
@@ -86,4 +86,10 @@
 /********** sound/oss/ **********/
 #define OSS_POISON_FREE		0xAB
 
+/********** include/linux/mm_types.h **********/
+#ifdef CONFIG_DEBUG_VM_POISON
+#define MM_POISON_BEGIN		0x89ABCDEF
+#define MM_POISON_END		0xFEDCBA98
+#endif /* DEBUG_VM_POISON */
+
 #endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c366c8a..3b82772 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -543,6 +543,15 @@ config DEBUG_VM_RB
 
 	  If unsure, say N.
 
+config DEBUG_VM_POISON
+	bool "Poison VM structures"
+	depends on DEBUG_VM
+	help
+	  Add poison to the beggining and end of various VM structure to
+	  detect memory corruption in VM management code.
+
+	  If unsure, say N.
+
 config DEBUG_VIRTUAL
 	bool "Debug VM translations"
 	depends on DEBUG_KERNEL && X86
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/5] mm: constify dump_page and friends

[Sasha Levin]

Constify dump_page so that we could dump_page const pages, there is no
functional change here.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 include/linux/memcontrol.h  | 8 ++++----
 include/linux/mm.h          | 2 +-
 include/linux/mmdebug.h     | 4 ++--
 include/linux/page_cgroup.h | 4 ++--
 mm/debug.c                  | 4 ++--
 mm/memcontrol.c             | 6 +++---
 mm/page_cgroup.c            | 4 ++--
 7 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 19df5d8..534633f 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -200,8 +200,8 @@ void mem_cgroup_split_huge_fixup(struct page *head);
 #endif
 
 #ifdef CONFIG_DEBUG_VM
-bool mem_cgroup_bad_page_check(struct page *page);
-void mem_cgroup_print_bad_page(struct page *page);
+bool mem_cgroup_bad_page_check(const struct page *page);
+void mem_cgroup_print_bad_page(const struct page *page);
 #endif
 #else /* CONFIG_MEMCG */
 struct mem_cgroup;
@@ -373,13 +373,13 @@ void mem_cgroup_count_vm_event(struct mm_struct *mm, enum vm_event_item idx)
 
 #if !defined(CONFIG_MEMCG) || !defined(CONFIG_DEBUG_VM)
 static inline bool
-mem_cgroup_bad_page_check(struct page *page)
+mem_cgroup_bad_page_check(const struct page *page)
 {
 	return false;
 }
 
 static inline void
-mem_cgroup_print_bad_page(struct page *page)
+mem_cgroup_print_bad_page(const struct page *page)
 {
 }
 #endif
diff --git a/include/linux/mm.h b/include/linux/mm.h
index a91874b..0c13412 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -447,7 +447,7 @@ static inline void page_mapcount_reset(struct page *page)
 	atomic_set(&(page)->_mapcount, -1);
 }
 
-static inline int page_mapcount(struct page *page)
+static inline int page_mapcount(const struct page *page)
 {
 	return atomic_read(&(page)->_mapcount) + 1;
 }
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 877ef22..7d05557 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -7,8 +7,8 @@ struct page;
 struct vm_area_struct;
 struct mm_struct;
 
-extern void dump_page(struct page *page, const char *reason);
-extern void dump_page_badflags(struct page *page, const char *reason,
+extern void dump_page(const struct page *page, const char *reason);
+extern void dump_page_badflags(const struct page *page, const char *reason,
 			       unsigned long badflags);
 void dump_vma(const struct vm_area_struct *vma);
 void dump_mm(const struct mm_struct *mm);
diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index 5c831f1..fa30115 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -39,7 +39,7 @@ static inline void page_cgroup_init(void)
 }
 #endif
 
-struct page_cgroup *lookup_page_cgroup(struct page *page);
+struct page_cgroup *lookup_page_cgroup(const struct page *page);
 
 static inline int PageCgroupUsed(struct page_cgroup *pc)
 {
@@ -52,7 +52,7 @@ static inline void pgdat_page_cgroup_init(struct pglist_data *pgdat)
 {
 }
 
-static inline struct page_cgroup *lookup_page_cgroup(struct page *page)
+static inline struct page_cgroup *lookup_page_cgroup(const struct page *page)
 {
 	return NULL;
 }
diff --git a/mm/debug.c b/mm/debug.c
index 5ce45c9..d699471 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -80,7 +80,7 @@ static void dump_flags(unsigned long flags,
 	pr_cont(")\n");
 }
 
-void dump_page_badflags(struct page *page, const char *reason,
+void dump_page_badflags(const struct page *page, const char *reason,
 		unsigned long badflags)
 {
 	pr_emerg("page:%p count:%d mapcount:%d mapping:%p index:%#lx\n",
@@ -98,7 +98,7 @@ void dump_page_badflags(struct page *page, const char *reason,
 	mem_cgroup_print_bad_page(page);
 }
 
-void dump_page(struct page *page, const char *reason)
+void dump_page(const struct page *page, const char *reason)
 {
 	dump_page_badflags(page, reason, 0);
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 23976fd..b698778 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3546,7 +3546,7 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry,
 #endif
 
 #ifdef CONFIG_DEBUG_VM
-static struct page_cgroup *lookup_page_cgroup_used(struct page *page)
+static struct page_cgroup *lookup_page_cgroup_used(const struct page *page)
 {
 	struct page_cgroup *pc;
 
@@ -3561,7 +3561,7 @@ static struct page_cgroup *lookup_page_cgroup_used(struct page *page)
 	return NULL;
 }
 
-bool mem_cgroup_bad_page_check(struct page *page)
+bool mem_cgroup_bad_page_check(const struct page *page)
 {
 	if (mem_cgroup_disabled())
 		return false;
@@ -3569,7 +3569,7 @@ bool mem_cgroup_bad_page_check(struct page *page)
 	return lookup_page_cgroup_used(page) != NULL;
 }
 
-void mem_cgroup_print_bad_page(struct page *page)
+void mem_cgroup_print_bad_page(const struct page *page)
 {
 	struct page_cgroup *pc;
 
diff --git a/mm/page_cgroup.c b/mm/page_cgroup.c
index 3708264..0f14421 100644
--- a/mm/page_cgroup.c
+++ b/mm/page_cgroup.c
@@ -21,7 +21,7 @@ void __meminit pgdat_page_cgroup_init(struct pglist_data *pgdat)
 	pgdat->node_page_cgroup = NULL;
 }
 
-struct page_cgroup *lookup_page_cgroup(struct page *page)
+struct page_cgroup *lookup_page_cgroup(const struct page *page)
 {
 	unsigned long pfn = page_to_pfn(page);
 	unsigned long offset;
@@ -89,7 +89,7 @@ fail:
 
 #else /* CONFIG_FLAT_NODE_MEM_MAP */
 
-struct page_cgroup *lookup_page_cgroup(struct page *page)
+struct page_cgroup *lookup_page_cgroup(const struct page *page)
 {
 	unsigned long pfn = page_to_pfn(page);
 	struct mem_section *section = __pfn_to_section(pfn);
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/5] mm: poison mm_struct

[Sasha Levin]

Add poisoning to mm_struct to catch corruption at either the beginning or the
end of the struct.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 include/linux/mm_types.h |  6 ++++++
 include/linux/mmdebug.h  |  8 ++++++++
 kernel/fork.c            | 11 +++++++++++
 mm/debug.c               |  7 +++++++
 mm/mmap.c                |  2 ++
 mm/vmacache.c            |  2 ++
 6 files changed, 36 insertions(+)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 6e0b286..0b0d324 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -343,6 +343,9 @@ struct mm_rss_stat {
 
 struct kioctx_table;
 struct mm_struct {
+#ifdef CONFIG_DEBUG_VM_POISON
+	u32 poison_start;
+#endif
 	struct vm_area_struct *mmap;		/* list of VMAs */
 	struct rb_root mm_rb;
 	u32 vmacache_seqnum;                   /* per-thread vmacache */
@@ -454,6 +457,9 @@ struct mm_struct {
 	bool tlb_flush_pending;
 #endif
 	struct uprobes_state uprobes_state;
+#ifdef CONFIG_DEBUG_VM_POISON
+	u32 poison_end;
+#endif
 };
 
 static inline void mm_init_cpumask(struct mm_struct *mm)
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 7d05557..339e40f 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -39,6 +39,13 @@ void dump_mm(const struct mm_struct *mm);
 #define VM_WARN_ON(cond) WARN_ON(cond)
 #define VM_WARN_ON_ONCE(cond) WARN_ON_ONCE(cond)
 #define VM_WARN_ONCE(cond, format...) WARN_ONCE(cond, format)
+#ifdef CONFIG_DEBUG_VM_POISON
+#define VM_CHECK_POISON_MM(mm)						\
+	do {								\
+		VM_BUG_ON_MM((mm)->poison_start != MM_POISON_BEGIN, (mm));\
+		VM_BUG_ON_MM((mm)->poison_end != MM_POISON_END, (mm));	\
+	} while (0)
+#endif
 #else
 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_BUG_ON_PAGE(cond, page) VM_BUG_ON(cond)
@@ -47,6 +54,7 @@ void dump_mm(const struct mm_struct *mm);
 #define VM_WARN_ON(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
+#define VM_CHECK_POISON_MM(mm) do { } while(0)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL
diff --git a/kernel/fork.c b/kernel/fork.c
index 807633f..26bedfa 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -600,6 +600,8 @@ static void check_mm(struct mm_struct *mm)
 {
 	int i;
 
+	VM_CHECK_POISON_MM(mm);
+
 	for (i = 0; i < NR_MM_COUNTERS; i++) {
 		long x = atomic_long_read(&mm->rss_stat.count[i]);
 
@@ -624,6 +626,12 @@ struct mm_struct *mm_alloc(void)
 		return NULL;
 
 	memset(mm, 0, sizeof(*mm));
+
+#ifdef CONFIG_DEBUG_VM_POISON
+	mm->poison_start = MM_POISON_BEGIN;
+	mm->poison_end = MM_POISON_END;
+#endif
+
 	return mm_init(mm, current);
 }
 
@@ -650,6 +658,8 @@ void mmput(struct mm_struct *mm)
 {
 	might_sleep();
 
+	VM_CHECK_POISON_MM(mm);
+
 	if (atomic_dec_and_test(&mm->mm_users)) {
 		uprobe_clear_state(mm);
 		exit_aio(mm);
@@ -714,6 +724,7 @@ struct mm_struct *get_task_mm(struct task_struct *task)
 	task_lock(task);
 	mm = task->mm;
 	if (mm) {
+		VM_CHECK_POISON_MM(mm);
 		if (task->flags & PF_KTHREAD)
 			mm = NULL;
 		else
diff --git a/mm/debug.c b/mm/debug.c
index d699471..a1ebc5e 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -167,6 +167,9 @@ EXPORT_SYMBOL(dump_vma);
 void dump_mm(const struct mm_struct *mm)
 {
 	pr_emerg("mm %p mmap %p seqnum %d task_size %lu\n"
+#ifdef CONFIG_DEBUG_VM_POISON
+		"start poison: %s end poison: %s\n"
+#endif
 #ifdef CONFIG_MMU
 		"get_unmapped_area %p\n"
 #endif
@@ -197,6 +200,10 @@ void dump_mm(const struct mm_struct *mm)
 		"%s",	/* This is here to hold the comma */
 
 		mm, mm->mmap, mm->vmacache_seqnum, mm->task_size,
+#ifdef CONFIG_DEBUG_VM_POISON
+		(mm->poison_start == MM_POISON_BEGIN) ? "valid" : "invalid",
+		(mm->poison_end == MM_POISON_END) ? "valid" : "invalid",
+#endif
 #ifdef CONFIG_MMU
 		mm->get_unmapped_area,
 #endif
diff --git a/mm/mmap.c b/mm/mmap.c
index 9156612..3240bbc 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -469,6 +469,8 @@ static void validate_mm(struct mm_struct *mm)
 		bug = 1;
 	}
 	VM_BUG_ON_MM(bug, mm);
+
+	VM_CHECK_POISON_MM(mm);
 }
 #else
 #define validate_mm_rb(root, ignore) do { } while (0)
diff --git a/mm/vmacache.c b/mm/vmacache.c
index 9f25af8..d507caa 100644
--- a/mm/vmacache.c
+++ b/mm/vmacache.c
@@ -52,6 +52,8 @@ void vmacache_flush_all(struct mm_struct *mm)
  */
 static bool vmacache_valid_mm(struct mm_struct *mm)
 {
+	VM_CHECK_POISON_MM(mm);
+
 	return current->mm == mm && !(current->flags & PF_KTHREAD);
 }
 
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/5] mm: poison vm_area_struct

[Sasha Levin]

Add poisoning to vm_area_struct to catch corruption at either the beginning or
the end of the struct.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 fs/exec.c                |  5 +++++
 include/linux/mm_types.h |  6 ++++++
 include/linux/mmdebug.h  |  6 ++++++
 kernel/fork.c            |  2 ++
 mm/debug.c               | 11 +++++++++--
 mm/mmap.c                | 19 ++++++++++++++++++-
 mm/nommu.c               |  7 +++++++
 mm/vmacache.c            |  3 +++
 8 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 7302b75..6cdd652 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -257,6 +257,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 		return -ENOMEM;
 
 	down_write(&mm->mmap_sem);
+#ifdef CONFIG_DEBUG_VM_POISON
+	vma->poison_start = MM_POISON_BEGIN;
+	vma->poison_end = MM_POISON_END;
+#endif
 	vma->vm_mm = mm;
 
 	/*
@@ -283,6 +287,7 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 err:
 	up_write(&mm->mmap_sem);
 	bprm->vma = NULL;
+	VM_CHECK_POISON_VMA(vma);
 	kmem_cache_free(vm_area_cachep, vma);
 	return err;
 }
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 0b0d324..4e2cf93 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -245,6 +245,9 @@ struct vm_region {
  * library, the executable area etc).
  */
 struct vm_area_struct {
+#ifdef CONFIG_DEBUG_VM_POISON
+	u32 poison_start;
+#endif
 	/* The first cache line has the info for VMA tree walking. */
 
 	unsigned long vm_start;		/* Our start address within vm_mm. */
@@ -308,6 +311,9 @@ struct vm_area_struct {
 #ifdef CONFIG_NUMA
 	struct mempolicy *vm_policy;	/* NUMA policy for the VMA */
 #endif
+#ifdef CONFIG_DEBUG_VM_POISON
+	u32 poison_end;
+#endif
 };
 
 struct core_thread {
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 339e40f..75bc69d 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -45,6 +45,11 @@ void dump_mm(const struct mm_struct *mm);
 		VM_BUG_ON_MM((mm)->poison_start != MM_POISON_BEGIN, (mm));\
 		VM_BUG_ON_MM((mm)->poison_end != MM_POISON_END, (mm));	\
 	} while (0)
+#define VM_CHECK_POISON_VMA(vma)					\
+	do {                                                            \
+		VM_BUG_ON_VMA((vma)->poison_start != MM_POISON_BEGIN, (vma));\
+		VM_BUG_ON_VMA((vma)->poison_end != MM_POISON_END, (vma));\
+	} while (0)
 #endif
 #else
 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
@@ -55,6 +60,7 @@ void dump_mm(const struct mm_struct *mm);
 #define VM_WARN_ON_ONCE(cond) BUILD_BUG_ON_INVALID(cond)
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #define VM_CHECK_POISON_MM(mm) do { } while(0)
+#define VM_CHECK_POISON_VMA(vma) do { } while(0)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL
diff --git a/kernel/fork.c b/kernel/fork.c
index 26bedfa..c3ae913 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -415,6 +415,7 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
 		tmp = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
 		if (!tmp)
 			goto fail_nomem;
+		VM_CHECK_POISON_VMA(mpnt);
 		*tmp = *mpnt;
 		INIT_LIST_HEAD(&tmp->anon_vma_chain);
 		retval = vma_dup_policy(mpnt, tmp);
@@ -489,6 +490,7 @@ out:
 fail_nomem_anon_vma_fork:
 	mpol_put(vma_policy(tmp));
 fail_nomem_policy:
+	VM_CHECK_POISON_VMA(tmp);
 	kmem_cache_free(vm_area_cachep, tmp);
 fail_nomem:
 	retval = -ENOMEM;
diff --git a/mm/debug.c b/mm/debug.c
index a1ebc5e..d53174e 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -152,11 +152,18 @@ static const struct trace_print_flags vmaflags_names[] = {
 void dump_vma(const struct vm_area_struct *vma)
 {
 	pr_emerg("vma %p start %p end %p\n"
+#ifdef CONFIG_DEBUG_VM_POISON
+		"start poison: %s end poison: %s\n"
+#endif
 		"next %p prev %p mm %p\n"
 		"prot %lx anon_vma %p vm_ops %p\n"
 		"pgoff %lx file %p private_data %p\n",
-		vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_next,
-		vma->vm_prev, vma->vm_mm,
+		vma, (void *)vma->vm_start, (void *)vma->vm_end,
+#ifdef CONFIG_DEBUG_VM_POISON
+		(vma->poison_start == MM_POISON_BEGIN) ? "valid" : "invalid",
+		(vma->poison_end == MM_POISON_END) ? "valid" : "invalid",
+#endif
+		vma->vm_next, vma->vm_prev, vma->vm_mm,
 		(unsigned long)pgprot_val(vma->vm_page_prot),
 		vma->anon_vma, vma->vm_ops, vma->vm_pgoff,
 		vma->vm_file, vma->vm_private_data);
diff --git a/mm/mmap.c b/mm/mmap.c
index 3240bbc..da5ffeb 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -274,6 +274,7 @@ static struct vm_area_struct *remove_vma(struct vm_area_struct *vma)
 	struct vm_area_struct *next = vma->vm_next;
 
 	might_sleep();
+	VM_CHECK_POISON_VMA(vma);
 	if (vma->vm_ops && vma->vm_ops->close)
 		vma->vm_ops->close(vma);
 	if (vma->vm_file)
@@ -900,6 +901,7 @@ again:			remove_next = 1 + (end > next->vm_end);
 			anon_vma_merge(vma, next);
 		mm->map_count--;
 		mpol_put(vma_policy(next));
+		VM_CHECK_POISON_VMA(next);
 		kmem_cache_free(vm_area_cachep, next);
 		/*
 		 * In mprotect's case 6 (see comments on vma_merge),
@@ -1594,6 +1596,10 @@ munmap_back:
 		goto unacct_error;
 	}
 
+#ifdef CONFIG_DEBUG_VM_POISON
+	vma->poison_start = MM_POISON_BEGIN;
+	vma->poison_end = MM_POISON_END;
+#endif
 	vma->vm_mm = mm;
 	vma->vm_start = addr;
 	vma->vm_end = addr + len;
@@ -1691,6 +1697,7 @@ allow_write_and_free_vma:
 	if (vm_flags & VM_DENYWRITE)
 		allow_write_access(file);
 free_vma:
+	VM_CHECK_POISON_VMA(vma);
 	kmem_cache_free(vm_area_cachep, vma);
 unacct_error:
 	if (charged)
@@ -2454,7 +2461,7 @@ static int __split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
 	new = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
 	if (!new)
 		goto out_err;
-
+	VM_CHECK_POISON_VMA(vma);
 	/* most fields are the same, copy all, and then fixup */
 	*new = *vma;
 
@@ -2499,6 +2506,7 @@ static int __split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
  out_free_mpol:
 	mpol_put(vma_policy(new));
  out_free_vma:
+	VM_CHECK_POISON_VMA(new);
 	kmem_cache_free(vm_area_cachep, new);
  out_err:
 	return err;
@@ -2771,6 +2779,10 @@ static unsigned long do_brk(unsigned long addr, unsigned long len)
 		return -ENOMEM;
 	}
 
+#ifdef CONFIG_DEBUG_VM_POISON
+	vma->poison_start = MM_POISON_BEGIN;
+	vma->poison_end = MM_POISON_END;
+#endif
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
 	vma->vm_mm = mm;
 	vma->vm_start = addr;
@@ -2942,6 +2954,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 		*need_rmap_locks = (new_vma->vm_pgoff <= vma->vm_pgoff);
 	} else {
 		new_vma = kmem_cache_alloc(vm_area_cachep, GFP_KERNEL);
+		VM_CHECK_POISON_VMA(vma);
 		if (new_vma) {
 			*new_vma = *vma;
 			new_vma->vm_start = addr;
@@ -3058,6 +3071,10 @@ static struct vm_area_struct *__install_special_mapping(
 		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(&vma->anon_vma_chain);
+#ifdef CONFIG_DEBUG_VM_POISON
+	vma->poison_start = MM_POISON_BEGIN;
+	vma->poison_end = MM_POISON_END;
+#endif
 	vma->vm_mm = mm;
 	vma->vm_start = addr;
 	vma->vm_end = addr + len;
diff --git a/mm/nommu.c b/mm/nommu.c
index bd10aa1..e81b656 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -820,6 +820,7 @@ static void delete_vma_from_mm(struct vm_area_struct *vma)
 static void delete_vma(struct mm_struct *mm, struct vm_area_struct *vma)
 {
 	kenter("%p", vma);
+	VM_CHECK_POISON_VMA(vma);
 	if (vma->vm_ops && vma->vm_ops->close)
 		vma->vm_ops->close(vma);
 	if (vma->vm_file)
@@ -1302,6 +1303,10 @@ unsigned long do_mmap_pgoff(struct file *file,
 	if (!vma)
 		goto error_getting_vma;
 
+#ifdef CONFIG_DEBUG_VM_POISON
+	vma->poison_start = MM_POISON_BEGIN;
+	vma->poison_end = MM_POISON_END;
+#endif
 	region->vm_usage = 1;
 	region->vm_flags = vm_flags;
 	region->vm_pgoff = pgoff;
@@ -1465,6 +1470,7 @@ error:
 	kmem_cache_free(vm_region_jar, region);
 	if (vma->vm_file)
 		fput(vma->vm_file);
+	VM_CHECK_POISON_VMA(vma);
 	kmem_cache_free(vm_area_cachep, vma);
 	kleave(" = %d", ret);
 	return ret;
@@ -1571,6 +1577,7 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
 	}
 
 	/* most fields are the same, copy all, and then fixup */
+	VM_CHECK_POISON_VMA(vma);
 	*new = *vma;
 	*region = *vma->vm_region;
 	new->vm_region = region;
diff --git a/mm/vmacache.c b/mm/vmacache.c
index d507caa..27760cf 100644
--- a/mm/vmacache.c
+++ b/mm/vmacache.c
@@ -59,6 +59,7 @@ static bool vmacache_valid_mm(struct mm_struct *mm)
 
 void vmacache_update(unsigned long addr, struct vm_area_struct *newvma)
 {
+	VM_CHECK_POISON_VMA(newvma);
 	if (vmacache_valid_mm(newvma->vm_mm))
 		current->vmacache[VMACACHE_HASH(addr)] = newvma;
 }
@@ -99,6 +100,7 @@ struct vm_area_struct *vmacache_find(struct mm_struct *mm, unsigned long addr)
 			continue;
 		if (WARN_ON_ONCE(vma->vm_mm != mm))
 			break;
+		VM_CHECK_POISON_VMA(vma);
 		if (vma->vm_start <= addr && vma->vm_end > addr) {
 			count_vm_vmacache_event(VMACACHE_FIND_HITS);
 			return vma;
@@ -123,6 +125,7 @@ struct vm_area_struct *vmacache_find_exact(struct mm_struct *mm,
 	for (i = 0; i < VMACACHE_SIZE; i++) {
 		struct vm_area_struct *vma = current->vmacache[i];
 
+		VM_CHECK_POISON_VMA(vma);
 		if (vma && vma->vm_start == start && vma->vm_end == end) {
 			count_vm_vmacache_event(VMACACHE_FIND_HITS);
 			return vma;
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 5/5] mm: poison page struct

[Sasha Levin]

Add poisoning to page struct to catch corruption at either the beginning or
the end of the struct.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 include/linux/mm.h         |  9 +++++++++
 include/linux/mm_types.h   |  6 ++++++
 include/linux/mmdebug.h    |  6 ++++++
 include/linux/page-flags.h | 24 ++++++++++++++++--------
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0c13412..c48c4e2 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -524,6 +524,10 @@ static inline struct page *virt_to_head_page(const void *x)
  */
 static inline void init_page_count(struct page *page)
 {
+#ifdef CONFIG_DEBUG_VM_POISON
+	page->poison_start = MM_POISON_BEGIN;
+	page->poison_end = MM_POISON_END;
+#endif
 	atomic_set(&page->_count, 1);
 }
 
@@ -1482,12 +1486,17 @@ static inline void pgtable_init(void)
 
 static inline bool pgtable_page_ctor(struct page *page)
 {
+#ifdef CONFIG_DEBUG_VM_POISON
+	page->poison_start = MM_POISON_BEGIN;
+	page->poison_end = MM_POISON_END;
+#endif
 	inc_zone_page_state(page, NR_PAGETABLE);
 	return ptlock_init(page);
 }
 
 static inline void pgtable_page_dtor(struct page *page)
 {
+	VM_CHECK_POISON_PAGE(page);
 	pte_lock_deinit(page);
 	dec_zone_page_state(page, NR_PAGETABLE);
 }
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 4e2cf93..7cab56a 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -42,6 +42,9 @@ struct address_space;
  * and lru list pointers also.
  */
 struct page {
+#ifdef CONFIG_DEBUG_VM_POISON
+	u32 poison_start;
+#endif
 	/* First double word block */
 	unsigned long flags;		/* Atomic flags, some possibly
 					 * updated asynchronously */
@@ -196,6 +199,9 @@ struct page {
 #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
 	int _last_cpupid;
 #endif
+#ifdef CONFIG_DEBUG_VM_POISON
+	u32 poison_end;
+#endif
 }
 /*
  * The struct page can be forced to be double word aligned so that atomic ops
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 75bc69d..461c452 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -50,6 +50,11 @@ void dump_mm(const struct mm_struct *mm);
 		VM_BUG_ON_VMA((vma)->poison_start != MM_POISON_BEGIN, (vma));\
 		VM_BUG_ON_VMA((vma)->poison_end != MM_POISON_END, (vma));\
 	} while (0)
+#define VM_CHECK_POISON_PAGE(page)					\
+	do {                                                            \
+		VM_BUG_ON_PAGE((page)->poison_start != MM_POISON_BEGIN, (page));\
+		VM_BUG_ON_PAGE((page)->poison_end != MM_POISON_END, (page));\
+	} while (0)
 #endif
 #else
 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)
@@ -61,6 +66,7 @@ void dump_mm(const struct mm_struct *mm);
 #define VM_WARN_ONCE(cond, format...) BUILD_BUG_ON_INVALID(cond)
 #define VM_CHECK_POISON_MM(mm) do { } while(0)
 #define VM_CHECK_POISON_VMA(vma) do { } while(0)
+#define VM_CHECK_POISON_PAGE(page) do { } while(0)
 #endif
 
 #ifdef CONFIG_DEBUG_VIRTUAL
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index e1f5fcd..688f72c 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -135,35 +135,43 @@ enum pageflags {
  */
 #define TESTPAGEFLAG(uname, lname)					\
 static inline int Page##uname(const struct page *page)			\
-			{ return test_bit(PG_##lname, &page->flags); }
+			{ VM_CHECK_POISON_PAGE(page);			\
+			  return test_bit(PG_##lname, &page->flags); }
 
 #define SETPAGEFLAG(uname, lname)					\
 static inline void SetPage##uname(struct page *page)			\
-			{ set_bit(PG_##lname, &page->flags); }
+			{ VM_CHECK_POISON_PAGE(page);			\
+			  set_bit(PG_##lname, &page->flags); }
 
 #define CLEARPAGEFLAG(uname, lname)					\
 static inline void ClearPage##uname(struct page *page)			\
-			{ clear_bit(PG_##lname, &page->flags); }
+			{ VM_CHECK_POISON_PAGE(page);			\
+			  clear_bit(PG_##lname, &page->flags); }
 
 #define __SETPAGEFLAG(uname, lname)					\
 static inline void __SetPage##uname(struct page *page)			\
-			{ __set_bit(PG_##lname, &page->flags); }
+			{ VM_CHECK_POISON_PAGE(page);			\
+			  __set_bit(PG_##lname, &page->flags); }
 
 #define __CLEARPAGEFLAG(uname, lname)					\
 static inline void __ClearPage##uname(struct page *page)		\
-			{ __clear_bit(PG_##lname, &page->flags); }
+			{ VM_CHECK_POISON_PAGE(page);			\
+			  __clear_bit(PG_##lname, &page->flags); }
 
 #define TESTSETFLAG(uname, lname)					\
 static inline int TestSetPage##uname(struct page *page)			\
-		{ return test_and_set_bit(PG_##lname, &page->flags); }
+		{ VM_CHECK_POISON_PAGE(page);				\
+		  return test_and_set_bit(PG_##lname, &page->flags); }
 
 #define TESTCLEARFLAG(uname, lname)					\
 static inline int TestClearPage##uname(struct page *page)		\
-		{ return test_and_clear_bit(PG_##lname, &page->flags); }
+		{ VM_CHECK_POISON_PAGE(page);				\
+		  return test_and_clear_bit(PG_##lname, &page->flags); }
 
 #define __TESTCLEARFLAG(uname, lname)					\
 static inline int __TestClearPage##uname(struct page *page)		\
-		{ return __test_and_clear_bit(PG_##lname, &page->flags); }
+		{ VM_CHECK_POISON_PAGE(page);				\
+		  return __test_and_clear_bit(PG_##lname, &page->flags); }
 
 #define PAGEFLAG(uname, lname) TESTPAGEFLAG(uname, lname)		\
 	SETPAGEFLAG(uname, lname) CLEARPAGEFLAG(uname, lname)
-- 
1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: poison page struct

[Dave Hansen]

On 09/29/2014 06:47 PM, Sasha Levin wrote:
>  struct page {
> +#ifdef CONFIG_DEBUG_VM_POISON
> +	u32 poison_start;
> +#endif
>  	/* First double word block */
>  	unsigned long flags;		/* Atomic flags, some possibly
>  					 * updated asynchronously */
> @@ -196,6 +199,9 @@ struct page {
>  #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
>  	int _last_cpupid;
>  #endif
> +#ifdef CONFIG_DEBUG_VM_POISON
> +	u32 poison_end;
> +#endif
>  }

Does this break slub's __cmpxchg_double_slab trick?  I thought it
required page->freelist and page->counters to be doubleword-aligned.

It's not like we really require this optimization when we're debugging,
but trying to use it will unnecessarily slow things down.

FWIW, if you're looking to trim down the number of lines of code, you
could certainly play some macro tricks and #ifdef tricks.

struct vm_poison {
#ifdef CONFIG_DEBUG_VM_POISON
	u32 val;
#endif	
};

Then, instead of #ifdefs in each structure, you do:

struct page {
	struct vm_poison poison_start;
	... other gunk
	struct vm_poison poison_end;
};

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: poison page struct

[Christoph Lameter]

On Tue, 7 Oct 2014, Dave Hansen wrote:

> Does this break slub's __cmpxchg_double_slab trick?  I thought it
> required page->freelist and page->counters to be doubleword-aligned.

Sure that would be required for it to work.

> It's not like we really require this optimization when we're debugging,
> but trying to use it will unnecessarily slow things down.

Debugging by inserting more data into the page struct will already cause a
significant slow down because the cache footprint of key functions will
increase significantly. I would think that using the fallback functions
is reasonable in this scenario,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 5/5] mm: poison page struct

[Sasha Levin]

On 10/07/2014 06:02 PM, Dave Hansen wrote:
> On 09/29/2014 06:47 PM, Sasha Levin wrote:
>>  struct page {
>> +#ifdef CONFIG_DEBUG_VM_POISON
>> +	u32 poison_start;
>> +#endif
>>  	/* First double word block */
>>  	unsigned long flags;		/* Atomic flags, some possibly
>>  					 * updated asynchronously */
>> @@ -196,6 +199,9 @@ struct page {
>>  #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
>>  	int _last_cpupid;
>>  #endif
>> +#ifdef CONFIG_DEBUG_VM_POISON
>> +	u32 poison_end;
>> +#endif
>>  }
> 
> Does this break slub's __cmpxchg_double_slab trick?  I thought it
> required page->freelist and page->counters to be doubleword-aligned.

I'll probably have to switch it to 8 bytes anyways to make it work with
kasan. This should take care of the slub optimization as well.

> It's not like we really require this optimization when we're debugging,
> but trying to use it will unnecessarily slow things down.
> 
> FWIW, if you're looking to trim down the number of lines of code, you
> could certainly play some macro tricks and #ifdef tricks.
> 
> struct vm_poison {
> #ifdef CONFIG_DEBUG_VM_POISON
> 	u32 val;
> #endif	
> };
> 
> Then, instead of #ifdefs in each structure, you do:
> 
> struct page {
> 	struct vm_poison poison_start;
> 	... other gunk
> 	struct vm_poison poison_end;
> };

Agreed, I'll reword that in the next version.


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Andrew Morton]

On Mon, 29 Sep 2014 21:47:14 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:

> Currently we're seeing a few issues which are unexplainable by looking at the
> data we see and are most likely caused by a memory corruption caused
> elsewhere.
> 
> This is wasting time for folks who are trying to figure out an issue provided
> a stack trace that can't really point out the real issue.
> 
> This patch introduces poisoning on struct page, vm_area_struct, and mm_struct,
> and places checks in busy paths to catch corruption early.
> 
> This series was tested, and it detects corruption in vm_area_struct. Right now
> I'm working on figuring out the source of the corruption, (which is a long
> standing bug) using KASan, but the current code is useful as it is.

Is this still useful if/when kasan is in place?

It looks fairly cheap - I wonder if it should simply fall under
CONFIG_DEBUG_VM rather than the new CONFIG_DEBUG_VM_POISON.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Sasha Levin]

On 10/01/2014 05:07 PM, Andrew Morton wrote:
> On Mon, 29 Sep 2014 21:47:14 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
> 
>> Currently we're seeing a few issues which are unexplainable by looking at the
>> data we see and are most likely caused by a memory corruption caused
>> elsewhere.
>>
>> This is wasting time for folks who are trying to figure out an issue provided
>> a stack trace that can't really point out the real issue.
>>
>> This patch introduces poisoning on struct page, vm_area_struct, and mm_struct,
>> and places checks in busy paths to catch corruption early.
>>
>> This series was tested, and it detects corruption in vm_area_struct. Right now
>> I'm working on figuring out the source of the corruption, (which is a long
>> standing bug) using KASan, but the current code is useful as it is.
> 
> Is this still useful if/when kasan is in place?

Yes, the corruption we're seeing happens inside the struct rather than around it.
kasan doesn't look there.

When kasan is merged, we could complement this patchset by making kasan trap on
when the poison is getting written, rather than triggering a BUG in some place
else after we saw the corruption.

> It looks fairly cheap - I wonder if it should simply fall under
> CONFIG_DEBUG_VM rather than the new CONFIG_DEBUG_VM_POISON.

Config options are cheap as well :)

I'd rather expand it further and add poison/kasan trapping into other places such
as the vma interval tree rather than having to keep it "cheap".


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Andrew Morton]

On Wed, 01 Oct 2014 17:39:39 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:

> > It looks fairly cheap - I wonder if it should simply fall under
> > CONFIG_DEBUG_VM rather than the new CONFIG_DEBUG_VM_POISON.
> 
> Config options are cheap as well :)

Thing is, lots of people are enabling CONFIG_DEBUG_VM, but a smaller
number of people will enable CONFIG_DEBUG_VM_POISON.  Less coverage. 

Defaulting to y if CONFIG_DEBUG_VM might help, but if people do `make
oldconfig' when CONFIG_DEBUG_VM=n, their CONFIG_DEBUG_VM_POISON will
get set to `n' and will remain that way when they set CONFIG_DEBUG_VM
again.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Sasha Levin]

On 10/01/2014 05:48 PM, Andrew Morton wrote:
> On Wed, 01 Oct 2014 17:39:39 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
> 
>>> It looks fairly cheap - I wonder if it should simply fall under
>>> CONFIG_DEBUG_VM rather than the new CONFIG_DEBUG_VM_POISON.
>>
>> Config options are cheap as well :)
> 
> Thing is, lots of people are enabling CONFIG_DEBUG_VM, but a smaller
> number of people will enable CONFIG_DEBUG_VM_POISON.  Less coverage. 
> 
> Defaulting to y if CONFIG_DEBUG_VM might help, but if people do `make
> oldconfig' when CONFIG_DEBUG_VM=n, their CONFIG_DEBUG_VM_POISON will
> get set to `n' and will remain that way when they set CONFIG_DEBUG_VM
> again.

In that case, what about:

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index db41b15..b2c7038 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -546,6 +546,7 @@ config DEBUG_VM_RB
 config DEBUG_VM_POISON
        bool "Poison VM structures"
        depends on DEBUG_VM
+       def_bool y
        help
          Add poison to the beggining and end of various VM structure to
          detect memory corruption in VM management code.

We'll default to "Y" in 'make oldconfig' and it'll automatically be switched
on when the user selects CONFIG_DEBUG_VM=y, but we still keep the advantages
of having it in a different config option.


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Hugh Dickins]

On Wed, 1 Oct 2014, Sasha Levin wrote:
> On 10/01/2014 05:07 PM, Andrew Morton wrote:
> > On Mon, 29 Sep 2014 21:47:14 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
> > 
> >> Currently we're seeing a few issues which are unexplainable by looking at the
> >> data we see and are most likely caused by a memory corruption caused
> >> elsewhere.
> >>
> >> This is wasting time for folks who are trying to figure out an issue provided
> >> a stack trace that can't really point out the real issue.
> >>
> >> This patch introduces poisoning on struct page, vm_area_struct, and mm_struct,
> >> and places checks in busy paths to catch corruption early.
> >>
> >> This series was tested, and it detects corruption in vm_area_struct. Right now
> >> I'm working on figuring out the source of the corruption, (which is a long
> >> standing bug) using KASan, but the current code is useful as it is.
> > 
> > Is this still useful if/when kasan is in place?
> 
> Yes, the corruption we're seeing happens inside the struct rather than around it.
> kasan doesn't look there.
> 
> When kasan is merged, we could complement this patchset by making kasan trap on
> when the poison is getting written, rather than triggering a BUG in some place
> else after we saw the corruption.
> 
> > It looks fairly cheap - I wonder if it should simply fall under
> > CONFIG_DEBUG_VM rather than the new CONFIG_DEBUG_VM_POISON.
> 
> Config options are cheap as well :)
> 
> I'd rather expand it further and add poison/kasan trapping into other places such
> as the vma interval tree rather than having to keep it "cheap".

I like to run with CONFIG_DEBUG_VM, and would not want this stuff
turned on in my builds (especially not the struct page enlargement);
so I'm certainly with you in preferring a separate option.

But it all seems very ad hoc to me.  Are people going to be adding
more and more mm structures into it, ad infinitum?  And adding
CONFIG_DEBUG_SCHED_POISON one day when someone notices corruption
of a scheduler structure? etc etc.

What does this add on top of slab poisoning?  Some checks in some
mm places while the object is active, I guess: why not base those
on slab poisoning?  And add them in as appropriate to the problem
at hand, when a problem is seen.

I think these patches are fine for investigating whatever is the
problem currently afflicting you and mm under trinity; but we all
have our temporary debugging patches, I don't think all deserve
preservation in everyone else's kernel, that amounts to far more
clutter than any are worth.

I'm glad to hear they've confirmed some vm_area_struct corruption:
any ideas on where that's coming from?

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Sasha Levin]

On 10/02/2014 05:23 AM, Hugh Dickins wrote:
> On Wed, 1 Oct 2014, Sasha Levin wrote:
>> On 10/01/2014 05:07 PM, Andrew Morton wrote:
>>> On Mon, 29 Sep 2014 21:47:14 -0400 Sasha Levin <sasha.levin@oracle.com> wrote:
>>>
>>>> Currently we're seeing a few issues which are unexplainable by looking at the
>>>> data we see and are most likely caused by a memory corruption caused
>>>> elsewhere.
>>>>
>>>> This is wasting time for folks who are trying to figure out an issue provided
>>>> a stack trace that can't really point out the real issue.
>>>>
>>>> This patch introduces poisoning on struct page, vm_area_struct, and mm_struct,
>>>> and places checks in busy paths to catch corruption early.
>>>>
>>>> This series was tested, and it detects corruption in vm_area_struct. Right now
>>>> I'm working on figuring out the source of the corruption, (which is a long
>>>> standing bug) using KASan, but the current code is useful as it is.
>>>
>>> Is this still useful if/when kasan is in place?
>>
>> Yes, the corruption we're seeing happens inside the struct rather than around it.
>> kasan doesn't look there.
>>
>> When kasan is merged, we could complement this patchset by making kasan trap on
>> when the poison is getting written, rather than triggering a BUG in some place
>> else after we saw the corruption.
>>
>>> It looks fairly cheap - I wonder if it should simply fall under
>>> CONFIG_DEBUG_VM rather than the new CONFIG_DEBUG_VM_POISON.
>>
>> Config options are cheap as well :)
>>
>> I'd rather expand it further and add poison/kasan trapping into other places such
>> as the vma interval tree rather than having to keep it "cheap".
> 
> I like to run with CONFIG_DEBUG_VM, and would not want this stuff
> turned on in my builds (especially not the struct page enlargement);
> so I'm certainly with you in preferring a separate option.
> 
> But it all seems very ad hoc to me.  Are people going to be adding
> more and more mm structures into it, ad infinitum?  And adding
> CONFIG_DEBUG_SCHED_POISON one day when someone notices corruption
> of a scheduler structure? etc etc.

That was my plan, yes.

> What does this add on top of slab poisoning?  Some checks in some
> mm places while the object is active, I guess: why not base those
> on slab poisoning?  And add them in as appropriate to the problem
> at hand, when a problem is seen.

The extra you're getting is detecting corruption that happened
inside the object rather than around it. In the case of poisoning
working along with kasan you don't have to limit it to slab either,
so you can detect issues in static objects as well.

fwiw, there's currently a long standing issue with corruption inside
spinlocks in sched code. This sort of issues always exist, so (at least)
my kernel would always have poisoning in some form from now on.

> I think these patches are fine for investigating whatever is the
> problem currently afflicting you and mm under trinity; but we all
> have our temporary debugging patches, I don't think all deserve
> preservation in everyone else's kernel, that amounts to far more
> clutter than any are worth.

If the issue is lines of code we can look into making it cleaner.

> I'm glad to hear they've confirmed some vm_area_struct corruption:
> any ideas on where that's coming from?

Nope, I've added kasan poisoning to vm_area_struct but it has not
reproduced since then, I've just hit bunch of different issues.


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Dave Hansen]

On 10/02/2014 07:58 AM, Sasha Levin wrote:
>> > What does this add on top of slab poisoning?  Some checks in some
>> > mm places while the object is active, I guess: why not base those
>> > on slab poisoning?  And add them in as appropriate to the problem
>> > at hand, when a problem is seen.
> The extra you're getting is detecting corruption that happened
> inside the object rather than around it.

Isn't this more akin to redzoning that poisoning?

I'm not sure I follow the logic here.  The poison is inside the object,
but it's now at the edges.  With slub at least, you get a redzone right
after the object(s):

	{ OBJ } | REDZONE | { OBJ } | REDZONE | ...

With this patch, you'd get something along these lines:

	{ POISON | OBJ | POISON } { POISON | OBJ | POISON }  ...

So if somebody overflows OBJ, they'll hit the redzone/poison in either
case.  If they're randomly scribbling on memory, their likelihood of
hitting the redzone/poison is proportional to the size of the
redzone/poison.

The only place this really helps is if someone overflows from a
non-redzoned page or structure in to the beginning of a slub redzoned
one.  The fact that the redzone is at the end means we'll miss it.

But, all that means is that we should probably add redzones to the
beginning of slub objects, not just the end.  That doesn't help us with
'struct page' of course, but it does for the mm_struct and vma.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Sasha Levin]

On 10/07/2014 06:16 PM, Dave Hansen wrote:
> On 10/02/2014 07:58 AM, Sasha Levin wrote:
>>>> What does this add on top of slab poisoning?  Some checks in some
>>>> mm places while the object is active, I guess: why not base those
>>>> on slab poisoning?  And add them in as appropriate to the problem
>>>> at hand, when a problem is seen.
>> The extra you're getting is detecting corruption that happened
>> inside the object rather than around it.
> 
> Isn't this more akin to redzoning that poisoning?
> 
> I'm not sure I follow the logic here.  The poison is inside the object,
> but it's now at the edges.  With slub at least, you get a redzone right
> after the object(s):
> 
> 	{ OBJ } | REDZONE | { OBJ } | REDZONE | ...
> 
> With this patch, you'd get something along these lines:
> 
> 	{ POISON | OBJ | POISON } { POISON | OBJ | POISON }  ...
> 
> So if somebody overflows OBJ, they'll hit the redzone/poison in either
> case.  If they're randomly scribbling on memory, their likelihood of
> hitting the redzone/poison is proportional to the size of the
> redzone/poison.
> 
> The only place this really helps is if someone overflows from a
> non-redzoned page or structure in to the beginning of a slub redzoned
> one.  The fact that the redzone is at the end means we'll miss it.
> 
> But, all that means is that we should probably add redzones to the
> beginning of slub objects, not just the end.  That doesn't help us with
> 'struct page' of course, but it does for the mm_struct and vma.

This patchset is based on an actual issue we're seeing where the vma
gets corrupted without triggering any of the slub redzones.

Testing this patchset locally confirmed that while slub redzones stay
intact, the poison fields get overwritten - so now we're able to catch
the corruption after it happened.

I'm not sure what's the scenario that causes that, but once we figure
that out I could have a better response to your question...


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Dave Jones]

On Thu, Oct 02, 2014 at 02:23:08AM -0700, Hugh Dickins wrote:

 > I think these patches are fine for investigating whatever is the
 > problem currently afflicting you and mm under trinity; but we all
 > have our temporary debugging patches, I don't think all deserve
 > preservation in everyone else's kernel, that amounts to far more
 > clutter than any are worth.

One problem with keeping things like this in -mm (or other non-Linus tree)
is that they bit-rot quickly, and become a pain to apply, especially if
they are perpetually on top of other changes in -mm.

I looked at trying these patches on Linus' tree when Sasha posted them,
but lost motivation when I realized they needed other bits of -mm too.

It may be that after Andrews 3.18+ mega-merge things would be simpler,
but I have a feeling it wouldn't be long before the situation would
arise again.

	Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/5] mm: poison critical mm/ structs

[Sasha Levin]

On 10/02/2014 05:23 AM, Hugh Dickins wrote:
> I'm glad to hear they've confirmed some vm_area_struct corruption:
> any ideas on where that's coming from?

Hugh,

I think that what we're seeing isn't a corruption of vm_area_struct
per-se, but something weirder.

I've poisoned every spot where vm_area_struct is allocated, and yet
there seems to be nothing that's hitting that field before we end
up using a "zeroed out" vm_area_struct.

The results are the same both with and without kasan, there seems
to be no corruption happening anywhere, but we somehow end up with
an empty vm_area_struct.

It also somewhat makes sense considering that we're seeing no slub
corruption either. Either something is zeroing out *exactly*
vm_area_struct, or it's not really corruption...


Thanks,
Sasha

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>